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

fix(cdk): cdk diff --quiet to print stack name when there is diffs #28576

Closed
wants to merge 9 commits into from

Conversation

sakurai-ryo
Copy link
Contributor

The --quiet flag on the cdk diff command will prevent the stack name and default message from being printed when there is no diff.
If diffs exist, the stack name and diffs are expected to be printed, but currently the stack name is not displayed and it is difficult to determine which stack the diff is for.

$ cdk diff --quiet
Resources
[~] AWS::S3::Bucket MyFirstBucket MyFirstBucketB8884501
 ├─ [~] DeletionPolicy
 │   ├─ [-] Delete
 │   └─ [+] Retain
 └─ [~] UpdateReplacePolicy
     ├─ [-] Delete
     └─ [+] Retain


✨  Number of stacks with differences: 1

This PR fixed to print the stack name when the --quiet flag is specified and diffs exist.

Closes #27128


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 bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jan 4, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team January 4, 2024 14:16
@github-actions github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jan 4, 2024
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 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 4, 2024
@@ -187,15 +187,37 @@ describe('non-nested stacks', () => {

// WHEN
const exitCode = await toolkit.diff({
stackNames: ['A', 'A'],
Copy link
Contributor Author

@sakurai-ryo sakurai-ryo Jan 4, 2024

Choose a reason for hiding this comment

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

This test was broken, so fixed this.
This test assumed that the diff does not exist, but the stackNames: ['A', 'A'] specification indicates that the diff does exist and the not.toContain assertion succeeded because of the presence or absence of ANSI escape sequences.
Therefore, I changed it to specify stackNames: ['D'], where no diff exists, and removed the ANSI escape sequence in the string used for the assertion.


if (stackArtifact.stackName === 'D') {

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch here!

@@ -23,9 +23,10 @@ export function printStackDiff(
context: number,
quiet: boolean,
changeSet?: CloudFormation.DescribeChangeSetOutput,
stream?: cfnDiff.FormatStream): number {
stream?: cfnDiff.FormatStream,
stackDiff?: cfnDiff.TemplateDiff): number {
Copy link
Contributor Author

@sakurai-ryo sakurai-ryo Jan 4, 2024

Choose a reason for hiding this comment

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

The cfnDiff.fullDiff function must be called in the CdkToolkit.diff method, since we need to get the presence or absence of diffs when determining whether to print the stack name.
I changed it so that the diff can be passed in optionally so that the relatively heavy processing of cfnDiff.fullDiff does not need to be executed multiple times.

@sakurai-ryo sakurai-ryo changed the title feat(cdk): cdk diff --quiet to print stack name when there is no diff fix(cdk): cdk diff --quiet to print stack name when there is no diff Jan 4, 2024
@sakurai-ryo sakurai-ryo changed the title fix(cdk): cdk diff --quiet to print stack name when there is no diff fix(cdk): cdk diff --quiet to print stack name when there is diffs Jan 4, 2024
@sakurai-ryo
Copy link
Contributor Author

Exemption Request: I think the unit tests cover the test for this change.

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jan 4, 2024
@nomike
Copy link

nomike commented Jan 24, 2024

Exemption Request: This pull request will not cause any change in the produced code, so it should be exempt from having to contain a change to the integration test file and the resulting snapshot.

Also. as it's a CLI code change, I request someone to review it please. It's a very minor change, so it will hopefully be quick to review.

@paulhcsun paulhcsun added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Jan 25, 2024
@aws-cdk-automation aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 25, 2024
@paulhcsun paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 25, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 25, 2024 21:39

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@nomike
Copy link

nomike commented Feb 8, 2024

Can someone please approve the test-pipeline?

Thanks!

@nomike
Copy link

nomike commented Feb 12, 2024

Looks like this branch needs re-basing again and then needs an approval for that test pipeline.
Could someone please kick this off?

@nomike
Copy link

nomike commented Feb 21, 2024

Is there any way to push this forward?
One of my changes is blocked by this.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

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

@paulhcsun
Copy link
Contributor

paulhcsun commented Mar 21, 2024

Hi @sakurai-ryo, @nomike. I'm very sorry for not getting around to this. It looks like I added the cli-integ-tested label already so it definitely passed the cli integ tests at some point. There was some miscommunication on our side in handing over this PR to another team but I will reach out for someone to have one final look to get this over the line.

In the meantime there are a couple of merge conflicts to be addressed, I'd like to run the cli integ tests again after those have been resolved to be safe. I will keep a close eye on this PR until then.

Thank you both for you patience.

Comment on lines -188 to +191
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet)) > 0 ? 1 : 0)
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, stream) > 0 ? 1 : 0);
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet, diff)) > 0 ? 1 : 0)
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, stream, diff) > 0 ? 1 : 0);
Copy link
Contributor

@comcalvi comcalvi Mar 21, 2024

Choose a reason for hiding this comment

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

Let's not pass the diff in the same function that takes the inputs to create the diff (the old template and new template). Instead let's move the

          stream.write(format('Stack %s\n', chalk.bold(stack.displayName)));

to printStackDiff (you can access the stackName from the stack object passed as the second argument to printStackDiff).

@@ -187,15 +187,37 @@ describe('non-nested stacks', () => {

// WHEN
const exitCode = await toolkit.diff({
stackNames: ['A', 'A'],
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch here!

@comcalvi comcalvi self-assigned this Mar 21, 2024
@comcalvi comcalvi added pr/needs-cli-test-run This PR needs CLI tests run against it. and removed pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Mar 21, 2024
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 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.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@comcalvi comcalvi removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 25, 2024
@aws-cdk-automation
Copy link
Collaborator

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/28576/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
Copy link
Collaborator

This PR has been in the MERGE CONFLICTS 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.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 8, 2024
@aws-cdk-automation
Copy link
Collaborator

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

@TheCaffeintedSloth
Copy link

This is still broken and should be fixed. The --quiet flag is broken without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort p1 pr/needs-cli-test-run This PR needs CLI tests run against it. 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.

cdk-cli: --quiet flag suppresses "Stack stack-name" messages also for stacks which do have changes
7 participants