Skip to content

Conversation

GavinZZ
Copy link
Contributor

@GavinZZ GavinZZ commented May 2, 2025

Allow cdk diff command to create a change set that imports existing resources.

The current cdk diff command implicitly calls
CloudFormation change set creation, providing high-level details such as "add", "delete", "modify", "import",
and etc. like the following:

$ cdk diff
[-] AWS::DynamoDB::Table MyTable orphan
[+] AWS::DynamoDB::GlobalTable MyGlobalTable add

However, when the resource is meant to be imported, the cdk diff command still shows this as add. Adding cdk diff --import-existing-resources flag to show the new resource being imported instead of add.

$ cdk diff --import-existing-resources
[-] AWS::DynamoDB::Table MyTable orphan
[←] AWS::DynamoDB::GlobalTable MyGlobalTable import

Here is the underlying CFN change set JSON output

[
  {
    "type": "Resource",
    "resourceChange": {
      "action": "Import", # NOTE THAT THIS SHOWS "Import"
      "logicalResourceId": "MyTable794EDED1",
      "physicalResourceId": "DemoStack-MyTable794EDED1-11W4MR8VZ0UPE",
      "resourceType": "AWS::DynamoDB::GlobalTable",
      "replacement": "True",
      "scope": [],
      "details": [],
      "afterContext": "..."
    }
  },
  {
    "type": "Resource",
    "resourceChange": {
      "policyAction": "Retain", # Note that this is "Retain"
      "action": "Remove",
      "logicalResourceId": "MyTable794EDED1",
      "physicalResourceId": "DemoStack-MyTable794EDED1-11W4MR8VZ0UPE",
      "resourceType": "AWS::DynamoDB::Table",
      "scope": [],
      "details": [],
      "beforeContext": "..."
    }
  }
]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.98%. Comparing base (583bae0) to head (d4545ff).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
packages/aws-cdk/lib/cli/cli.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   78.95%   78.98%   +0.03%     
==========================================
  Files          46       46              
  Lines        6979     6994      +15     
  Branches      774      776       +2     
==========================================
+ Hits         5510     5524      +14     
- Misses       1451     1452       +1     
  Partials       18       18              
Flag Coverage Δ
suite.unit 78.98% <93.33%> (+0.03%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GavinZZ
Copy link
Contributor Author

GavinZZ commented May 2, 2025

Pending adding unit and integ tests

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add this feature to toolkit-lib/toolkit/toolkit.ts

@rix0rrr
Copy link
Contributor

rix0rrr commented May 5, 2025

Is there any downside to just switching this on by default?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 5, 2025

Do we also already have this feature in cdk deploy ?

@GavinZZ
Copy link
Contributor Author

GavinZZ commented May 5, 2025

toolkit-lib/toolkit/toolkit.ts

For my understanding, is this for programmatic SDK call?

@GavinZZ
Copy link
Contributor Author

GavinZZ commented May 5, 2025

Is there any downside to just switching this on by default?

I think it's not enabled by default because this is also the CFN behaviour and CDK deploy behaviour. CDK deploy already supports --import-existing-resources option which will create the change set with --import-existing-resources flag. This is not the default behaviour to use by cdk deploy and I think it makes sense to do the same in cdk diff.

@mrgrain
Copy link
Contributor

mrgrain commented May 6, 2025

Do we also already have this feature in cdk deploy ?

yes

@mrgrain
Copy link
Contributor

mrgrain commented May 6, 2025

toolkit-lib/toolkit/toolkit.ts

For my understanding, is this for programmatic SDK call?

Correct, that's for the Programmatic Toolkit. Unfortunately we have some unavoidable duplication at the moment.

@mrgrain
Copy link
Contributor

mrgrain commented May 6, 2025

Is there any downside to just switching this on by default?

The main risk is if the real resource and the CDK definition don't match. Would have to test, but I think CFN can get in a weird state then or at least it's unexpected if an existing (production) resource is changed.

@GavinZZ GavinZZ changed the title [WIP] feat(cli): allow change set imports resources that already exist feat(cli): allow change set imports resources that already exist May 8, 2025
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there.

disable flag when local template

chore: self mutation

Signed-off-by: github-actions <[email protected]>

update cdk diff output

add unit and integ tests to import feature

add an alias to the method

add README changes

address build errors

address PR feedbacks

chore: self mutation

Signed-off-by: github-actions <[email protected]>
@rix0rrr rix0rrr temporarily deployed to integ-approval May 15, 2025 09:45 — with GitHub Actions Inactive
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue May 15, 2025
Merged via the queue into main with commit 319f1ad May 15, 2025
21 checks passed
@aws-cdk-automation aws-cdk-automation deleted the yuanhaoz/cdk-diff-import-existing-resources branch May 15, 2025 11:50
@tmokmss
Copy link

tmokmss commented May 21, 2025

I just noticed this feature got merged (finally after aws/aws-cdk#32831)! Great job team! I'll write a blog about this 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants