fix(cli): `diff` allow specifying `changeSet` via `cdk.json` #29375

andreialecu · 2024-03-06T18:03:45Z

Issue # (if applicable)

Reason for this change

Allows specifying whether to use changesets via cdk.json:

"diff": {
  "changeSet": false
}

It also moves this informational message to the debug() log level:

Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)

There was a --quiet CLI argument that was not squelching it, which seemed like an oversight in the initial PR.

With many stacks, the message can be quite verbose.--verbose will reveal it, but otherwise it will be hidden

Description of changes

The default value of the change-set flag is removed and instead is handled in the diff call.

The value for changeSet in the diff call is processed in this way: setting the flag gives the value > if the flag is un-set, check the context in cdk.json > if both are un-set, default to true.

This poses a question about what flags we allow to be set in cdk.json. Here, "diff" is new in cdk.json, and "changeSet" is the first and only flag able to be set. The team can have a discussion and come to a conclusion.

Description of how you validated changes

Added tests.

Manual 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

Closes aws#29232

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.

andreialecu · 2024-03-07T07:16:28Z

Exemption Request: There are integration tests here but there should be no need to touch any snapshots for this change.

TheRealAmazonKendra · 2024-03-07T20:40:14Z

@Mergifyio update

mergify · 2024-03-07T20:40:32Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2024-03-07T20:43:21Z

Thank you for picking up this change. I'm running this through our test pipeline before doing a full review. I've also added the integ test exemption because, you are correct, this does not require a change to snapshots.

scanlonp · 2024-03-08T01:13:04Z

@Mergifyio update

mergify · 2024-03-08T01:13:18Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2024-03-08T03:14:14Z

@Mergifyio update

mergify · 2024-03-08T03:14:18Z

update

✅ Branch has been successfully updated

andreialecu · 2024-03-08T07:34:50Z

Thanks for taking a look at this @TheRealAmazonKendra

I have another PR open at #29372, which is a bit more important to me; if there was any chance someone could take a look.

blimmer · 2024-03-21T15:47:13Z

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

@@ -341,7 +341,7 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
    const exists = (await CloudFormationStack.lookup(cfn, options.stack.stackName, false)).exists;

    const executionRoleArn = preparedSdk.cloudFormationRoleArn;
-    options.stream.write('Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)\n');
+    debug('Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)');


💯 - thank you! This message appears too often now.

andreialecu · 2024-03-21T18:15:21Z

packages/aws-cdk/lib/cli.ts

@@ -514,7 +514,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
          stream: args.ci ? process.stdout : undefined,
          compareAgainstProcessedTemplate: args.processed,
          quiet: args.quiet,
-          changeSet: args['change-set'],
+          changeSet: configuration.settings.get(['diff', 'changeSet']) ?? args['change-set'],


Perhaps this needs to be swapped so that the cdk.json setting can be overridden via the cli argument?

andreialecu · 2024-03-21T18:16:47Z

packages/aws-cdk/lib/cli.ts

@@ -514,7 +514,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
          stream: args.ci ? process.stdout : undefined,
          compareAgainstProcessedTemplate: args.processed,
          quiet: args.quiet,
-          changeSet: args['change-set'],
+          changeSet: configuration.settings.get(['diff', 'changeSet']) ?? args['change-set'],


Suggested change

changeSet: configuration.settings.get(['diff', 'changeSet']) ?? args['change-set'],

changeSet: args['change-set'] ?? configuration.settings.get(['diff', 'changeSet'],

I think you are right about this order of operations. Only question is if there is a difference between no flag being provided and the default.

If the flag is always given a value regardless of being set, we may have to put the context first (but I will ask some others on the team about this).

scanlonp · 2024-03-22T06:05:55Z

Hey @andreialecu, I have eyes on this. Will try to give a full review tomorrow, but at a glance this looks helpful and straightforward!

aws-cdk-automation · 2024-03-28T00:09:01Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

scanlonp · 2024-03-28T01:51:17Z

I pushed some changes so the flag and context are both respected. Will put a full explanation in the PR description.

TheRealAmazonKendra · 2024-03-28T03:30:58Z

Shushing up the PR bot that says changes have been requested for too long. That's on us. The bot will still want us to run the PR through our test pipeline, but now it won't close it.

aws-cdk-automation · 2024-04-23T16:11:43Z

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/29375/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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

aws-cdk-automation · 2024-04-23T16:39:11Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2024-04-24T16:14:28Z

➡️ 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.

comcalvi · 2024-04-24T22:19:43Z

One of the CLI integ tests is failing for this PR:

  ✕ cdk diff should only show verbose messages in --verbose mode
Expected substring: "Hold on while we create a read-only change set"
Received string:    "[17:46:04] CDK toolkit version: 2.139.0-rc.0 (build 9cd365e)

Could you fix this one?

andreialecu added 2 commits March 6, 2024 19:20

fix(cli): diff allow specifying changeSet via cdk.json

8bcba5c

Closes aws#29232

fixup! fix(cli): diff allow specifying changeSet via cdk.json

ad0d49c

aws-cdk-automation requested a review from a team March 6, 2024 18:03

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Mar 6, 2024

aws-cdk-automation requested changes Mar 6, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Mar 6, 2024

andreialecu mentioned this pull request Mar 6, 2024

cli: diff --change-set flag not configurable via cdk.json #29232

Open

docs: document cdk.json diff.changeSet option

f0553df

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 7, 2024

TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Mar 7, 2024

Merge branch 'main' into fix-changesetverbosity

3d782dd

mergify bot temporarily deployed to test-pipeline March 7, 2024 20:41 Inactive

scanlonp self-assigned this Mar 8, 2024

Merge branch 'main' into fix-changesetverbosity

a8fbfc4

mergify bot had a problem deploying to test-pipeline March 8, 2024 01:14 Failure

Merge branch 'main' into fix-changesetverbosity

f056ce2

mergify bot temporarily deployed to test-pipeline March 8, 2024 03:15 Inactive

blimmer reviewed Mar 21, 2024

View reviewed changes

andreialecu commented Mar 21, 2024

View reviewed changes

Merge branch 'main' into fix-changesetverbosity

df07d8e

scanlonp had a problem deploying to test-pipeline March 28, 2024 01:06 — with GitHub Actions Failure

flag and context respected

da815c5

scanlonp had a problem deploying to test-pipeline March 28, 2024 01:49 — with GitHub Actions Failure

TheRealAmazonKendra added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Mar 28, 2024

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

Monthly PR metrics report #29668

Closed

Merge branch 'main' into fix-changesetverbosity

9cd365e

comcalvi temporarily deployed to test-pipeline April 23, 2024 16:10 — with GitHub Actions Inactive

comcalvi temporarily deployed to test-pipeline April 23, 2024 22:26 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): `diff` allow specifying `changeSet` via `cdk.json` #29375

fix(cli): `diff` allow specifying `changeSet` via `cdk.json` #29375

andreialecu commented Mar 6, 2024 •

edited by scanlonp

Loading

aws-cdk-automation left a comment

andreialecu commented Mar 7, 2024

TheRealAmazonKendra commented Mar 7, 2024

mergify bot commented Mar 7, 2024

TheRealAmazonKendra commented Mar 7, 2024

scanlonp commented Mar 8, 2024

mergify bot commented Mar 8, 2024

TheRealAmazonKendra commented Mar 8, 2024

mergify bot commented Mar 8, 2024

andreialecu commented Mar 8, 2024

blimmer Mar 21, 2024

andreialecu Mar 21, 2024

andreialecu Mar 21, 2024

scanlonp Mar 22, 2024

scanlonp commented Mar 22, 2024

aws-cdk-automation commented Mar 28, 2024

scanlonp commented Mar 28, 2024

TheRealAmazonKendra commented Mar 28, 2024

aws-cdk-automation commented Apr 23, 2024

aws-cdk-automation commented Apr 23, 2024

aws-cdk-automation commented Apr 24, 2024

comcalvi commented Apr 24, 2024

	changeSet: configuration.settings.get(['diff', 'changeSet']) ?? args['change-set'],
	changeSet: args['change-set'] ?? configuration.settings.get(['diff', 'changeSet'],

fix(cli): diff allow specifying changeSet via cdk.json #29375

Are you sure you want to change the base?

fix(cli): diff allow specifying changeSet via cdk.json #29375

Conversation

andreialecu commented Mar 6, 2024 • edited by scanlonp 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

andreialecu commented Mar 7, 2024

TheRealAmazonKendra commented Mar 7, 2024

mergify bot commented Mar 7, 2024

✅ Branch has been successfully updated

TheRealAmazonKendra commented Mar 7, 2024

scanlonp commented Mar 8, 2024

mergify bot commented Mar 8, 2024

✅ Branch has been successfully updated

TheRealAmazonKendra commented Mar 8, 2024

mergify bot commented Mar 8, 2024

✅ Branch has been successfully updated

andreialecu commented Mar 8, 2024

blimmer Mar 21, 2024

Choose a reason for hiding this comment

andreialecu Mar 21, 2024

Choose a reason for hiding this comment

andreialecu Mar 21, 2024

Choose a reason for hiding this comment

scanlonp Mar 22, 2024

Choose a reason for hiding this comment

scanlonp commented Mar 22, 2024

aws-cdk-automation commented Mar 28, 2024

scanlonp commented Mar 28, 2024

TheRealAmazonKendra commented Mar 28, 2024

aws-cdk-automation commented Apr 23, 2024

aws-cdk-automation commented Apr 23, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 24, 2024

comcalvi commented Apr 24, 2024

fix(cli): `diff` allow specifying `changeSet` via `cdk.json` #29375

fix(cli): `diff` allow specifying `changeSet` via `cdk.json` #29375

andreialecu commented Mar 6, 2024 •

edited by scanlonp

Loading