fix(core): asset names for nested stacks contain Tokens #33966
Conversation
Since Nested Stack names are Tokens (resolving to `{ Ref:
'AWS::StackName' }`), if we use `nestedStack.stackName` as the asset
name just like we do for regular stacks, we end up with Tokens in the
display name of the nested stack template asset.
This is pointless because the token will not be resolved and look like
`${Token[TOKEN.639]}`; plus, it breaks tests because the Token numbers
are different on every run. In CDK Pipelines, it would also lead to the
pipeline restarting on every pipeline run.
Instead, test for and reject Tokens, and use the nested stack's
construct path instead.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33966 +/- ##
==========================================
+ Coverage 82.38% 82.39% +0.01%
==========================================
Files 120 120
Lines 6955 6960 +5
Branches 1173 1175 +2
==========================================
+ Hits 5730 5735 +5
Misses 1120 1120
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
rix0rrr
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
kaizencc
left a comment
kaizencc
left a comment
There was a problem hiding this comment.
In general this is approved. But--
silly as it is, isn't this a breaking change? previously we were allowed to name an asset as a token. now we will run into a synth error. will people come knocking on our door saying we broke them?
approved and do-not-merge for @rix0rrr to make a final decision
|
This PR actually causes the main issues, and since it has not yet been released, this change should be safe to merge in. |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
4 participants
Since Nested Stack names are Tokens (resolving to
{ Ref: 'AWS::StackName' }), if we usethis.stackNameas the asset name just like we do for regular stacks, we end up with Tokens in the display name of the nested stack template asset.This is pointless because the token will not be resolved and look like
${Token[TOKEN.639]}; plus, it breaks tests because the Token numbers are different on every run. In CDK Pipelines, it would also lead to the pipeline restarting on every pipeline run.Instead, test for and reject Tokens, and use the nested stack's construct path instead just like we do for other assets.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license