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 #30186

Merged
merged 16 commits into from
Sep 25, 2024

Conversation

sakurai-ryo
Copy link
Contributor

Issue # (if applicable)

Closes #27128

Reason for this change

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

for example:

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


✨  Number of stacks with differences: 1

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

Description of changes

Changed the position of the fullDiff function call.
It is possible to output the stack name in the printSecurityDiff or printStackDiff functions, but since the message has already been output before these functions are called, the stack name must be output first.
I think it would be more user-friendly to have all messages after the output of the stack name, but if this is not the case, please point this out.

Description of how you validated changes

I added a unit test to confirm to print the stack name when diff exists.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team May 13, 2024 15:29
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels May 13, 2024
@@ -393,15 +393,37 @@ describe('non-nested stacks', () => {

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

Choose a reason for hiding this comment

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

I fixed a broken test. Originally, the test assumed there was no difference, but the specification stackNames: ['A', 'A'] actually implied a difference existed. The assertion not.toContain passed due to the presence or absence of ANSI escape sequences. To correct this, I changed the specification to stackNames: ['D'], where no difference exists, and I also removed the ANSI escape sequences from the assertion string.

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

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.

@sakurai-ryo
Copy link
Contributor Author

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

@@ -191,14 +188,21 @@ export class CdkToolkit {
}
}

const diff = options.securityOnly
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous PR, it was commented that the stack name output should be done in the printSecurityDiff function, but I decided to write it this way because we think it is more user-friendly to write all messages after the stack name output.
Please let me know if you have a better way to write it or if you have another opinion!

#28576 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should print all messages after the stack name output, so can we move all of those messages after the stack name output and move the stack name to printSecurityDiff?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, can you explain which messages are unable to be moved?

Copy link
Contributor Author

@sakurai-ryo sakurai-ryo May 31, 2024

Choose a reason for hiding this comment

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

Thanks @comcalvi.
I think such an implementation is possible.
I have one question:
I forgot to mention that in the current code, the messages generated during the changeset creation are displayed before the stack name.
Is this acceptable?

To fix this issue, it is necessary to control the display of the stack name based on the presence of a diff when the --quiet flag is true.
However, determining the diff requires creating a changeset (though there are cases where it is unnecessary), and messages are output during this process.
While it might be necessary to suppress these messages when the --quiet flag is true, debug messages are also output during the changeset creation process when the -v flag is specified.
We would need to allow cases where messages output during the changeset creation process appear before the stack name.

Copy link
Contributor Author

@sakurai-ryo sakurai-ryo May 31, 2024

Choose a reason for hiding this comment

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

For example, I am thinking of changing the printStackDiff function to look like this

export function printStackDiff(
  oldTemplate: any,
  newTemplate: cxapi.CloudFormationStackArtifact,
  strict: boolean,
  context: number,
  quiet: boolean,
+ stackName?: string,
  changeSet?: DescribeChangeSetOutput,
  isImport?: boolean,
  stream: FormatStream = process.stderr,
  nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number {
  let diff = fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);

+  // must output the stack name if there are differences, even if quiet
+  if (stackName && (!quiet || !diff.isEmpty)) {
+    stream.write(format('Stack %s\n', chalk.bold(stackName)));
+  }

+  if (!quiet && isImport) {
+    stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
+  }

  // detect and filter out mangled characters from the diff
  let filteredChangesCount = 0;
  if (diff.differenceCount && !strict) {
    const mangledNewTemplate = JSON.parse(mangleLikeCloudFormation(JSON.stringify(newTemplate.template)));
    const mangledDiff = fullDiff(oldTemplate, mangledNewTemplate, changeSet);
    filteredChangesCount = Math.max(0, diff.differenceCount - mangledDiff.differenceCount);
    if (filteredChangesCount > 0) {
      diff = mangledDiff;
    }
  }

  // filter out 'AWS::CDK::Metadata' resources from the template
  if (diff.resources && !strict) {
    diff.resources = diff.resources.filter(change => {
      if (!change) { return true; }
      if (change.newResourceType === 'AWS::CDK::Metadata') { return false; }
      if (change.oldResourceType === 'AWS::CDK::Metadata') { return false; }
      return true;
    });
  }

  let stackDiffCount = 0;
  if (!diff.isEmpty) {
    stackDiffCount++;
    formatDifferences(stream, diff, {
      ...logicalIdMapFromTemplate(oldTemplate),
      ...buildLogicalToPathMap(newTemplate),
    }, context);
  } else if (!quiet) {
    print(chalk.green('There were no differences'));
  }
  if (filteredChangesCount > 0) {
    print(chalk.yellow(`Omitted ${filteredChangesCount} changes because they are likely mangled non-ASCII characters. Use --strict to print them.`));
  }

  for (const nestedStackLogicalId of Object.keys(nestedStackTemplates ?? {})) {
    if (!nestedStackTemplates) {
      break;
    }
    const nestedStack = nestedStackTemplates[nestedStackLogicalId];
-    if (!quiet) {
-      stream.write(format('Stack %s\n', chalk.bold(nestedStack.physicalName ?? nestedStackLogicalId)));
-    }


    (newTemplate as any)._template = nestedStack.generatedTemplate;
    stackDiffCount += printStackDiff(
      nestedStack.deployedTemplate,
      newTemplate,
      strict,
      context,
      quiet,
+      nestedStack.physicalName ?? nestedStackLogicalId,
      undefined,
      isImport,
      stream,
      nestedStack.nestedStackTemplates,
    );
  }

  return stackDiffCount;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see no harm in creating the changeset before printing the stack name. Since we now need the changeset to determine the diff, I think that is what we must do.

@sakurai-ryo sakurai-ryo changed the title fix(cdk): cdk diff --quiet to print stack name when there is diffs fix(cdk): cdk diff with --quiet option to print stack name when there is diffs May 13, 2024
@sakurai-ryo sakurai-ryo changed the title fix(cdk): cdk diff with --quiet option to print stack name when there is diffs fix(cdk): cdk diff --quiet to print stack name when there is diffs May 13, 2024
@SankyRed SankyRed added the pr/needs-cli-test-run This PR needs CLI tests run against it. label May 13, 2024
@SankyRed SankyRed added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels May 22, 2024
@SankyRed SankyRed added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label May 22, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review May 22, 2024 14:24

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 22, 2024
@SankyRed SankyRed added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested labels May 22, 2024
@aws-cdk-automation
Copy link
Collaborator

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.

@mergify mergify bot dismissed comcalvi’s stale review June 21, 2024 13:27

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 21, 2024
@sakurai-ryo
Copy link
Contributor Author

@comcalvi
I have modified the code and added a test for nested stacks, could you review it again?

@nomike
Copy link

nomike commented Jul 15, 2024

Is there any update on this PR?

@nomike
Copy link

nomike commented Aug 28, 2024

How can we grab the attention of someone who can review this?

Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

This is good to merge, pending the success of the CLI test run.

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

Copy link
Contributor

mergify bot commented Sep 25, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 25, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit bcf9209 into aws:main Sep 25, 2024
11 of 12 checks passed
Copy link
Contributor

mergify bot commented Sep 25, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes star-contributor [Pilot] contributed between 25-49 PRs to the CDK
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
5 participants