From 3d2c68ce6fd31a3136593e04de172926c1d34570 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 26 Nov 2025 14:21:27 +0000 Subject: [PATCH] refactor: filter stacks before getting their templates (#960) The `refactor` command only operates on the stacks that are relevant for the target CDK application. However, it gets all the templates of the deployed stacks before filtering them, which is wasteful. Invert the order, so that the filtering happens before getting the templates. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license --- .../cli-integ/resources/cdk-apps/app/app.js | 4 + ...cdk-destroy-nonexistent-stack.integtest.ts | 21 ++ .../cdk-destroy-stage-only.integtest.ts | 28 ++ packages/aws-cdk/lib/cli/cdk-toolkit.ts | 57 ++++ packages/aws-cdk/test/cli/cdk-toolkit.test.ts | 270 +++++++++++++++++- 5 files changed, 371 insertions(+), 9 deletions(-) create mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-nonexistent-stack.integtest.ts create mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-stage-only.integtest.ts diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js index fe20f0b63..52e635ae6 100755 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js @@ -1184,6 +1184,10 @@ switch (stackSet) { case 'stage-with-no-stacks': break; + case 'stage-only': + new SomeStage(app, `${stackPrefix}-stage`); + break; + default: throw new Error(`Unrecognized INTEG_STACK_SET: '${stackSet}'`); } diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-nonexistent-stack.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-nonexistent-stack.integtest.ts new file mode 100644 index 000000000..429e6c5be --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-nonexistent-stack.integtest.ts @@ -0,0 +1,21 @@ +import { integTest, withDefaultFixture } from '../../../lib'; + +integTest('cdk destroy does not fail even if the stacks do not exist', withDefaultFixture(async (fixture) => { + const nonExistingStackName1 = 'non-existing-stack-1'; + const nonExistingStackName2 = 'non-existing-stack-2'; + + await expect(fixture.cdkDestroy([nonExistingStackName1, nonExistingStackName2])).resolves.not.toThrow(); +})); + +integTest('cdk destroy with no force option exits without prompt if the stacks do not exist', withDefaultFixture(async (fixture) => { + const nonExistingStackName1 = 'non-existing-stack-1'; + const nonExistingStackName2 = 'non-existing-stack-2'; + + await expect(fixture.cdkDestroy([nonExistingStackName1, nonExistingStackName2], { + force: false, + })).resolves.not.toThrow(); +})); + +integTest('cdk destroy does not fail even if the stages do not exist', withDefaultFixture(async (fixture) => { + await expect(fixture.cdkDestroy('NonExistent/*')).resolves.not.toThrow(); +})); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-stage-only.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-stage-only.integtest.ts new file mode 100644 index 000000000..95a7d1c50 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-stage-only.integtest.ts @@ -0,0 +1,28 @@ +import { DescribeStacksCommand } from '@aws-sdk/client-cloudformation'; +import { integTest, withDefaultFixture } from '../../../lib'; + +integTest('cdk destroy can destroy stacks in stage-only configuration', withDefaultFixture(async (fixture) => { + const integStackSet = 'stage-only'; + + const stageNameSuffix = 'stage'; + const specifiedStackName = `${stageNameSuffix}/*`; + + await fixture.cdkDeploy(specifiedStackName, { + modEnv: { + INTEG_STACK_SET: integStackSet, + }, + }); + + const stackName = `${fixture.fullStackName(stageNameSuffix)}-StackInStage`; + const stack = await fixture.aws.cloudFormation.send(new DescribeStacksCommand({ StackName: stackName })); + expect(stack.Stacks?.length ?? 0).toEqual(1); + + await fixture.cdkDestroy(specifiedStackName, { + modEnv: { + INTEG_STACK_SET: integStackSet, + }, + }); + + await expect(fixture.aws.cloudFormation.send(new DescribeStacksCommand({ StackName: stackName }))) + .rejects.toThrow(/does not exist/); +})); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 8f35662f3..4d2487000 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -10,6 +10,7 @@ import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; import { type EventName, EVENTS } from 'chokidar/handler.js'; import * as fs from 'fs-extra'; +import * as picomatch from 'picomatch'; import { CliIoHost } from './io-host'; import type { Configuration } from './user-configuration'; import { PROJECT_CONFIG } from './user-configuration'; @@ -917,6 +918,17 @@ export class CdkToolkit { const stacks = await this.selectStacksForDestroy(options.selector, options.exclusively); + await this.suggestStacks({ + selector: options.selector, + stacks, + exclusively: options.exclusively, + }); + + if (stacks.stackArtifacts.length === 0) { + await this.ioHost.asIoHelper().defaults.warn(`No stacks match the name(s): ${chalk.red(options.selector.patterns.join(', '))}`); + return; + } + if (!options.force) { const motivation = 'Destroying stacks is an irreversible action'; const question = `Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))}`; @@ -1319,6 +1331,51 @@ export class CdkToolkit { return stacks; } + private async suggestStacks(props: { + selector: StackSelector; + stacks: StackCollection; + exclusively: boolean; + }) { + if (props.selector.patterns.length === 0) { + return; + } + + const assembly = await this.assembly(); + const selectorWithoutPatterns: StackSelector = { + patterns: [], + }; + const stacksWithoutPatterns = await assembly.selectStacks(selectorWithoutPatterns, { + extend: props.exclusively ? ExtendedStackSelection.None : ExtendedStackSelection.Downstream, + defaultBehavior: DefaultSelection.AllStacks, + }); + + const patterns = props.selector.patterns.map(pattern => { + const notExist = !props.stacks.stackArtifacts.find(stack => + picomatch.isMatch(stack.hierarchicalId, pattern), + ); + + const closelyMatched = notExist ? stacksWithoutPatterns.stackArtifacts.map(stack => { + if (picomatch.isMatch(stack.hierarchicalId.toLowerCase(), pattern.toLowerCase())) { + return stack.hierarchicalId; + } + return; + }).filter((stack): stack is string => stack !== undefined) : []; + + return { + pattern, + notExist, + closelyMatched, + }; + }); + + for (const pattern of patterns) { + if (pattern.notExist) { + const closelyMatched = pattern.closelyMatched.length > 0 ? ` Do you mean ${chalk.blue(pattern.closelyMatched.join(', '))}?` : ''; + await this.ioHost.asIoHelper().defaults.warn(`${chalk.red(pattern.pattern)} does not exist.${closelyMatched}`); + } + } + } + /** * Validate the stacks for errors and warnings according to the CLI's current settings */ diff --git a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts index ecf816ece..b47cc8f80 100644 --- a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts @@ -72,6 +72,7 @@ import type { CloudFormationClientResolvedConfig, CreateChangeSetInput, CreateCh import { CreateChangeSetCommand, DeleteChangeSetCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, StackStatus } from '@aws-sdk/client-cloudformation'; import { GetParameterCommand } from '@aws-sdk/client-ssm'; import type { AwsStub } from 'aws-sdk-client-mock'; +import * as chalk from 'chalk'; import * as fs from 'fs-extra'; import { type Template, type SdkProvider, WorkGraphBuilder } from '../../lib/api'; import { Bootstrapper, type BootstrapSource } from '../../lib/api/bootstrap'; @@ -163,6 +164,42 @@ function defaultToolkitSetup() { }); } +async function singleStackToolkitSetup() { + const singleStackExecutable = await MockCloudExecutable.create({ + stacks: [MockStack.MOCK_STACK_B], + }); + + return new CdkToolkit({ + ioHost, + cloudExecutable: singleStackExecutable, + configuration: singleStackExecutable.configuration, + sdkProvider: singleStackExecutable.sdkProvider, + deployments: new FakeCloudFormation({ + 'Test-Stack-B': { Foo: 'Bar' }, + }), + }); +} + +// only stacks within stages (no top-level stacks) +async function stageOnlyToolkitSetup() { + const stageOnlyExecutable = await MockCloudExecutable.create({ + stacks: [], + nestedAssemblies: [{ + stacks: [MockStack.MOCK_STACK_C], + }], + }); + + return new CdkToolkit({ + ioHost, + cloudExecutable: stageOnlyExecutable, + configuration: stageOnlyExecutable.configuration, + sdkProvider: stageOnlyExecutable.sdkProvider, + deployments: new FakeCloudFormation({ + 'Test-Stack-C': { Baz: 'Zinga!' }, + }), + }); +} + const mockSdk = new MockSdk(); describe('bootstrap', () => { @@ -1738,17 +1775,232 @@ describe('deploy', () => { }); describe('destroy', () => { - test('destroy correct stack', async () => { + test('destroys correct stack', async () => { const toolkit = defaultToolkitSetup(); - expect(() => { - return toolkit.destroy({ - selector: { patterns: ['Test-Stack-A/Test-Stack-C'] }, - exclusively: true, - force: true, - fromDeploy: true, - }); - }).resolves; + await expect(toolkit.destroy({ + selector: { patterns: ['Test-Stack-A/Test-Stack-C'] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys with --all flag', async () => { + const toolkit = defaultToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { allTopLevel: true, patterns: [] }, // --all flag sets allTopLevel: true + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys stack within stage with wildcard pattern', async () => { + const toolkit = defaultToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { patterns: ['Test*/*'] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys stack in single-stack configuration', async () => { + const toolkit = await singleStackToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { patterns: [] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys stack with pattern in single-stack configuration', async () => { + const toolkit = await singleStackToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { patterns: ['Test-Stack-B'] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys stack within stage in stage-only configuration', async () => { + const toolkit = await stageOnlyToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { patterns: ['Test-Stack-A/Test-Stack-C'] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('destroys stack within stage with wildcard pattern in stage-only configuration', async () => { + const toolkit = await stageOnlyToolkitSetup(); + + await expect(toolkit.destroy({ + selector: { patterns: ['Test*/*'] }, + exclusively: true, + force: true, + fromDeploy: true, + })).resolves.not.toThrow(); + }); + + test('warns if there are only non-existent stacks', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['Test-Stack-X', 'Test-Stack-Y'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual([ + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-X')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-Y')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('Test-Stack-X, Test-Stack-Y')}`), 'warn'), + ]); + }); + + test('warns if there are only non-existent stacks even when exclusively is false', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['Test-Stack-X', 'Test-Stack-Y'] }, + exclusively: false, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual([ + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-X')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-Y')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('Test-Stack-X, Test-Stack-Y')}`), 'warn'), + ]); + }); + + test('warns if there is a non-existent stack and the other exists', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['Test-Stack-X', 'Test-Stack-B'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-X')} does not exist.`), 'warn'), + ]), + ); + expect(flatten(notifySpy.mock.calls)).not.toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('Test-Stack-B')} does not exist.`), 'warn'), + ]), + ); + expect(flatten(notifySpy.mock.calls)).not.toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringMatching(/No stacks match the name\(s\)/), 'warn'), + ]), + ); + }); + + test('warns when wildcard pattern does not match any stacks', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['Foo*/*'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('Foo*/*')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('Foo*/*')}`), 'warn'), + ]), + ); + }); + + test('warns when destroying non-existent stack in stage-only configuration', async () => { + const toolkit = await stageOnlyToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['Foo*/*'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('Foo*/*')} does not exist.`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('Foo*/*')}`), 'warn'), + ]), + ); + }); + + test('suggests valid names if there is a non-existent but closely matching stack', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['test-stack-b'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('test-stack-b')} does not exist. Do you mean ${chalk.blue('Test-Stack-B')}?`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('test-stack-b')}`), 'warn'), + ]), + ); + }); + + test('suggests stack names within stages if there is a non-existent but closely matching stack', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['test-stack-a/test-stack-c'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('test-stack-a/test-stack-c')} does not exist. Do you mean ${chalk.blue('Test-Stack-A/Test-Stack-C')}?`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('test-stack-a/test-stack-c')}`), 'warn'), + ]), + ); + }); + + test('suggests stack with wildcard pattern when only case differs', async () => { + const toolkit = defaultToolkitSetup(); + + await toolkit.destroy({ + selector: { patterns: ['test*/*'] }, + exclusively: true, + force: true, + fromDeploy: true, + }); + + expect(flatten(notifySpy.mock.calls)).toEqual( + expect.arrayContaining([ + expectIoMsg(expect.stringContaining(`${chalk.red('test*/*')} does not exist. Do you mean ${chalk.blue('Test-Stack-A/Test-Stack-C')}?`), 'warn'), + expectIoMsg(expect.stringContaining(`No stacks match the name(s): ${chalk.red('test*/*')}`), 'warn'), + ]), + ); }); test('destroy with concurrency', async () => {