From bcf9209fb1b9e9aa295f50c5681201db094b8c00 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Thu, 26 Sep 2024 07:30:53 +0900 Subject: [PATCH 1/2] 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', () => { From 4b00ffeb86b3ebb9a0190c2842bd36ebb4043f52 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 26 Sep 2024 11:02:38 +0200 Subject: [PATCH 2/2] fix(cli): deployment errors are printed 3 times (#31389) The CLI prints deployment errors 3 times. This is caused by an catching an error, printing it, and then throwing it again; to another `catch` statement that catches the error, prints it, and then throws it again. In this PR, get rid of one catch and change the error that gets rethrown in a different case. Also in this PR: fix the inconsistency of printing the progress of asset publishing. Compared to the progress of stack deployments, the stack name isn't bold and there is a single space offset. (A little work to change the printing, a LOT of work to get the integration+regression tests to pass, that all assert way too many specifics about the error messages that get printed to the screen) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../cli-integ/lib/with-cdk-app.ts | 2 +- .../v2.160.0/skip-tests.txt | 2 + .../@aws-cdk-testing/cli-integ/skip-tests.txt | 2 +- .../cli-integ-tests/cli-lib.integtest.ts | 2 +- .../tests/cli-integ-tests/cli.integtest.ts | 4 +- packages/aws-cdk/lib/api/deployments.ts | 5 +- packages/aws-cdk/lib/cdk-toolkit.ts | 56 +++++++++---------- 7 files changed, 37 insertions(+), 36 deletions(-) create mode 100644 packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v2.160.0/skip-tests.txt diff --git a/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts b/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts index 934531f45ffcb..b5778a0d1af0d 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts @@ -13,7 +13,7 @@ import { shell, ShellOptions, ShellHelper, rimraf } from './shell'; import { AwsContext, withAws } from './with-aws'; import { withTimeout } from './with-timeout'; -export const DEFAULT_TEST_TIMEOUT_S = 10 * 60; +export const DEFAULT_TEST_TIMEOUT_S = 20 * 60; export const EXTENDED_TEST_TIMEOUT_S = 30 * 60; /** diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v2.160.0/skip-tests.txt b/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v2.160.0/skip-tests.txt new file mode 100644 index 0000000000000..3f514816d0b88 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v2.160.0/skip-tests.txt @@ -0,0 +1,2 @@ +# This test asserts too much about the output of the CLI. +security related changes without a CLI are expected to fail when approval is required \ No newline at end of file diff --git a/packages/@aws-cdk-testing/cli-integ/skip-tests.txt b/packages/@aws-cdk-testing/cli-integ/skip-tests.txt index bb43b8f55b68f..e48344380dbaf 100644 --- a/packages/@aws-cdk-testing/cli-integ/skip-tests.txt +++ b/packages/@aws-cdk-testing/cli-integ/skip-tests.txt @@ -1,7 +1,7 @@ # This file is empty on purpose. Leave it here as documentation # and an example. # -# Copy this file to cli-regression-patches/vX.Y.Z/skip-tests.txt +# Copy this file to resources/cli-regression-patches/vX.Y.Z/skip-tests.txt # and edit it there if you want to exclude certain tests from running # when performing a certain version's regression tests. # diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts index c0b90cf503642..f41b97e257c77 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts @@ -75,7 +75,7 @@ integTest( 'This deployment will make potentially sensitive changes according to your current security approval level', ); expect(stdErr).toContain( - 'Deployment failed: Error: "--require-approval" is enabled and stack includes security-sensitive updates', + '"--require-approval" is enabled and stack includes security-sensitive updates', ); // Ensure stack was not deployed diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts index aac17c71ffca6..cfb4680987d2c 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts @@ -19,7 +19,6 @@ import { import { InvokeCommand } from '@aws-sdk/client-lambda'; import { CreateTopicCommand, DeleteTopicCommand } from '@aws-sdk/client-sns'; import { AssumeRoleCommand, GetCallerIdentityCommand } from '@aws-sdk/client-sts'; -import * as chalk from 'chalk'; import { integTest, cloneDirectory, @@ -2198,8 +2197,7 @@ integTest( allowErrExit: true, }); - const stackName = `${fixture.stackNamePrefix}-ecs-hotswap`; - const expectedSubstring = `❌ ${chalk.bold(stackName)} failed: ResourceNotReady: Resource is not in the state deploymentCompleted`; + const expectedSubstring = 'Resource is not in the state deploymentCompleted'; expect(deployOutput).toContain(expectedSubstring); expect(deployOutput).not.toContain('hotswapped!'); diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 482c9fe20b408..5a1422b4ea616 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -1,6 +1,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import * as cdk_assets from 'cdk-assets'; import { AssetManifest, IManifestEntry } from 'cdk-assets'; +import * as chalk from 'chalk'; import { Tag } from '../cdk-toolkit'; import { debug, warning, error } from '../logging'; import { Mode } from './aws-auth/credentials'; @@ -700,7 +701,7 @@ export class Deployments { if (existing) { return existing; } - const prefix = stackName ? `${stackName}: ` : ''; + const prefix = stackName ? `${chalk.bold(stackName)}: ` : ''; const publisher = new cdk_assets.AssetPublishing(assetManifest, { aws: new PublishingAws(this.sdkProvider, env), progressListener: new ParallelSafeAssetProgress(prefix, this.props.quiet ?? false), @@ -719,7 +720,7 @@ class ParallelSafeAssetProgress implements cdk_assets.IPublishProgressListener { public onPublishEvent(type: cdk_assets.EventType, event: cdk_assets.IPublishProgress): void { const handler = this.quiet && type !== 'fail' ? debug : EVENT_TO_LOGGER[type]; - handler(`${this.prefix} ${type}: ${event.message}`); + handler(`${this.prefix}${type}: ${event.message}`); } } diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 76e9278b156b2..51c0b47a35b0f 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -371,9 +371,14 @@ export class CdkToolkit { print('Stack ARN:'); data(result.stackArn); - } catch (e) { - error('\n ❌ %s failed: %s', chalk.bold(stack.displayName), e); - throw e; + } catch (e: any) { + // It has to be exactly this string because an integration test tests for + // "bold(stackname) failed: ResourceNotReady: " + throw new Error([ + `❌ ${chalk.bold(stack.stackName)} failed:`, + ...e.code ? [`${e.code}:`] : [], + e.message, + ].join(' ')); } finally { if (options.cloudWatchLogMonitor) { const foundLogGroupsResult = await findCloudWatchLogGroups(this.props.sdkProvider, stack); @@ -401,33 +406,28 @@ export class CdkToolkit { warning('⚠️ The --concurrency flag only supports --progress "events". Switching to "events".'); } - try { - const stacksAndTheirAssetManifests = stacks.flatMap(stack => [ - stack, - ...stack.dependencies.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact), - ]); - const workGraph = new WorkGraphBuilder(prebuildAssets).build(stacksAndTheirAssetManifests); - - // Unless we are running with '--force', skip already published assets - if (!options.force) { - await this.removePublishedAssets(workGraph, options); - } + const stacksAndTheirAssetManifests = stacks.flatMap(stack => [ + stack, + ...stack.dependencies.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact), + ]); + const workGraph = new WorkGraphBuilder(prebuildAssets).build(stacksAndTheirAssetManifests); - const graphConcurrency: Concurrency = { - 'stack': concurrency, - 'asset-build': 1, // This will be CPU-bound/memory bound, mostly matters for Docker builds - 'asset-publish': (options.assetParallelism ?? true) ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable - }; - - await workGraph.doParallel(graphConcurrency, { - deployStack, - buildAsset, - publishAsset, - }); - } catch (e) { - error('\n ❌ Deployment failed: %s', e); - throw e; + // Unless we are running with '--force', skip already published assets + if (!options.force) { + await this.removePublishedAssets(workGraph, options); } + + const graphConcurrency: Concurrency = { + 'stack': concurrency, + 'asset-build': 1, // This will be CPU-bound/memory bound, mostly matters for Docker builds + 'asset-publish': (options.assetParallelism ?? true) ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable + }; + + await workGraph.doParallel(graphConcurrency, { + deployStack, + buildAsset, + publishAsset, + }); } public async watch(options: WatchOptions) {