Skip to content

Commit

Permalink
fix(cdk): cdk diff --quiet to print stack name when there is diffs (a…
Browse files Browse the repository at this point in the history 
…ws#30186)

### Issue # (if applicable)

Closes aws#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:
```bash
$ 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
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@sakurai-ryo
sakurai-ryo authored Sep 25, 2024
1 parent 2813eb2 commit bcf9209
Show file tree
Hide file tree
Showing 3 changed files with 219 additions and 24 deletions.
22 changes: 9 additions & 13 deletions packages/aws-cdk/lib/cdk-toolkit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,11 @@ export class CdkToolkit {

const template = deserializeStructure(await fs.readFile(options.templatePath, { encoding: 'UTF-8' }));
diffs = options.securityOnly
? numberFromBool(printSecurityDiff(template, stacks.firstStack, RequireApproval.Broadening, undefined))
: printStackDiff(template, stacks.firstStack, strict, contextLines, quiet, undefined, false, stream);
? numberFromBool(printSecurityDiff(template, stacks.firstStack, RequireApproval.Broadening, quiet))
: printStackDiff(template, stacks.firstStack, strict, contextLines, quiet, undefined, undefined, false, stream);
} 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 All @@ -170,7 +166,9 @@ export class CdkToolkit {
});
} catch (e: any) {
debug(e.message);
stream.write('Checking if the stack exists before creating the changeset has failed, will base the diff on template differences (run again with -v to see the reason)\n');
if (!quiet) {
stream.write(`Checking if the stack ${stack.stackName} exists before creating the changeset has failed, will base the diff on template differences (run again with -v to see the reason)\n`);
}
stackExists = false;
}

Expand All @@ -190,14 +188,12 @@ export class CdkToolkit {
}
}

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, quiet, stack.displayName, changeSet)))
: (printStackDiff(
currentTemplate, stack, strict, contextLines, quiet, stack.displayName, changeSet, !!resourcesToImport, stream, nestedStacks,
));

diffs += stackCount;
}
Expand Down
23 changes: 19 additions & 4 deletions packages/aws-cdk/lib/diff.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,22 @@ export function printStackDiff(
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) {
Expand Down Expand Up @@ -74,9 +83,6 @@ export function printStackDiff(
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(
Expand All @@ -85,6 +91,7 @@ export function printStackDiff(
strict,
context,
quiet,
nestedStack.physicalName ?? nestedStackLogicalId,
undefined,
isImport,
stream,
Expand Down Expand Up @@ -112,10 +119,18 @@ export function printSecurityDiff(
oldTemplate: any,
newTemplate: cxapi.CloudFormationStackArtifact,
requireApproval: RequireApproval,
quiet?: boolean,
stackName?: string,
changeSet?: DescribeChangeSetOutput,
stream: FormatStream = process.stderr,
): boolean {
const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);

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

if (difRequiresApproval(diff, requireApproval)) {
// eslint-disable-next-line max-len
warning(`This deployment will make potentially sensitive changes according to your current security approval level (--require-approval ${requireApproval}).`);
Expand Down
198 changes: 191 additions & 7 deletions packages/aws-cdk/test/diff.test.ts
View file
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'],
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 Expand Up @@ -548,10 +570,16 @@ describe('stack exists checks', () => {
describe('nested stacks', () => {
beforeEach(() => {
cloudExecutable = new MockCloudExecutable({
stacks: [{
stackName: 'Parent',
template: {},
}],
stacks: [
{
stackName: 'Parent',
template: {},
},
{
stackName: 'UnchangedParent',
template: {},
},
],
});

cloudFormation = instanceMockFrom(Deployments);
Expand Down Expand Up @@ -718,6 +746,78 @@ describe('nested stacks', () => {
},
physicalName: undefined,
},
UnChangedChild: {
deployedTemplate: {
Resources: {
SomeResource: {
Type: 'AWS::Something',
Properties: {
Prop: 'unchanged',
},
},
},
},
generatedTemplate: {
Resources: {
SomeResource: {
Type: 'AWS::Something',
Properties: {
Prop: 'unchanged',
},
},
},
},
nestedStackTemplates: {},
physicalName: 'UnChangedChild',
},
},
});
}
if (stackArtifact.stackName === 'UnchangedParent') {
stackArtifact.template.Resources = {
UnchangedChild: {
Type: 'AWS::CloudFormation::Stack',
Properties: {
TemplateURL: 'child-url',
},
},
};
return Promise.resolve({
deployedRootTemplate: {
Resources: {
UnchangedChild: {
Type: 'AWS::CloudFormation::Stack',
Properties: {
TemplateURL: 'child-url',
},
},
},
},
nestedStacks: {
UnchangedChild: {
deployedTemplate: {
Resources: {
SomeResource: {
Type: 'AWS::Something',
Properties: {
Prop: 'unchanged',
},
},
},
},
generatedTemplate: {
Resources: {
SomeResource: {
Type: 'AWS::Something',
Properties: {
Prop: 'unchanged',
},
},
},
},
nestedStackTemplates: {},
physicalName: 'UnchangedChild',
},
},
});
}
Expand Down Expand Up @@ -784,6 +884,7 @@ Stack newGrandChild
Resources
[+] AWS::Something SomeResource
Stack UnChangedChild
✨ Number of stacks with differences: 6`);

Expand Down Expand Up @@ -847,12 +948,95 @@ Stack newGrandChild
Resources
[+] AWS::Something SomeResource
Stack UnChangedChild
✨ Number of stacks with differences: 6`);

expect(exitCode).toBe(0);
expect(changeSetSpy).not.toHaveBeenCalled();
});

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

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

// THEN
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').replace(/[ \t]+$/mg, '');
expect(plainTextOutput).not.toContain('Stack UnchangedParent');
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, nested stacks with diffs should print stack name to stdout', async () => {
// GIVEN
const buffer = new StringWritable();

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

// THEN
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').replace(/[ \t]+$/mg, '');
expect(plainTextOutput).toContain(`Stack Parent
Resources
[~] AWS::CloudFormation::Stack AdditionChild
└─ [~] TemplateURL
├─ [-] addition-child-url-new
└─ [+] addition-child-url-old
[~] AWS::CloudFormation::Stack DeletionChild
└─ [~] TemplateURL
├─ [-] deletion-child-url-new
└─ [+] deletion-child-url-old
[~] AWS::CloudFormation::Stack ChangedChild
└─ [~] TemplateURL
├─ [-] changed-child-url-new
└─ [+] changed-child-url-old
Stack AdditionChild
Resources
[~] AWS::Something SomeResource
└─ [+] Prop
└─ added-value
Stack DeletionChild
Resources
[~] AWS::Something SomeResource
└─ [-] Prop
└─ value-to-be-removed
Stack ChangedChild
Resources
[~] AWS::Something SomeResource
└─ [~] Prop
├─ [-] old-value
└─ [+] new-value
Stack newChild
Resources
[+] AWS::Something SomeResource
Stack newGrandChild
Resources
[+] AWS::Something SomeResource
✨ Number of stacks with differences: 6`);
expect(plainTextOutput).not.toContain('Stack UnChangedChild');
expect(exitCode).toBe(0);
});
});

describe('--strict', () => {
Expand Down

0 comments on commit bcf9209

Please sign in to comment.