-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove CDK generated hash suffix on resource names #215
Conversation
src/stack.ts
Outdated
* Resources with this ID are complete hidden from the logical ID calculation. | ||
*/ | ||
const HIDDEN_ID = 'Default'; | ||
const MAX_HUMAN_LEN = 240; // this is the value in CDK and seems like a good default to keep |
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.
I guess this is the max length in CDK because CloudFormation automatically takes care of resources that have shorter limits (i.e. truncates their names)?
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.
yeah and CloudFormation has a hard limit of 255 (I think).
src/stack.ts
Outdated
/** | ||
* Calculates a unique ID for a set of textual components. | ||
* | ||
* This is forked from the internal cdk implementation with the removal of the hash suffix. |
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.
Not a lawyer, but don't we have to retain the copyright information if we copy the code from the CDK repo?
It would be great if we could mark where we modified the code
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.
@flostadler I moved all this code to it's own file and added license information. Let me know what you think.
592ec54
to
7e79906
Compare
src/cdk-logical-id.ts
Outdated
throw new Error('Unable to calculate a unique id for an empty set of components'); | ||
} | ||
|
||
// Lazy require in order to break a module dependency cycle |
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.
What does this comment refer to?
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 was copied over and I don't understand it either so I'm removing it.
CDK automatically adds a hash suffix to the resource logicalId (which we then use as the Pulumi Resource name). When pulumi autonaming takes place we then append an additional random suffix so we end up with names like `testapimyapiCloudWatchRoleA84BAE8E-120b18f`. Apart from this looking bad it also takes up 16 characters for each name and increases the likelihood of running into naming limits. This PR overrides the `allocateLogicalId` method on the `cdk.Stack`. Most of the code is simply forked from the CDK implementation with the removal of the piece that generates the hash. You can view the CDK version here https://github.com/aws/aws-cdk/blob/ccab485b87a7090ddf0773508d7b8ee84ff654b0/packages/aws-cdk-lib/core/lib/stack.ts?plain=1#L1357 closes #185
7e79906
to
bc18e1a
Compare
This PR has been shipped in release v1.0.0. |
CDK automatically adds a hash suffix to the resource logicalId (which we
then use as the Pulumi Resource name). When pulumi autonaming takes
place we then append an additional random suffix so we end up with names
like
testapimyapiCloudWatchRoleA84BAE8E-120b18f
. Apart from thislooking bad it also takes up 16 characters for each name and increases
the likelihood of running into naming limits.
This PR overrides the
allocateLogicalId
method on thecdk.Stack
.Most of the code is simply forked from the CDK implementation with the
removal of the piece that generates the hash.
You can view the CDK version here
https://github.com/aws/aws-cdk/blob/ccab485b87a7090ddf0773508d7b8ee84ff654b0/packages/aws-cdk-lib/core/lib/stack.ts?plain=1#L1357
closes #185