fix(diff): properties from ChangeSet diff were ignored #30268

bergjaak · 2024-05-19T00:30:49Z

Issue # (if applicable)

Reason for this change

This is a second attempt at https://github.com/aws/aws-cdk/pull/30093/files, after that PR had to be reverted due to causing issues. (That pr made it possible for ResourceTypes to be undefined, which caused includes to be invoked on undefined.)

Description of changes

Since the changeset logic is significant enough, I moved out the existing changeset functions to its own file, to allow for better separation of concerns. Further, I wrote each function to be concerned with a single idea and tested each function directly, with full coverage.
The resourceType getter that caused includes to be called on undefined in the previous PR attempting this change -- I changed the resourceType getter to state that it can return undefined, so that callers easily know to handle undefined.
I add include-property-values to our describeChangeSet API call so that the changeset description includes the before and after values of the changed properties, which is a pretty new CloudFormation feature https://aws.amazon.com/about-aws/whats-new/2024/04/aws-cloudformation-changesets-enhanced-change-visibility-deployments/
Also, changes from the changeset will now show up in the security diff (e.g., cdk diff --security-only).

Description of how you validated changes

full unit test coverage on the individual functions of the new class, an integration test, and manually testing

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…iff2

…s from the template with what's in the changeset

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.

bergjaak · 2024-05-22T17:47:42Z

packages/aws-cdk/lib/api/util/cloudformation.ts

  // If fetchAll is true, traverse all pages from the change set description.
  while (fetchAll && response.NextToken != null) {
    const nextPage = await cfn.describeChangeSet({
      StackName: stackName,
      ChangeSetName: response.ChangeSetId ?? changeSetName,
      NextToken: response.NextToken,
+      IncludePropertyValues: true,


I need to handle this for regions where this parameter isn't supported

…n option

bergjaak · 2024-05-23T11:43:07Z

packages/aws-cdk/lib/api/util/cloudformation.ts

@@ -191,21 +191,31 @@ export class CloudFormationStack {
 *
 * @returns       CloudFormation information about the ChangeSet
 */
-async function describeChangeSet(
+export async function describeChangeSet(


this is where we get the changeset for the diff

…iff2

…bergjaak/ResourceDiff2

aws-cdk-automation · 2024-05-24T03:29:00Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/30268/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2024-05-24T03:29:24Z

➡️ 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 · 2024-05-24T03:55:07Z

AWS CodeBuild CI Report

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

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

…eset to templateDiff (#30332) ### Reason for this change I am making this change as part of #30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done. ### Description of changes * A ton of unit tests and moved changeset diff logic into a dedicated class and file. ### Description of how you validated changes * Many unit tests, integration tests, and manual tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…eset to templateDiff (aws#30332) ### Reason for this change I am making this change as part of aws#30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done. ### Description of changes * A ton of unit tests and moved changeset diff logic into a dedicated class and file. ### Description of how you validated changes * Many unit tests, integration tests, and manual tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions · 2024-08-31T07:52:12Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

bergjaak added 6 commits May 18, 2024 15:40

in progress

b6d2cda

in progress

73e97bb

Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…

cd751b3

…iff2

in progress

c15b448

I am going to consider what happens if I just always replace resource…

48c694c

…s from the template with what's in the changeset

get resource details from changeset

62338a6

aws-cdk-automation requested a review from a team May 19, 2024 00:31

github-actions bot added the p2 label May 19, 2024

get resource details from changeset

7a74592

mergify bot added the contribution/core This is a PR that came from AWS. label May 19, 2024

bergjaak had a problem deploying to test-pipeline May 19, 2024 00:32 — with GitHub Actions Failure

get resource details from changeset

0eba0cc

aws-cdk-automation requested changes May 19, 2024

View reviewed changes

bergjaak had a problem deploying to test-pipeline May 19, 2024 00:33 — with GitHub Actions Failure

update unit tests

2e259b2

bergjaak had a problem deploying to test-pipeline May 19, 2024 00:52 — with GitHub Actions Failure

separate file for changeset tests

464b699

bergjaak had a problem deploying to test-pipeline May 19, 2024 01:36 — with GitHub Actions Failure

integ test added and updated

f8593d4

bergjaak had a problem deploying to test-pipeline May 19, 2024 01:52 — with GitHub Actions Failure

bergjaak changed the title ~~Bergjaak/resource diff2~~ fix(diff): properties from ChangeSet diff were ignored May 19, 2024

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 and removed p2 labels May 19, 2024

more tests

05956d6

bergjaak had a problem deploying to test-pipeline May 19, 2024 18:56 — with GitHub Actions Failure

clean

52129b0

bergjaak had a problem deploying to test-pipeline May 19, 2024 19:00 — with GitHub Actions Failure

tests are passing

2683ed7

bergjaak had a problem deploying to test-pipeline May 22, 2024 17:43 — with GitHub Actions Failure

bergjaak commented May 22, 2024

View reviewed changes

bergjaak added 2 commits May 22, 2024 16:09

stuff

6fcfafd

handle regions where describeChangeSet.IncludePropertyValues is not a…

84ce20b

…n option

bergjaak had a problem deploying to test-pipeline May 23, 2024 11:37 — with GitHub Actions Failure

bergjaak commented May 23, 2024

View reviewed changes

clean up comments

e65abf7

bergjaak had a problem deploying to test-pipeline May 23, 2024 11:46 — with GitHub Actions Failure

bergjaak added 2 commits May 23, 2024 17:46

removed context backups

f221971

ready

213df93

bergjaak had a problem deploying to test-pipeline May 24, 2024 01:34 — with GitHub Actions Failure

ready

357ff5e

bergjaak had a problem deploying to test-pipeline May 24, 2024 01:42 — with GitHub Actions Failure

Merge branch 'main' into bergjaak/ResourceDiff2

ed49a27

bergjaak had a problem deploying to test-pipeline May 24, 2024 01:47 — with GitHub Actions Failure

bergjaak added 3 commits May 23, 2024 23:26

ready

3694e71

Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…

e1ff643

…iff2

Merge branch 'bergjaak/ResourceDiff2' of github.com:aws/aws-cdk into …

8af1b4e

…bergjaak/ResourceDiff2

bergjaak temporarily deployed to test-pipeline May 24, 2024 03:28 — with GitHub Actions Inactive

bergjaak mentioned this pull request May 24, 2024

refactor(diff): make dedicated file and class for incorporating changeset to templateDiff #30332

Merged

1 task

github-actions bot mentioned this pull request Jun 1, 2024

Monthly PR metrics report #30414

Closed

TheRealAmazonKendra closed this Aug 31, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(diff): properties from ChangeSet diff were ignored #30268

fix(diff): properties from ChangeSet diff were ignored #30268

bergjaak commented May 19, 2024 •

edited

Loading

aws-cdk-automation left a comment

bergjaak May 22, 2024

bergjaak May 23, 2024

aws-cdk-automation commented May 24, 2024

aws-cdk-automation commented May 24, 2024

aws-cdk-automation commented May 24, 2024

github-actions bot commented Aug 31, 2024

fix(diff): properties from ChangeSet diff were ignored #30268

fix(diff): properties from ChangeSet diff were ignored #30268

Conversation

bergjaak commented May 19, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

bergjaak May 22, 2024

Choose a reason for hiding this comment

bergjaak May 23, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented May 24, 2024

aws-cdk-automation commented May 24, 2024

aws-cdk-automation commented May 24, 2024

AWS CodeBuild CI Report

github-actions bot commented Aug 31, 2024

bergjaak commented May 19, 2024 •

edited

Loading