-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix(core): overrideLogicalId validation #29708
Conversation
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 pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request: This is just a synth time check, CloudFormation is already failing if unhappy with the logical ID |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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'm hesitant to accept this but am open to conversation about it. If CloudFormation updates their requirements here, it would create more manual work on our end. Is the error message unclear enough at deploy time that this provides a major quality of life improvement for users?
The CloudFormation deploy time message is absolutely clear about this issue, this is a duplicated synth time check The main issue is the difference in how the CDK resolves regular construct generated logical IDs vs overrides. The IDs provided to the constructor are slugified, but not the overrides: new cdk.CfnOutput(this, 'Invalid construct name', {
value: 'value',
});
new cdk.CfnOutput(this, 'Output', {
value: 'value',
}).overrideLogicalId('Another invalid construct name'); $ npm run cdk synth
Outputs:
Invalidconstructname:
Value: value
Another invalid construct name:
Value: value I don't think changing this behavior to also slugify the override is warranted, to leave the escape hatch as is. Giving the user an early warning is probably best. This PR would also prevent an issue like #29700 from reoccurring. An integration test would have caught it, but a unit test would have been sufficient with this change. |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
@TheRealAmazonKendra Bump |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
@TheRealAmazonKendra Bunping again 😅 |
Issue # (if applicable)
Closes #29701
Reason for this change
Calling
overrideLogicalId
on aConstruct
with an invalid logical ID (docs) would not throw an error at synthesis time. CloudFormation wouldDescription of changes
overrideLogicalId
(must not be empty, must not be over 255 characters, must match/^[A-Za-z0-9]+$/
@error
JSDoc tagsDescription of how you validated changes
I've added unit tests, integration tests should not be necessary
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license