fix(globalaccelerator): parameter name can exceed limit of 64 characters#24796
fix(globalaccelerator): parameter name can exceed limit of 64 characters#24796mergify[bot] merged 4 commits intoaws:mainfrom
Conversation
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Overall, this looks great. Please see my comments inline.
| separator: '-', | ||
| maxLength: maxResourceNameLength, | ||
| allowedSpecialCharacters: '/_-', | ||
| }) : props.acceleratorName ?? cdk.Names.uniqueId(this); |
There was a problem hiding this comment.
We'll still have a problem here if acceleratorName is longer than 64 characters. I'm not seeing anywhere that we validate the length of it. We should probably add that to this change.
There was a problem hiding this comment.
Should I trim the length automatically(props.acceleratorName ?? cdk.Names.uniqueId(this)).slice(-64); or check the value and throw an error in case it exceeds the character limit?
| super(scope, id); | ||
|
|
||
| const maxResourceNameLength = 64; | ||
| const name = cdk.FeatureFlags.of(this).isEnabled(cxapi.GLOBAL_ACCELERATOR_SAFE_RESOURCE_NAME) ? |
There was a problem hiding this comment.
Because this isn't used on a stateful resource, I'm not actually sure we need the feature flag here. Checking in with other team members on this.
There was a problem hiding this comment.
I've verified that we don't need the feature flag here (or in the linked one).
There was a problem hiding this comment.
I've removed the FF.
Once this has been reviewed and accepted I will align the other PR as well.
Pull request has been modified.
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Looks like there are a few issues to resolve. Our restructure caused some conflicts but also the build failed due to the integ test needing to be updated.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The global accelerator should automatically generate a Name with a length smaller than 64 characters if the
acceleratorNameproperty is not specified, otherwise, it will fail during synth.If
acceleratorNameis specified, it should validate its length to be in the expected range (1-64 characters).Closes #24325.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license