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
14 changes: 8 additions & 6 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as path from 'path';
import { format } from 'util';
import * as cfnDiff 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 @@ -153,10 +154,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 templateWithNames = await this.props.deployments.readCurrentTemplateWithNestedStacks(
stack, options.compareAgainstProcessedTemplate,
);
Expand All @@ -179,14 +176,19 @@ export class CdkToolkit {
stream,
}) : undefined;

const diff = cfnDiff.fullDiff(currentTemplate, stack.template, changeSet);
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)) > 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);
Comment on lines -188 to +191
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).


diffs += stackCount + nestedStackCount;
}
Expand Down
8 changes: 5 additions & 3 deletions packages/aws-cdk/lib/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.


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

// detect and filter out mangled characters from the diff
let filteredChangesCount = 0;
Expand Down Expand Up @@ -81,8 +82,9 @@ export function printSecurityDiff(
newTemplate: cxapi.CloudFormationStackArtifact,
requireApproval: RequireApproval,
changeSet?: CloudFormation.DescribeChangeSetOutput,
stackDiff?: cfnDiff.TemplateDiff,
): boolean {
const diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet);
const diff = stackDiff ?? cfnDiff.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
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,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!

stackNames: ['D'],
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).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
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