From 551b52361a91c8abd798cb1bd6669ca33131a7ba Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Tue, 14 May 2024 00:15:09 +0900 Subject: [PATCH 1/6] fix: quiet flag to print stack name when having diffs --- packages/aws-cdk/lib/cdk-toolkit.ts | 16 ++++++++++------ packages/aws-cdk/lib/diff.ts | 8 +++++--- packages/aws-cdk/test/diff.test.ts | 28 +++++++++++++++++++++++++--- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 854b7ec6419c2..7b7a160b56e44 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -1,5 +1,6 @@ import * as path from 'path'; import { format } from 'util'; +import { fullDiff, TemplateDiff } from '@aws-cdk/cloudformation-diff'; import * as cxapi from '@aws-cdk/cx-api'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; @@ -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, ); @@ -191,14 +188,21 @@ export class CdkToolkit { } } + const diff = options.securityOnly + ? fullDiff(currentTemplate, stack.template, changeSet) + : fullDiff(currentTemplate, stack.template, changeSet, !!resourcesToImport); + 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; } diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 2fc043fcd38a3..b3b0b32ab4018 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -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; @@ -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 diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index 0155f74dd192d..6a8a20e030158 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); }); }); From ae2bf0b842f8f270156efc330b6367860d8997ae Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Tue, 14 May 2024 00:50:36 +0900 Subject: [PATCH 2/6] fix: build error --- packages/aws-cdk/lib/cdk-toolkit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 7b7a160b56e44..6a2674d1c30b0 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -1,6 +1,6 @@ import * as path from 'path'; import { format } from 'util'; -import { fullDiff, TemplateDiff } from '@aws-cdk/cloudformation-diff'; +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'; From dc6773d559d53087a31657a9ac1fe4bde46dd6b0 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Sat, 1 Jun 2024 03:02:33 +0900 Subject: [PATCH 3/6] tmp --- packages/aws-cdk/lib/cdk-toolkit.ts | 16 ++++++---------- packages/aws-cdk/lib/diff.ts | 8 +++----- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 6a2674d1c30b0..854b7ec6419c2 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -1,6 +1,5 @@ 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'; @@ -144,6 +143,10 @@ 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, ); @@ -188,21 +191,14 @@ export class CdkToolkit { } } - const diff = options.securityOnly - ? fullDiff(currentTemplate, stack.template, changeSet) - : fullDiff(currentTemplate, stack.template, changeSet, !!resourcesToImport); - 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, diff))) - : (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, !!resourcesToImport, stream, nestedStacks, diff)); + ? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet))) + : (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, !!resourcesToImport, stream, nestedStacks)); diffs += stackCount; } diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index b3b0b32ab4018..2fc043fcd38a3 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -34,10 +34,9 @@ export function printStackDiff( changeSet?: DescribeChangeSetOutput, isImport?: boolean, stream: FormatStream = process.stderr, - nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }, - diff?: TemplateDiff): number { + nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number { - diff = diff ?? fullDiff(oldTemplate, newTemplate.template, changeSet, isImport); + let diff = fullDiff(oldTemplate, newTemplate.template, changeSet, isImport); // detect and filter out mangled characters from the diff let filteredChangesCount = 0; @@ -118,9 +117,8 @@ export function printSecurityDiff( newTemplate: cxapi.CloudFormationStackArtifact, requireApproval: RequireApproval, changeSet?: DescribeChangeSetOutput, - diff?: TemplateDiff, ): boolean { - diff = diff ?? fullDiff(oldTemplate, newTemplate.template, changeSet); + const diff = fullDiff(oldTemplate, newTemplate.template, changeSet); if (difRequiresApproval(diff, requireApproval)) { // eslint-disable-next-line max-len From 6e198b3cc641c37982c9512f323acc77e4f1f5bf Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Sat, 1 Jun 2024 03:41:18 +0900 Subject: [PATCH 4/6] tmp --- packages/aws-cdk/lib/cdk-toolkit.ts | 20 +++++++------------- packages/aws-cdk/lib/diff.ts | 23 +++++++++++++++++++---- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 854b7ec6419c2..8adc9e17a31d5 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, ); @@ -171,7 +167,7 @@ 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'); + 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; } @@ -191,14 +187,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 2fc043fcd38a3..a532c678b34ab 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) { @@ -78,9 +87,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( @@ -89,6 +95,7 @@ export function printStackDiff( strict, context, quiet, + nestedStack.physicalName ?? nestedStackLogicalId, undefined, isImport, stream, @@ -116,10 +123,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}).`); From 0f251a2aaff3cd1a3d15e0bb74775e3f59af6d06 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 21 Jun 2024 23:08:27 +0900 Subject: [PATCH 5/6] quiet msg --- packages/aws-cdk/lib/cdk-toolkit.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 8adc9e17a31d5..046e590ec59b8 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -167,7 +167,9 @@ export class CdkToolkit { }); } catch (e: any) { debug(e.message); - 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`); + 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; } From 22494e804d056f407d2e5628a0f434e383f3e484 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 21 Jun 2024 23:08:32 +0900 Subject: [PATCH 6/6] add tests --- packages/aws-cdk/test/diff.test.ts | 170 ++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index 6a8a20e030158..557f589aadbf4 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -570,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); @@ -740,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', + }, }, }); } @@ -806,6 +884,7 @@ Stack newGrandChild Resources [+] AWS::Something SomeResource +Stack UnChangedChild ✨ Number of stacks with differences: 6`); @@ -869,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); + }); }); class StringWritable extends Writable {