Skip to content
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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

otaviomacedo
Copy link
Contributor

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:

  • Computing a distance function between resources, and then interact with the
    user to establish what is renaming and what is actual replacement.
  • Storing metadata based on this interaction to apply the change in other
    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:

MyStack
└ MyBucket
  └ Resource 

In the template, the bucket resource will have a logical ID like
MyBucketFD3F4958. So you deploy this template for the first time, creating
the stack. Then you decide to rename the MyBucket construct to NewName
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 with
the following message:

Some resources have been renamed:
  - MyStack/MyBucket/Resource -> MyStack/NewName/Resource

Do you wish to deploy these changes (y/n)?

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:

Some resources have been renamed:
  - {MyStack/MyBucket1/Resource, MyStack/MyBucket2/Resource} -> 
  {MyStack/NewName1/Resource, MyStack/NewName2/Resource}

Do you wish to deploy these changes (y/n)?

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

@github-actions github-actions bot added the p2 label Jan 31, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team January 31, 2025 14:25
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 31, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@otaviomacedo otaviomacedo added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.91%. Comparing base (c346e82) to head (e3f0a52).
Report is 1 commits behind head on main.

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              
Flag Coverage Δ
suite.unit 80.91% <95.58%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 79.76% <95.58%> (+0.12%) ⬆️
packages/aws-cdk-lib/core 82.17% <ø> (ø)

@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: e3f0a52
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

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);
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants