fix(cdk): only make Outputs Exports when necessary #1624
Conversation
Export names must be unique and can conflict, so automatically turning every Output into an Export can lead to problems for customers who deploy the same template multiple times. Especially when the outputs are created for them and they have no control over them. We'll turn Outputs into exports on-demand (when .makeImportValue() is called). Fixes #903, fixes #1611.
| */ | ||
| export?: string; | ||
|
|
||
| /** | ||
| * Disables the automatic allocation of an export name for this output. | ||
| * | ||
| * @default false, which means that an export name is either explicitly | ||
| * specified or allocated based on the output's logical ID and stack name. | ||
| * This disables use of `makeImportValue()` if `export` is not given. |
There was a problem hiding this comment.
I would say prohibits rather than disables here... What is disabled is auto-allocation of an export name, and the corollary is you cannot use makeImportValue() unless an export name was provided at creation time.
| @@ -76,16 +81,16 @@ export class Output extends StackElement { | |||
| this._value = props.value; | |||
| this.condition = props.condition; | |||
|
|
|||
| this.disableExport = props.disableExport !== undefined ? props.disableExport : false; | |||
There was a problem hiding this comment.
!!props.disableExport ?
| this.disableExport = props.disableExport !== undefined ? props.disableExport : false; | ||
|
|
||
| if (props.export && this.disableExport) { | ||
| throw new Error('Cannot set `disableExport` and specify an export name'); |
There was a problem hiding this comment.
This seems to contradict what the props documentation says. But TBH I prefer this... And I'd suggest replacing disableExport with something like exportable (semantic is flipped: if !exportable, then you cannot provide - or have allocated - an export name).
| */ | ||
| private uniqueOutputName() { | ||
| // prefix export name with stack name since exports are global within account + region. | ||
| const stackName = require('./stack').Stack.find(this).name; |
There was a problem hiding this comment.
I do not like this inline require call. Why not import Stack normally?
There was a problem hiding this comment.
Cyclic dependencies. The core library is rife with them. :(
| private uniqueOutputName() { | ||
| // prefix export name with stack name since exports are global within account + region. | ||
| const stackName = require('./stack').Stack.find(this).name; | ||
| return (stackName ? stackName + ':' : '') + this.logicalId; |
There was a problem hiding this comment.
Why bother for the case of an un-named stack? I guess this only ever happens in unit tests, where this shouldn't matter. I'd be up for using
return `${stackName}:${this.logicalId}`;
|
@rix0rrr any updates? |
|
This has not been added to the Golang CDK library. Exports are automatically created. |
Export names must be unique and can conflict, so automatically turning
every Output into an Export can lead to problems for customers who
deploy the same template multiple times. Especially when the outputs
are created for them and they have no control over them.
We'll turn Outputs into exports on-demand (when .makeImportValue() is
called).
Fixes #903, fixes #1611.
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.