From bcf9209fb1b9e9aa295f50c5681201db094b8c00 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Thu, 26 Sep 2024 07:30:53 +0900 Subject: [PATCH] fix(cdk): `cdk diff --quiet` to print stack name when there is diffs (#30186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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: ```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* --- packages/aws-cdk/lib/cdk-toolkit.ts | 22 ++-- packages/aws-cdk/lib/diff.ts | 23 +++- packages/aws-cdk/test/diff.test.ts | 198 +++++++++++++++++++++++++++- 3 files changed, 219 insertions(+), 24 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index d6c6000092f6d..76e9278b156b2 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -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, ); @@ -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; } @@ -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; } diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 0e9f1c15543dc..c940efb46fca7 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -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) { @@ -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( @@ -85,6 +91,7 @@ export function printStackDiff( strict, context, quiet, + nestedStack.physicalName ?? nestedStackLogicalId, undefined, isImport, stream, @@ -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}).`); diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index ffaa157e5fc20..7267f746b1af9 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -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); }); }); @@ -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); @@ -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', + }, }, }); } @@ -784,6 +884,7 @@ Stack newGrandChild Resources [+] AWS::Something SomeResource +Stack UnChangedChild ✨ Number of stacks with differences: 6`); @@ -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', () => {