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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
+219 −24
Open

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
551b523
fix: quiet flag to print stack name when having diffs
sakurai-ryo May 13, 2024
ae2bf0b
fix: build error
sakurai-ryo May 13, 2024
f312019
Merge branch 'main' into fix-quiet-stack-name
SankyRed May 21, 2024
94fb996
Merge branch 'main' into fix-quiet-stack-name
SankyRed May 22, 2024
ce831cc
Merge branch 'main' into fix-quiet-stack-name
SankyRed May 22, 2024
da1305c
Merge branch 'main' into fix-quiet-stack-name
SankyRed May 29, 2024
7a19200
Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into fi…
sakurai-ryo May 31, 2024
dc6773d
tmp
sakurai-ryo May 31, 2024
6e198b3
tmp
sakurai-ryo May 31, 2024
9a27a9f
Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into fi…
sakurai-ryo Jun 21, 2024
18ac849
Merge branch 'fix-quiet-stack-name' of https://github.com/sakurai-ryo…
sakurai-ryo Jun 21, 2024
0f251a2
quiet msg
sakurai-ryo Jun 21, 2024
22494e8
add tests
sakurai-ryo Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions packages/aws-cdk/lib/cdk-toolkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as path from 'path';
import { format } from 'util';
import { fullDiff } from '@aws-cdk/cloudformation-diff';
import * as cxapi from '@aws-cdk/cx-api';
import * as chalk from 'chalk';
import * as chokidar from 'chokidar';
Expand Down Expand Up @@ -143,10 +144,6 @@ export class CdkToolkit {
} else {
// Compare N stacks against deployed templates
for (const stack of stacks.stackArtifacts) {
if (!quiet) {
stream.write(format('Stack %s\n', chalk.bold(stack.displayName)));
}

const templateWithNestedStacks = await this.props.deployments.readCurrentTemplateWithNestedStacks(
stack, options.compareAgainstProcessedTemplate,
);
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

? fullDiff(currentTemplate, stack.template, changeSet)
: fullDiff(currentTemplate, stack.template, changeSet, !!resourcesToImport);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be calling fullDiff across both this method and printSecurityDiff. I'd prefer we keep fullDiff in printSecurityDiff, and then move the stack print to printSecurityDiff.

if (!quiet || !diff.isEmpty) {
stream.write(format('Stack %s\n', chalk.bold(stack.displayName)));
}

if (resourcesToImport) {
stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
}

const stackCount =
options.securityOnly
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet)))
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, !!resourcesToImport, stream, nestedStacks));
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet, diff)))
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, !!resourcesToImport, stream, nestedStacks, diff));

diffs += stackCount;
}
Expand Down
8 changes: 5 additions & 3 deletions packages/aws-cdk/lib/diff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ export function printStackDiff(
changeSet?: DescribeChangeSetOutput,
isImport?: boolean,
stream: FormatStream = process.stderr,
nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number {
nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates },
diff?: TemplateDiff): number {

let diff = fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);
diff = diff ?? fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);

// detect and filter out mangled characters from the diff
let filteredChangesCount = 0;
Expand Down Expand Up @@ -117,8 +118,9 @@ export function printSecurityDiff(
newTemplate: cxapi.CloudFormationStackArtifact,
requireApproval: RequireApproval,
changeSet?: DescribeChangeSetOutput,
diff?: TemplateDiff,
): boolean {
const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);
diff = diff ?? fullDiff(oldTemplate, newTemplate.template, changeSet);

if (difRequiresApproval(diff, requireApproval)) {
// eslint-disable-next-line max-len
Expand Down
28 changes: 25 additions & 3 deletions packages/aws-cdk/test/diff.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

aws-cdk/packages/aws-cdk/test/diff.test.ts

Line 57 in 10ed194

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

stream: buffer,
fail: false,
quiet: true,
});

// THEN
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
expect(plainTextOutput).not.toContain('Stack D');
expect(plainTextOutput).not.toContain('There were no differences');
expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 0');
expect(exitCode).toBe(0);
});

test('when quiet mode is enabled, stacks with diffs should print stack name to stdout', async () => {
// GIVEN
const buffer = new StringWritable();

// WHEN
const exitCode = await toolkit.diff({
stackNames: ['A'],
stream: buffer,
fail: false,
quiet: true,
});

// THEN
expect(buffer.data.trim()).not.toContain('Stack A');
expect(buffer.data.trim()).not.toContain('There were no differences');
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
expect(plainTextOutput).toContain('Stack A');
expect(plainTextOutput).not.toContain('There were no differences');
expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 1');
expect(exitCode).toBe(0);
});
});
Expand Down
Loading