-
Notifications
You must be signed in to change notification settings - Fork 4k
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(cli): warn the user of resource renaming #33257
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/aws-cdk/lib/cli/cdk-toolkit.ts
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 fails with the following errors:
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/33257/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33257 +/- ##
==========================================
+ Coverage 80.84% 80.91% +0.06%
==========================================
Files 232 233 +1
Lines 14135 14197 +62
Branches 2460 2469 +9
==========================================
+ Hits 11428 11487 +59
- Misses 2427 2430 +3
Partials 280 280
Flags with carried forward coverage won't be shown. Click here to find out more.
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Please move to api/refactoring.ts
dvd the test file accordingly.
@@ -359,6 +360,24 @@ export class CdkToolkit { | |||
); | |||
} | |||
|
|||
const currentTemplate = await this.props.deployments.readCurrentTemplate(stack); |
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.
This will need to be added to the new Toolkit as well (yes I now it's annoying).
TL;DR: with this feature, the CLI will detect when resources have been renamed
and ask the user if they really want to deploy the stack, considering that it will
cause resource replacement.
For now, it's just an additional barrier, to help protect against accidental
renaming. But in the future (possibly with the help of CloudFormation), we can
actually perform the refactoring on the user's behalf. Also, we are only checking
the simplest case, where the logical ID changes, but the resource is kept as it
was. Future work may include:
user to establish what is renaming and what is actual replacement.
environments without user interaction (e.g., CI/CD).
Reproducing the relevant part of the README below for convenience.
If you change the ID of a construct, this change will be propagated all the
way down to the CloudFormation level, leading to the change of at least one
resource logical ID. If you then use this template to update a stack created
with the previous version of the template, all the renamed resources will be
replaced.
Since a renaming like this might have happened by accident, the CLI will detect
such cases just before performing a deployment, and ask for confirmation to go
ahead. For example, suppose you have the following construct tree:
In the template, the bucket resource will have a logical ID like
MyBucketFD3F4958
. So you deploy this template for the first time, creatingthe stack. Then you decide to rename the
MyBucket
construct toNewName
and try to deploy it again, to update the stack. Since the logical ID has
changed (to something like
NewNameEF569C1A
), the CLI will prompt you withthe following message:
Notice that it displays the CDK metadata path for the resource. If this
field is not present, it will use the logical ID instead.
Resources are considered equal if they have the same content (up to reordering
of properties). So if you have different logical IDs referring to equal objects,
and you rename at least two of them, it's impossible to establish a 1:1
correspondence as above. So the CLI will display a correspondence between sets
of identifiers:
As usual, you can force the CLI to skip this approval with the
--require-approval nevel
option.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license