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 d6c6000092f6d..51c0b47a35b0f 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; } @@ -375,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); @@ -405,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) { 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', () => {