From 4b00ffeb86b3ebb9a0190c2842bd36ebb4043f52 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 26 Sep 2024 11:02:38 +0200 Subject: [PATCH] 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) {