-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): overrideLogicalId: override IDs of CFN elements #1670
Conversation
Allow users to explicitly override the logical ID of a CloudFormation element (such as "Cfn" resources) by invoking `overrideLogicalId` on the resource object. For example: const bucket = new s3.CfnBucket(this, 'MyBucket'); bucket.overrideLogicalId('YourBucket'); The resulting template will use `YourBucket` as the logical ID. NOTE: the `logicalId` property will now return a stringified token instead of a concrete value. Fixes #1594
@Doug-AWS we should update the escape hatch topic to reflect this (instead of renames) |
There is no good reason to be strict about token display names. We can just replace any disallowed chars with "." and get it over with.
@@ -197,7 +201,7 @@ export class Resource extends Referenceable { | |||
Type: this.resourceType, | |||
Properties: ignoreEmpty(this, properties), | |||
// Return a sorted set of dependencies to be consistent across tests | |||
DependsOn: ignoreEmpty(this, sortedSet(this.dependsOn)), | |||
DependsOn: ignoreEmpty(this, sortedSet(this.dependsOn).map(r => r.logicalId)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of sortedSet(this.dependsOn)
will be different now that it is a Set<Resource>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will actually be fine, because it will be sorted by the token, which is good enough for our purposes, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to get dependencies into a stable order for purposes of comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the tokens are okay for that purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you're doing sort(Resource)
THEN map to ID.
So the sort order is not based on the token strings, but on opaque objects. No idea how JavaScript will sort those.
If anything, you should have done sort(resource.map(r => r.id))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, you are right
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Allow users to explicitly override the logical ID of a CloudFormation
element (such as "Cfn" resources) by invoking
overrideLogicalId
on the resource object.
For example:
The resulting template will use
YourBucket
as the logical ID.NOTE: the
logicalId
property will now return a stringified tokeninstead of a concrete value.
Fixes #1594
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.