From bc4db2aba2062f206be3e9fcb2ee41fb69fb8359 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Thu, 24 Apr 2025 11:35:25 +0100 Subject: [PATCH 01/16] feat(refactor): support for stack filtering --- .../src/api/refactoring/index.ts | 16 +- .../toolkit-lib/lib/toolkit/toolkit.ts | 10 +- .../toolkit-lib/test/actions/refactor.test.ts | 55 +++++- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 15 +- packages/aws-cdk/lib/cli/cli-config.ts | 14 ++ .../aws-cdk/lib/cli/convert-to-user-input.ts | 11 ++ .../lib/cli/parse-command-line-arguments.ts | 7 + .../aws-cdk/lib/cli/user-configuration.ts | 1 + packages/aws-cdk/lib/cli/user-input.ts | 24 +++ .../test/api/refactoring/refactoring.test.ts | 159 ++++++++++++++++-- .../aws-cdk/test/cli/cli-arguments.test.ts | 1 + 11 files changed, 276 insertions(+), 37 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts index f61d03e9c..04e37e248 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts @@ -39,7 +39,7 @@ export class AmbiguityError extends Error { * merely the stack name. */ export class ResourceLocation { - constructor(readonly stack: CloudFormationStack, readonly logicalResourceId: string) { + constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) { } public toPath(): string { @@ -118,10 +118,20 @@ export function ambiguousMovements(movements: ResourceMovement[]) { * Converts a list of unambiguous resource movements into a list of resource mappings. * */ -export function resourceMappings(movements: ResourceMovement[]): ResourceMapping[] { +export function resourceMappings(movements: ResourceMovement[], stacks?: CloudFormationStack[]): ResourceMapping[] { + const predicate = stacks == null + ? () => true + : (m: ResourceMapping) => { + // Any movement that involves one of the selected stacks (either moving from or to) + // is considered a candidate for refactoring. + const stackNames = [m.source.stack.stackName, m.destination.stack.stackName]; + return stacks.some((stack) => stackNames.includes(stack.stackName)); + }; + return movements .filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0])) - .map(([pre, post]) => new ResourceMapping(pre[0], post[0])); + .map(([pre, post]) => new ResourceMapping(pre[0], post[0])) + .filter(predicate); } function removeUnmovedResources( diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 35c017179..f202287e2 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -967,19 +967,13 @@ export class Toolkit extends CloudAssemblySourceBuilder { throw new ToolkitError('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); } - const strategy = options.stacks?.strategy ?? StackSelectionStrategy.ALL_STACKS; - if (strategy !== StackSelectionStrategy.ALL_STACKS) { - await ioHelper.notify(IO.CDK_TOOLKIT_W8010.msg( - 'Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).', - )); - } const stacks = await assembly.selectStacksV2(ALL_STACKS); - const sdkProvider = await this.sdkProvider('refactor'); const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { - const typedMappings = resourceMappings(movements).map(m => m.toTypedMapping()); + const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); + const typedMappings = resourceMappings(movements, filteredStacks.stackArtifacts).map(m => m.toTypedMapping()); await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatTypedMappings(typedMappings), { typedMappings, })); diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts index 9341fcad8..28e67190b 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -156,7 +156,7 @@ test('fails when dry-run is false', async () => { ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); }); -test('warns when stack selector is passed', async () => { +test('filters stacks when stack selector is passed', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -166,6 +166,12 @@ test('warns when stack selector is passed', async () => { StackStatus: 'CREATE_COMPLETE', CreationTime: new Date(), }, + { + StackName: 'Stack2', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/Stack2', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, ], }); @@ -176,12 +182,29 @@ test('warns when stack selector is passed', async () => { .resolves({ TemplateBody: JSON.stringify({ Resources: { - OldLogicalID: { + OldBucketName: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + 'aws:cdk:path': 'Stack1/OldBucketName/Resource', + }, + }, + }, + }), + }) + .on(GetTemplateCommand, { + StackName: 'Stack2', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + OldQueueName: { + Type: 'AWS::SQS::Queue', + UpdateReplacePolicy: 'Delete', + DeletionPolicy: 'Delete', + Metadata: { + 'aws:cdk:path': 'Stack2/OldQueueName/Resource', }, }, }, @@ -189,7 +212,7 @@ test('warns when stack selector is passed', async () => { }); // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const cx = await builderFixture(toolkit, 'two-different-stacks'); await toolkit.refactor(cx, { dryRun: true, stacks: { @@ -198,13 +221,29 @@ test('warns when stack selector is passed', async () => { }, }); + // Resources were renamed in both stacks, but we are only including Stack1. + // So expect to see only changes for Stack1. expect(ioHost.notifySpy).toHaveBeenCalledWith( expect.objectContaining({ action: 'refactor', - level: 'warn', - code: 'CDK_TOOLKIT_W8010', - message: - 'Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).', + level: 'result', + code: 'CDK_TOOLKIT_I8900', + message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldBucketName\/Resource.*Stack1\/MyBucket\/Resource/), + data: expect.objectContaining({ + typedMappings: [ + { + sourcePath: 'Stack1/OldBucketName/Resource', + destinationPath: 'Stack1/MyBucket/Resource', + type: 'AWS::S3::Bucket', + }, + ], + }), + }), + ); + + expect(ioHost.notifySpy).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.not.stringMatching(/OldQueueName/), }), ); }); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 1a4e198dc..50656c55a 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1224,15 +1224,20 @@ export class CdkToolkit { return 1; } - if (options.selector.patterns.length > 0) { - warning('Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).'); - } - + // Initially, we select all stacks to find all resource movements. + // Otherwise, we might miss some resources that are not in the selected stacks. + // Example: resource X was moved from Stack A to Stack B. If we only select Stack A, + // we will only see a deletion of resource X, but not the creation of resource X in Stack B. const stacks = await this.selectStacksForList([]); const movements = await findResourceMovements(stacks.stackArtifacts, this.props.sdkProvider); const ambiguous = ambiguousMovements(movements); + if (ambiguous.length === 0) { - const typedMappings = resourceMappings(movements).map(m => m.toTypedMapping()); + // Now we can filter the stacks to only include the ones that are relevant for the user. + const patterns = options.selector.allTopLevel ? [] : options.selector.patterns; + const filteredStacks = await this.selectStacksForList(patterns); + const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts); + const typedMappings = selectedMappings.map(m => m.toTypedMapping()); formatTypedMappings(process.stdout, typedMappings); } else { const e = new AmbiguityError(ambiguous); diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index e8cea579c..183b511be 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -399,6 +399,20 @@ export async function makeConfig(): Promise { doctor: { description: 'Check your set-up for potential problems', }, + refactor: { + description: 'Moves resources between stacks or within the same stack', + arg: { + name: 'STACKS', + variadic: true, + }, + options: { + 'dry-run': { + type: 'boolean', + desc: 'Do not perform any changes, just show what would be done', + default: false, + }, + }, + }, }, }; } diff --git a/packages/aws-cdk/lib/cli/convert-to-user-input.ts b/packages/aws-cdk/lib/cli/convert-to-user-input.ts index 4ba7e0d6a..84ed01ee5 100644 --- a/packages/aws-cdk/lib/cli/convert-to-user-input.ts +++ b/packages/aws-cdk/lib/cli/convert-to-user-input.ts @@ -251,6 +251,13 @@ export function convertYargsToUserInput(args: any): UserInput { case 'doctor': commandOptions = {}; break; + + case 'refactor': + commandOptions = { + dryRun: args.dryRun, + STACKS: args.STACKS, + }; + break; } const userInput: UserInput = { command: args._[0], @@ -432,6 +439,9 @@ export function convertConfigToUserInput(config: any): UserInput { browser: config.docs?.browser, }; const doctorOptions = {}; + const refactorOptions = { + dryRun: config.refactor?.dryRun, + }; const userInput: UserInput = { globalOptions, list: listOptions, @@ -452,6 +462,7 @@ export function convertConfigToUserInput(config: any): UserInput { context: contextOptions, docs: docsOptions, doctor: doctorOptions, + refactor: refactorOptions, }; return userInput; diff --git a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts index e3b155b62..b7bddb9ee 100644 --- a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts @@ -840,6 +840,13 @@ export function parseCommandLineArguments(args: Array): any { }), ) .command('doctor', 'Check your set-up for potential problems') + .command('refactor [STACKS..]', 'Moves resources between stacks or within the same stack', (yargs: Argv) => + yargs.option('dry-run', { + default: false, + type: 'boolean', + desc: 'Do not perform any changes, just show what would be done', + }), + ) .version(helpers.cliVersion()) .demandCommand(1, '') .recommendCommands() diff --git a/packages/aws-cdk/lib/cli/user-configuration.ts b/packages/aws-cdk/lib/cli/user-configuration.ts index 6410af7e3..b4937360d 100644 --- a/packages/aws-cdk/lib/cli/user-configuration.ts +++ b/packages/aws-cdk/lib/cli/user-configuration.ts @@ -36,6 +36,7 @@ export enum Command { DOCS = 'docs', DOC = 'doc', DOCTOR = 'doctor', + REFACTOR = 'refactor', } const BUNDLING_COMMANDS = [ diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 3083eb626..4bfc9c3c7 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -118,6 +118,11 @@ export interface UserInput { * Check your set-up for potential problems */ readonly doctor?: {}; + + /** + * Moves resources between stacks or within the same stack + */ + readonly refactor?: RefactorOptions; } /** @@ -1334,3 +1339,22 @@ export interface DocsOptions { */ readonly browser?: string; } + +/** + * Moves resources between stacks or within the same stack + * + * @struct + */ +export interface RefactorOptions { + /** + * Do not perform any changes, just show what would be done + * + * @default - false + */ + readonly dryRun?: boolean; + + /** + * Positional argument for refactor + */ + readonly STACKS?: Array; +} diff --git a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts index 1e616cda7..67b0a070d 100644 --- a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts +++ b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts @@ -301,7 +301,7 @@ describe('computeResourceDigests', () => { test('uses physical ID if present', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -329,7 +329,7 @@ describe('computeResourceDigests', () => { test('uses physical ID if present - with dependencies', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -365,7 +365,7 @@ describe('computeResourceDigests', () => { test('different physical IDs lead to different digests', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -393,7 +393,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field - different values', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -423,7 +423,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field - same value', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -453,7 +453,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field but not all of them are set in the resource', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -481,7 +481,7 @@ describe('computeResourceDigests', () => { test('resource properties does not contain primaryIdentifier - different values', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -507,7 +507,7 @@ describe('computeResourceDigests', () => { test('resource properties does not contain primaryIdentifier - same value', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -625,7 +625,7 @@ describe('typed mappings', () => { }; const pairs = resourceMovements([stack1], [stack2]); const mappings = resourceMappings(pairs).map(toCfnMapping); - expect(mappings).toEqual([]) + expect(mappings).toEqual([]); }); test('normal updates are not mappings', () => { @@ -810,7 +810,6 @@ describe('typed mappings', () => { // even though they have the same properties. Since they have different types, // they are considered different resources. expect(mappings).toEqual([]); - }); test('ambiguous resources from multiple stacks', () => { @@ -950,7 +949,7 @@ describe('typed mappings', () => { }, ], ], - ]) + ]); }); test('combines addition, deletion, update, and rename', () => { @@ -1005,6 +1004,140 @@ describe('typed mappings', () => { }, ]); }); + + test('stack filtering', () => { + const environment = { + name: 'prod', + account: '123456789012', + region: 'us-east-1', + }; + + // Scenario: + // Foo.Bucket1 -> Bar.Bucket1 + // Zee.OldName -> Zee.NewName + + const stack1 = { + environment, + stackName: 'Foo', + template: { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: { Prop: 'XXXXXXXXX' }, + }, + }, + }, + }; + + const stack2 = { + environment, + stackName: 'Bar', + template: { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: { Prop: 'XXXXXXXXX' }, + }, + }, + }, + }; + + const stack3 = { + environment, + stackName: 'Zee', + template: { + Resources: { + OldName: { + Type: 'AWS::SQS::Queue', + Properties: { Prop: 'YYYYYYYYY' }, + }, + }, + }, + }; + + const stack4 = { + environment, + stackName: 'Zee', + template: { + Resources: { + NewName: { + Type: 'AWS::SQS::Queue', + Properties: { Prop: 'YYYYYYYYY' }, + }, + }, + }, + }; + + const movements = resourceMovements([stack1, stack3], [stack2, stack4]); + + // Testing different filters: + + // Only Foo. Should include Foo and Bar + expect(resourceMappings(movements, [stack1]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + ]); + + // Only Bar. Should include Foo and Bar + expect(resourceMappings(movements, [stack2]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + ]); + + // Only Zee. Should include Zee + expect(resourceMappings(movements, [stack3]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'NewName', + StackName: 'Zee', + }, + Source: { + LogicalResourceId: 'OldName', + StackName: 'Zee', + }, + }, + ]); + + // Foo and Zee. Should include all + expect(resourceMappings(movements, [stack1, stack3]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + { + Destination: { + LogicalResourceId: 'NewName', + StackName: 'Zee', + }, + Source: { + LogicalResourceId: 'OldName', + StackName: 'Zee', + }, + }, + ]); + }); }); describe('environment grouping', () => { @@ -1106,7 +1239,7 @@ describe('environment grouping', () => { provider.returnsDefaultAccounts(environment.account); const movements = await findResourceMovements([stack1, stack2], provider); - expect(ambiguousMovements((movements))).toEqual([]); + expect(ambiguousMovements(movements)).toEqual([]); expect(resourceMappings(movements).map(toCfnMapping)).toEqual([ { @@ -1233,7 +1366,7 @@ describe('environment grouping', () => { provider.returnsDefaultAccounts(environment1.account, environment2.account); const movements = await findResourceMovements([stack1, stack2], provider); - expect(ambiguousMovements((movements))).toEqual([]); + expect(ambiguousMovements(movements)).toEqual([]); expect(resourceMappings(movements).map(toCfnMapping)).toEqual([]); }); diff --git a/packages/aws-cdk/test/cli/cli-arguments.test.ts b/packages/aws-cdk/test/cli/cli-arguments.test.ts index 39df1c6c5..c6c83abe1 100644 --- a/packages/aws-cdk/test/cli/cli-arguments.test.ts +++ b/packages/aws-cdk/test/cli/cli-arguments.test.ts @@ -138,6 +138,7 @@ describe('config', () => { gc: expect.anything(), doctor: expect.anything(), docs: expect.anything(), + refactor: expect.anything(), }); }); }); From f0f510744dfe1241d57d445e42a9d7297c504bf1 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Thu, 24 Apr 2025 11:35:25 +0100 Subject: [PATCH 02/16] feat(refactor): support for stack filtering --- .../src/api/refactoring/index.ts | 16 +- .../toolkit-lib/lib/toolkit/toolkit.ts | 10 +- .../toolkit-lib/test/actions/refactor.test.ts | 55 +++++- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 15 +- packages/aws-cdk/lib/cli/cli-config.ts | 14 ++ .../aws-cdk/lib/cli/convert-to-user-input.ts | 11 ++ .../lib/cli/parse-command-line-arguments.ts | 7 + .../aws-cdk/lib/cli/user-configuration.ts | 1 + packages/aws-cdk/lib/cli/user-input.ts | 24 +++ .../test/api/refactoring/refactoring.test.ts | 159 ++++++++++++++++-- .../aws-cdk/test/cli/cli-arguments.test.ts | 1 + 11 files changed, 276 insertions(+), 37 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts index f61d03e9c..04e37e248 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts @@ -39,7 +39,7 @@ export class AmbiguityError extends Error { * merely the stack name. */ export class ResourceLocation { - constructor(readonly stack: CloudFormationStack, readonly logicalResourceId: string) { + constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) { } public toPath(): string { @@ -118,10 +118,20 @@ export function ambiguousMovements(movements: ResourceMovement[]) { * Converts a list of unambiguous resource movements into a list of resource mappings. * */ -export function resourceMappings(movements: ResourceMovement[]): ResourceMapping[] { +export function resourceMappings(movements: ResourceMovement[], stacks?: CloudFormationStack[]): ResourceMapping[] { + const predicate = stacks == null + ? () => true + : (m: ResourceMapping) => { + // Any movement that involves one of the selected stacks (either moving from or to) + // is considered a candidate for refactoring. + const stackNames = [m.source.stack.stackName, m.destination.stack.stackName]; + return stacks.some((stack) => stackNames.includes(stack.stackName)); + }; + return movements .filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0])) - .map(([pre, post]) => new ResourceMapping(pre[0], post[0])); + .map(([pre, post]) => new ResourceMapping(pre[0], post[0])) + .filter(predicate); } function removeUnmovedResources( diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 44e41e2b8..e7a12e834 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -979,19 +979,13 @@ export class Toolkit extends CloudAssemblySourceBuilder { throw new ToolkitError('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); } - const strategy = options.stacks?.strategy ?? StackSelectionStrategy.ALL_STACKS; - if (strategy !== StackSelectionStrategy.ALL_STACKS) { - await ioHelper.notify(IO.CDK_TOOLKIT_W8010.msg( - 'Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).', - )); - } const stacks = await assembly.selectStacksV2(ALL_STACKS); - const sdkProvider = await this.sdkProvider('refactor'); const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { - const typedMappings = resourceMappings(movements).map(m => m.toTypedMapping()); + const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); + const typedMappings = resourceMappings(movements, filteredStacks.stackArtifacts).map(m => m.toTypedMapping()); await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatTypedMappings(typedMappings), { typedMappings, })); diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts index 9341fcad8..28e67190b 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -156,7 +156,7 @@ test('fails when dry-run is false', async () => { ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); }); -test('warns when stack selector is passed', async () => { +test('filters stacks when stack selector is passed', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -166,6 +166,12 @@ test('warns when stack selector is passed', async () => { StackStatus: 'CREATE_COMPLETE', CreationTime: new Date(), }, + { + StackName: 'Stack2', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/Stack2', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, ], }); @@ -176,12 +182,29 @@ test('warns when stack selector is passed', async () => { .resolves({ TemplateBody: JSON.stringify({ Resources: { - OldLogicalID: { + OldBucketName: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + 'aws:cdk:path': 'Stack1/OldBucketName/Resource', + }, + }, + }, + }), + }) + .on(GetTemplateCommand, { + StackName: 'Stack2', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + OldQueueName: { + Type: 'AWS::SQS::Queue', + UpdateReplacePolicy: 'Delete', + DeletionPolicy: 'Delete', + Metadata: { + 'aws:cdk:path': 'Stack2/OldQueueName/Resource', }, }, }, @@ -189,7 +212,7 @@ test('warns when stack selector is passed', async () => { }); // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const cx = await builderFixture(toolkit, 'two-different-stacks'); await toolkit.refactor(cx, { dryRun: true, stacks: { @@ -198,13 +221,29 @@ test('warns when stack selector is passed', async () => { }, }); + // Resources were renamed in both stacks, but we are only including Stack1. + // So expect to see only changes for Stack1. expect(ioHost.notifySpy).toHaveBeenCalledWith( expect.objectContaining({ action: 'refactor', - level: 'warn', - code: 'CDK_TOOLKIT_W8010', - message: - 'Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).', + level: 'result', + code: 'CDK_TOOLKIT_I8900', + message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldBucketName\/Resource.*Stack1\/MyBucket\/Resource/), + data: expect.objectContaining({ + typedMappings: [ + { + sourcePath: 'Stack1/OldBucketName/Resource', + destinationPath: 'Stack1/MyBucket/Resource', + type: 'AWS::S3::Bucket', + }, + ], + }), + }), + ); + + expect(ioHost.notifySpy).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.not.stringMatching(/OldQueueName/), }), ); }); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 9090770d3..7d1be871b 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1223,15 +1223,20 @@ export class CdkToolkit { return 1; } - if (options.selector.patterns.length > 0) { - warning('Refactor does not yet support stack selection. Proceeding with the default behavior (considering all stacks).'); - } - + // Initially, we select all stacks to find all resource movements. + // Otherwise, we might miss some resources that are not in the selected stacks. + // Example: resource X was moved from Stack A to Stack B. If we only select Stack A, + // we will only see a deletion of resource X, but not the creation of resource X in Stack B. const stacks = await this.selectStacksForList([]); const movements = await findResourceMovements(stacks.stackArtifacts, this.props.sdkProvider); const ambiguous = ambiguousMovements(movements); + if (ambiguous.length === 0) { - const typedMappings = resourceMappings(movements).map(m => m.toTypedMapping()); + // Now we can filter the stacks to only include the ones that are relevant for the user. + const patterns = options.selector.allTopLevel ? [] : options.selector.patterns; + const filteredStacks = await this.selectStacksForList(patterns); + const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts); + const typedMappings = selectedMappings.map(m => m.toTypedMapping()); formatTypedMappings(process.stdout, typedMappings); } else { const e = new AmbiguityError(ambiguous); diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index e8cea579c..183b511be 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -399,6 +399,20 @@ export async function makeConfig(): Promise { doctor: { description: 'Check your set-up for potential problems', }, + refactor: { + description: 'Moves resources between stacks or within the same stack', + arg: { + name: 'STACKS', + variadic: true, + }, + options: { + 'dry-run': { + type: 'boolean', + desc: 'Do not perform any changes, just show what would be done', + default: false, + }, + }, + }, }, }; } diff --git a/packages/aws-cdk/lib/cli/convert-to-user-input.ts b/packages/aws-cdk/lib/cli/convert-to-user-input.ts index 4ba7e0d6a..84ed01ee5 100644 --- a/packages/aws-cdk/lib/cli/convert-to-user-input.ts +++ b/packages/aws-cdk/lib/cli/convert-to-user-input.ts @@ -251,6 +251,13 @@ export function convertYargsToUserInput(args: any): UserInput { case 'doctor': commandOptions = {}; break; + + case 'refactor': + commandOptions = { + dryRun: args.dryRun, + STACKS: args.STACKS, + }; + break; } const userInput: UserInput = { command: args._[0], @@ -432,6 +439,9 @@ export function convertConfigToUserInput(config: any): UserInput { browser: config.docs?.browser, }; const doctorOptions = {}; + const refactorOptions = { + dryRun: config.refactor?.dryRun, + }; const userInput: UserInput = { globalOptions, list: listOptions, @@ -452,6 +462,7 @@ export function convertConfigToUserInput(config: any): UserInput { context: contextOptions, docs: docsOptions, doctor: doctorOptions, + refactor: refactorOptions, }; return userInput; diff --git a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts index e3b155b62..b7bddb9ee 100644 --- a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts @@ -840,6 +840,13 @@ export function parseCommandLineArguments(args: Array): any { }), ) .command('doctor', 'Check your set-up for potential problems') + .command('refactor [STACKS..]', 'Moves resources between stacks or within the same stack', (yargs: Argv) => + yargs.option('dry-run', { + default: false, + type: 'boolean', + desc: 'Do not perform any changes, just show what would be done', + }), + ) .version(helpers.cliVersion()) .demandCommand(1, '') .recommendCommands() diff --git a/packages/aws-cdk/lib/cli/user-configuration.ts b/packages/aws-cdk/lib/cli/user-configuration.ts index 6410af7e3..b4937360d 100644 --- a/packages/aws-cdk/lib/cli/user-configuration.ts +++ b/packages/aws-cdk/lib/cli/user-configuration.ts @@ -36,6 +36,7 @@ export enum Command { DOCS = 'docs', DOC = 'doc', DOCTOR = 'doctor', + REFACTOR = 'refactor', } const BUNDLING_COMMANDS = [ diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 3083eb626..4bfc9c3c7 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -118,6 +118,11 @@ export interface UserInput { * Check your set-up for potential problems */ readonly doctor?: {}; + + /** + * Moves resources between stacks or within the same stack + */ + readonly refactor?: RefactorOptions; } /** @@ -1334,3 +1339,22 @@ export interface DocsOptions { */ readonly browser?: string; } + +/** + * Moves resources between stacks or within the same stack + * + * @struct + */ +export interface RefactorOptions { + /** + * Do not perform any changes, just show what would be done + * + * @default - false + */ + readonly dryRun?: boolean; + + /** + * Positional argument for refactor + */ + readonly STACKS?: Array; +} diff --git a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts index 1e616cda7..67b0a070d 100644 --- a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts +++ b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts @@ -301,7 +301,7 @@ describe('computeResourceDigests', () => { test('uses physical ID if present', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -329,7 +329,7 @@ describe('computeResourceDigests', () => { test('uses physical ID if present - with dependencies', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -365,7 +365,7 @@ describe('computeResourceDigests', () => { test('different physical IDs lead to different digests', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -393,7 +393,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field - different values', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -423,7 +423,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field - same value', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -453,7 +453,7 @@ describe('computeResourceDigests', () => { test('primaryIdentifier is a composite field but not all of them are set in the resource', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'] + primaryIdentifier: ['FooName', 'BarName'], }); const template = { @@ -481,7 +481,7 @@ describe('computeResourceDigests', () => { test('resource properties does not contain primaryIdentifier - different values', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -507,7 +507,7 @@ describe('computeResourceDigests', () => { test('resource properties does not contain primaryIdentifier - same value', () => { mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'] + primaryIdentifier: ['FooName'], }); const template = { @@ -625,7 +625,7 @@ describe('typed mappings', () => { }; const pairs = resourceMovements([stack1], [stack2]); const mappings = resourceMappings(pairs).map(toCfnMapping); - expect(mappings).toEqual([]) + expect(mappings).toEqual([]); }); test('normal updates are not mappings', () => { @@ -810,7 +810,6 @@ describe('typed mappings', () => { // even though they have the same properties. Since they have different types, // they are considered different resources. expect(mappings).toEqual([]); - }); test('ambiguous resources from multiple stacks', () => { @@ -950,7 +949,7 @@ describe('typed mappings', () => { }, ], ], - ]) + ]); }); test('combines addition, deletion, update, and rename', () => { @@ -1005,6 +1004,140 @@ describe('typed mappings', () => { }, ]); }); + + test('stack filtering', () => { + const environment = { + name: 'prod', + account: '123456789012', + region: 'us-east-1', + }; + + // Scenario: + // Foo.Bucket1 -> Bar.Bucket1 + // Zee.OldName -> Zee.NewName + + const stack1 = { + environment, + stackName: 'Foo', + template: { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: { Prop: 'XXXXXXXXX' }, + }, + }, + }, + }; + + const stack2 = { + environment, + stackName: 'Bar', + template: { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: { Prop: 'XXXXXXXXX' }, + }, + }, + }, + }; + + const stack3 = { + environment, + stackName: 'Zee', + template: { + Resources: { + OldName: { + Type: 'AWS::SQS::Queue', + Properties: { Prop: 'YYYYYYYYY' }, + }, + }, + }, + }; + + const stack4 = { + environment, + stackName: 'Zee', + template: { + Resources: { + NewName: { + Type: 'AWS::SQS::Queue', + Properties: { Prop: 'YYYYYYYYY' }, + }, + }, + }, + }; + + const movements = resourceMovements([stack1, stack3], [stack2, stack4]); + + // Testing different filters: + + // Only Foo. Should include Foo and Bar + expect(resourceMappings(movements, [stack1]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + ]); + + // Only Bar. Should include Foo and Bar + expect(resourceMappings(movements, [stack2]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + ]); + + // Only Zee. Should include Zee + expect(resourceMappings(movements, [stack3]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'NewName', + StackName: 'Zee', + }, + Source: { + LogicalResourceId: 'OldName', + StackName: 'Zee', + }, + }, + ]); + + // Foo and Zee. Should include all + expect(resourceMappings(movements, [stack1, stack3]).map(toCfnMapping)).toEqual([ + { + Destination: { + LogicalResourceId: 'Bucket1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + { + Destination: { + LogicalResourceId: 'NewName', + StackName: 'Zee', + }, + Source: { + LogicalResourceId: 'OldName', + StackName: 'Zee', + }, + }, + ]); + }); }); describe('environment grouping', () => { @@ -1106,7 +1239,7 @@ describe('environment grouping', () => { provider.returnsDefaultAccounts(environment.account); const movements = await findResourceMovements([stack1, stack2], provider); - expect(ambiguousMovements((movements))).toEqual([]); + expect(ambiguousMovements(movements)).toEqual([]); expect(resourceMappings(movements).map(toCfnMapping)).toEqual([ { @@ -1233,7 +1366,7 @@ describe('environment grouping', () => { provider.returnsDefaultAccounts(environment1.account, environment2.account); const movements = await findResourceMovements([stack1, stack2], provider); - expect(ambiguousMovements((movements))).toEqual([]); + expect(ambiguousMovements(movements)).toEqual([]); expect(resourceMappings(movements).map(toCfnMapping)).toEqual([]); }); diff --git a/packages/aws-cdk/test/cli/cli-arguments.test.ts b/packages/aws-cdk/test/cli/cli-arguments.test.ts index 39df1c6c5..c6c83abe1 100644 --- a/packages/aws-cdk/test/cli/cli-arguments.test.ts +++ b/packages/aws-cdk/test/cli/cli-arguments.test.ts @@ -138,6 +138,7 @@ describe('config', () => { gc: expect.anything(), doctor: expect.anything(), docs: expect.anything(), + refactor: expect.anything(), }); }); }); From a50d6fcce4fb773bfe94b96aa94ebcc8d990c1cc Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 25 Apr 2025 15:04:19 +0100 Subject: [PATCH 03/16] feat(refactor): ability to skip refactoring for selected resources --- .../lib/cloud-assembly/metadata-schema.ts | 5 + .../src/api/refactoring/index.ts | 34 +++--- .../src/api/refactoring/skip.ts | 50 +++++++++ .../toolkit-lib/lib/actions/refactor/index.ts | 9 ++ .../toolkit-lib/lib/toolkit/toolkit.ts | 30 +++-- .../test/_fixtures/skip-refactor/index.ts | 10 ++ .../toolkit-lib/test/actions/refactor.test.ts | 52 +++++++++ packages/aws-cdk/lib/cli/cdk-toolkit.ts | 23 +++- packages/aws-cdk/lib/cli/cli-config.ts | 5 + packages/aws-cdk/lib/cli/cli.ts | 1 + .../aws-cdk/lib/cli/convert-to-user-input.ts | 2 + .../lib/cli/parse-command-line-arguments.ts | 17 ++- packages/aws-cdk/lib/cli/user-input.ts | 7 ++ .../aws-cdk/test/api/refactoring/skip.test.ts | 106 ++++++++++++++++++ 14 files changed, 324 insertions(+), 27 deletions(-) create mode 100644 packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts create mode 100644 packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts create mode 100644 packages/aws-cdk/test/api/refactoring/skip.test.ts diff --git a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts index 2896e42eb..2b7b0a769 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts +++ b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts @@ -312,6 +312,11 @@ export enum ArtifactMetadataEntryType { * Represents tags of a stack. */ STACK_TAGS = 'aws:cdk:stack-tags', + + /** + * Whether the resource should be skipped during refactoring. + */ + SKIP_REFACTOR = 'aws:cdk:skip-refactor', } /** diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts index 04e37e248..05b85a53e 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts @@ -11,6 +11,7 @@ import { Mode } from '../plugin'; import { StringWriteStream } from '../streams'; import type { CloudFormationStack } from './cloudformation'; import { computeResourceDigests, hashObject } from './digest'; +import type { SkipList } from './skip'; /** * Represents a set of possible movements of a resource from one location @@ -118,25 +119,32 @@ export function ambiguousMovements(movements: ResourceMovement[]) { * Converts a list of unambiguous resource movements into a list of resource mappings. * */ -export function resourceMappings(movements: ResourceMovement[], stacks?: CloudFormationStack[]): ResourceMapping[] { - const predicate = stacks == null - ? () => true - : (m: ResourceMapping) => { - // Any movement that involves one of the selected stacks (either moving from or to) - // is considered a candidate for refactoring. - const stackNames = [m.source.stack.stackName, m.destination.stack.stackName]; - return stacks.some((stack) => stackNames.includes(stack.stackName)); - }; +export function resourceMappings( + movements: ResourceMovement[], + stacks?: CloudFormationStack[], + skipList?: SkipList, +): ResourceMapping[] { + const stacksPredicate = + stacks == null + ? () => true + : (m: ResourceMapping) => { + // Any movement that involves one of the selected stacks (either moving from or to) + // is considered a candidate for refactoring. + const stackNames = [m.source.stack.stackName, m.destination.stack.stackName]; + return stacks.some((stack) => stackNames.includes(stack.stackName)); + }; + + const logicalIdsPredicate = + skipList == null ? () => true : (m: ResourceMapping) => !skipList!.resourceIds.includes(m.destination.logicalResourceId); return movements .filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0])) .map(([pre, post]) => new ResourceMapping(pre[0], post[0])) - .filter(predicate); + .filter(stacksPredicate) + .filter(logicalIdsPredicate); } -function removeUnmovedResources( - m: Record, -): Record { +function removeUnmovedResources(m: Record): Record { const result: Record = {}; for (const [hash, [before, after]] of Object.entries(m)) { const common = before.filter((b) => after.some((a) => a.equalTo(b))); diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts new file mode 100644 index 000000000..7ba3b165c --- /dev/null +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts @@ -0,0 +1,50 @@ +import * as fs from 'node:fs'; +import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema'; +import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +import { ToolkitError } from '../toolkit-error'; + +export interface SkipList { + resourceIds: string[]; +} + +export class ManifestSkipList implements SkipList { + constructor(private readonly manifest: AssemblyManifest) { + } + + get resourceIds(): string[] { + return Object.values(this.manifest.artifacts ?? {}) + .filter((manifest) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK) + .flatMap((manifest) => Object.entries(manifest.metadata ?? {})) + .filter(([_, entries]) => + entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true), + ) + .map(([_, entries]) => { + return entries.find((entry) => entry.type === ArtifactMetadataEntryType.LOGICAL_ID)!.data! as string; + }); + } +} + +export class FileSkipList implements SkipList { + constructor(private readonly filePath?: string) { + } + + get resourceIds(): string[] { + if (!this.filePath) { + return []; + } + const parsedData = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')); + if (!Array.isArray(parsedData) || !parsedData.every((item) => typeof item === 'string')) { + throw new ToolkitError('The content of a skip file must be a JSON array of strings'); + } + return parsedData; + } +} + +export class UnionSkipList implements SkipList { + constructor(private readonly skipLists: SkipList[]) { + } + + get resourceIds(): string[] { + return this.skipLists.flatMap((skipList) => skipList.resourceIds); + } +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts index c948cb652..12635319d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -14,4 +14,13 @@ export interface RefactorOptions { * @default - all stacks */ stacks?: StackSelector; + + /** + * The absolute path to a file that contains a list of + * resources to skip during the refactor. The file should + * be in JSON format and contain an array of _destination_ + * logical IDs, that is, the logical IDs of the resources + * as they would be after the refactor. + */ + skipFile?: string; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index e7a12e834..5fe8c7d6d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -9,6 +9,7 @@ import { NonInteractiveIoHost } from './non-interactive-io-host'; import type { ToolkitServices } from './private'; import { assemblyFromSource } from './private'; import type { DeployResult, DestroyResult, RollbackResult } from './types'; +import { FileSkipList, ManifestSkipList, UnionSkipList } from '../../../tmp-toolkit-helpers/src/api/refactoring/skip'; import type { BootstrapEnvironments, BootstrapOptions, @@ -240,7 +241,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { const bootstrapSpan = await ioHelper.span(SPAN.BOOTSTRAP_SINGLE) .begin(`${chalk.bold(environment.name)}: bootstrapping...`, { total: bootstrapEnvironments.length, - current: currentIdx+1, + current: currentIdx + 1, environment, }); @@ -334,7 +335,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { /** * Diff Action */ - public async diff(cx: ICloudAssemblySource, options: DiffOptions): Promise<{ [name: string]: TemplateDiff}> { + public async diff(cx: ICloudAssemblySource, options: DiffOptions): Promise<{ [name: string]: TemplateDiff }> { const ioHelper = asIoHelper(this.ioHost, 'diff'); const selectStacks = options.stacks ?? ALL_STACKS; const synthSpan = await ioHelper.span(SPAN.SYNTH_ASSEMBLY).begin({ stacks: selectStacks }); @@ -620,7 +621,10 @@ export class Toolkit extends CloudAssemblySourceBuilder { // Perform a rollback await this._rollback(assembly, action, { - stacks: { patterns: [stack.hierarchicalId], strategy: StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE }, + stacks: { + patterns: [stack.hierarchicalId], + strategy: StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE, + }, orphanFailedResources: options.orphanFailedResourcesDuringRollback, }); @@ -760,7 +764,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { if (options.include === undefined && options.exclude === undefined) { throw new ToolkitError( "Cannot use the 'watch' command without specifying at least one directory to monitor. " + - 'Make sure to add a "watch" key to your cdk.json', + 'Make sure to add a "watch" key to your cdk.json', ); } @@ -985,7 +989,14 @@ export class Toolkit extends CloudAssemblySourceBuilder { const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); - const typedMappings = resourceMappings(movements, filteredStacks.stackArtifacts).map(m => m.toTypedMapping()); + + const skipList = new UnionSkipList([ + new ManifestSkipList(assembly.cloudAssembly.manifest), + new FileSkipList(options.skipFile), + ]); + + const mappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); + const typedMappings = mappings.map(m => m.toTypedMapping()); await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatTypedMappings(typedMappings), { typedMappings, })); @@ -1080,9 +1091,12 @@ export class Toolkit extends CloudAssemblySourceBuilder { private async validateStacksMetadata(stacks: StackCollection, ioHost: IoHelper) { const builder = (level: IoMessageLevel) => { switch (level) { - case 'error': return IO.CDK_ASSEMBLY_E9999; - case 'warn': return IO.CDK_ASSEMBLY_W9999; - default: return IO.CDK_ASSEMBLY_I9999; + case 'error': + return IO.CDK_ASSEMBLY_E9999; + case 'warn': + return IO.CDK_ASSEMBLY_W9999; + default: + return IO.CDK_ASSEMBLY_I9999; } }; await stacks.validateMetadata( diff --git a/packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts b/packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts new file mode 100644 index 000000000..b4a9e80a8 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts @@ -0,0 +1,10 @@ +import * as s3 from 'aws-cdk-lib/aws-s3'; +import * as core from 'aws-cdk-lib/core'; + +export default async () => { + const app = new core.App({ autoSynth: false }); + const stack = new core.Stack(app, 'Stack1'); + const bucket = new s3.Bucket(stack, 'MyBucket'); + bucket.node.defaultChild?.node.addMetadata('aws:cdk:skip-refactor', true); + return app.synth(); +}; diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts index 28e67190b..cc10b53b8 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -247,3 +247,55 @@ test('filters stacks when stack selector is passed', async () => { }), ); }); + +test('resource is marked to be skipped for refactoring in the cloud assembly', async () => { + // GIVEN + mockCloudFormationClient.on(ListStacksCommand).resolves({ + StackSummaries: [ + { + StackName: 'Stack1', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/Stack1', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, + ], + }); + + mockCloudFormationClient + .on(GetTemplateCommand, { + StackName: 'Stack1', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + // This would have caused a refactor to be detected, + // but the resource is marked to be skipped... + OldLogicalID: { + Type: 'AWS::S3::Bucket', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { + 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + }, + }, + }, + }), + }); + + // WHEN + const cx = await builderFixture(toolkit, 'skip-refactor'); + await toolkit.refactor(cx, { + dryRun: true, + }); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'refactor', + level: 'result', + code: 'CDK_TOOLKIT_I8900', + // ...so we don't see it in the output + message: expect.stringMatching(/Nothing to refactor/), + }), + ); +}); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 7d1be871b..20b2bdd7c 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -19,6 +19,11 @@ import { } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api'; import { asIoHelper } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/io/private'; import { AmbiguityError } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; +import { + FileSkipList, + ManifestSkipList, + UnionSkipList, +} from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; import { PermissionChangeType } from '../../../@aws-cdk/tmp-toolkit-helpers/src/payloads'; import type { ToolkitOptions } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; import { Toolkit } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; @@ -1235,7 +1240,14 @@ export class CdkToolkit { // Now we can filter the stacks to only include the ones that are relevant for the user. const patterns = options.selector.allTopLevel ? [] : options.selector.patterns; const filteredStacks = await this.selectStacksForList(patterns); - const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts); + + const assembly = await this.assembly(); + const skipList = new UnionSkipList([ + new ManifestSkipList(assembly.assembly.manifest), + new FileSkipList(options.skipFile), + ]); + + const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); const typedMappings = selectedMappings.map(m => m.toTypedMapping()); formatTypedMappings(process.stdout, typedMappings); } else { @@ -1936,6 +1948,15 @@ export interface RefactorOptions { * Criteria for selecting stacks to deploy */ selector: StackSelector; + + /** + * The absolute path to a file that contains a list of + * resources to skip during the refactor. The file should + * be in JSON format and contain an array of _destination_ + * logical IDs, that is, the logical IDs of the resources + * as they would be after the refactor. + */ + skipFile?: string; } function buildParameterMap( diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index 183b511be..fb59fd107 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -411,6 +411,11 @@ export async function makeConfig(): Promise { desc: 'Do not perform any changes, just show what would be done', default: false, }, + 'skip-file': { + type: 'string', + requiresArg: true, + desc: 'If specified, CDK will use the given file to skip resources during the refactor', + }, }, }, }, diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index fd15044af..183130880 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -272,6 +272,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise): any { ) .command('doctor', 'Check your set-up for potential problems') .command('refactor [STACKS..]', 'Moves resources between stacks or within the same stack', (yargs: Argv) => - yargs.option('dry-run', { - default: false, - type: 'boolean', - desc: 'Do not perform any changes, just show what would be done', - }), + yargs + .option('dry-run', { + default: false, + type: 'boolean', + desc: 'Do not perform any changes, just show what would be done', + }) + .option('skip-file', { + default: undefined, + type: 'string', + requiresArg: true, + desc: 'If specified, CDK will use the given file to skip resources during the refactor', + }), ) .version(helpers.cliVersion()) .demandCommand(1, '') diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 4bfc9c3c7..19184fdc6 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1353,6 +1353,13 @@ export interface RefactorOptions { */ readonly dryRun?: boolean; + /** + * If specified, CDK will use the given file to skip resources during the refactor + * + * @default - undefined + */ + readonly skipFile?: string; + /** * Positional argument for refactor */ diff --git a/packages/aws-cdk/test/api/refactoring/skip.test.ts b/packages/aws-cdk/test/api/refactoring/skip.test.ts new file mode 100644 index 000000000..7122b81cd --- /dev/null +++ b/packages/aws-cdk/test/api/refactoring/skip.test.ts @@ -0,0 +1,106 @@ +import * as fs from 'node:fs'; +import { ManifestSkipList, FileSkipList, UnionSkipList } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; +import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +import { ToolkitError } from '@aws-cdk/toolkit-lib'; + +describe('ManifestSkipList', () => { + test('returns resource IDs marked with SKIP_REFACTOR in the manifest', () => { + const manifest = { + artifacts: { + Stack1: { + type: ArtifactType.AWS_CLOUDFORMATION_STACK, + metadata: { + LogicalId1: [ + { type: ArtifactMetadataEntryType.SKIP_REFACTOR, data: true }, + { type: ArtifactMetadataEntryType.LOGICAL_ID, data: 'Resource1' }, + ], + }, + }, + Stack2: { + type: ArtifactType.AWS_CLOUDFORMATION_STACK, + metadata: { + LogicalId2: [ + { type: ArtifactMetadataEntryType.SKIP_REFACTOR, data: true }, + { type: ArtifactMetadataEntryType.LOGICAL_ID, data: 'Resource2' }, + ], + }, + }, + 'Stack1.assets': { + type: 'cdk:asset-manifest', + properties: { + file: 'Stack1.assets.json', + requiresBootstrapStackVersion: 6, + bootstrapStackVersionSsmParameter: '/cdk-bootstrap/hnb659fds/version' + } + }, + }, + }; + + const skipList = new ManifestSkipList(manifest as any); + expect(skipList.resourceIds).toEqual(['Resource1', 'Resource2']); + }); + + test('returns an empty array if no SKIP_REFACTOR entries exist', () => { + const manifest = { + artifacts: { + Stack1: { + type: ArtifactType.AWS_CLOUDFORMATION_STACK, + metadata: { + LogicalId1: [{ type: ArtifactMetadataEntryType.LOGICAL_ID, data: 'Resource1' }], + }, + }, + }, + }; + + const skipList = new ManifestSkipList(manifest as any); + expect(skipList.resourceIds).toEqual([]); + }); +}); + +describe('FileSkipList', () => { + test('returns resource IDs from a valid JSON file', () => { + const filePath = '/path/to/skip-list.json'; + const fileContent = JSON.stringify(['Resource1', 'Resource2']); + jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); + + const skipList = new FileSkipList(filePath); + expect(skipList.resourceIds).toEqual(['Resource1', 'Resource2']); + }); + + test('returns an empty array if no file path is provided', () => { + const skipList = new FileSkipList(); + expect(skipList.resourceIds).toEqual([]); + }); + + test('throws an error if the file content is invalid', () => { + const filePath = '/path/to/skip-list.json'; + jest.spyOn(fs, 'readFileSync').mockReturnValue('invalid-json'); + + const skipList = new FileSkipList(filePath); + expect(() => skipList.resourceIds).toThrow(SyntaxError); + }); + + test('throws an error if the content is not an array', () => { + const filePath = '/path/to/skip-list.json'; + const fileContent = JSON.stringify({ resourceIds: ['Resource1', 'Resource2'] }); + jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); + + const skipList = new FileSkipList(filePath); + expect(() => skipList.resourceIds).toThrow(ToolkitError); + }); +}); + +describe('UnionSkipList', () => { + test('combines resource IDs from multiple skip lists', () => { + const skipList1 = { resourceIds: ['Resource1', 'Resource2'] }; + const skipList2 = { resourceIds: ['Resource3'] }; + + const unionSkipList = new UnionSkipList([skipList1, skipList2]); + expect(unionSkipList.resourceIds).toEqual(['Resource1', 'Resource2', 'Resource3']); + }); + + test('returns an empty array if no skip lists are provided', () => { + const unionSkipList = new UnionSkipList([]); + expect(unionSkipList.resourceIds).toEqual([]); + }); +}); \ No newline at end of file From 60cfd8d3b232b33c1a795636cd19cf8e9d0f6dec Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Mon, 28 Apr 2025 11:48:15 +0100 Subject: [PATCH 04/16] Resource locations instead of just resource IDs --- .../src/api/refactoring/index.ts | 10 +- .../src/api/refactoring/skip.ts | 67 +++++++--- .../toolkit-lib/lib/toolkit/toolkit.ts | 4 +- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 4 +- .../aws-cdk/test/api/refactoring/skip.test.ts | 120 ++++++++++++++---- 5 files changed, 156 insertions(+), 49 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts index 05b85a53e..a4d330cfb 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts @@ -135,7 +135,15 @@ export function resourceMappings( }; const logicalIdsPredicate = - skipList == null ? () => true : (m: ResourceMapping) => !skipList!.resourceIds.includes(m.destination.logicalResourceId); + skipList == null + ? () => true + : (m: ResourceMapping) => { + return !skipList!.resourceLocations.some( + (loc) => + loc.StackName === m.destination.stack.stackName && + loc.LogicalResourceId === m.destination.logicalResourceId, + ); + }; return movements .filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0])) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts index 7ba3b165c..6e96f483e 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts @@ -1,50 +1,83 @@ import * as fs from 'node:fs'; import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +import type { ResourceLocation as CfnResourceLocation } from '@aws-sdk/client-cloudformation'; import { ToolkitError } from '../toolkit-error'; export interface SkipList { - resourceIds: string[]; + resourceLocations: CfnResourceLocation[]; } export class ManifestSkipList implements SkipList { constructor(private readonly manifest: AssemblyManifest) { } - get resourceIds(): string[] { - return Object.values(this.manifest.artifacts ?? {}) - .filter((manifest) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK) - .flatMap((manifest) => Object.entries(manifest.metadata ?? {})) - .filter(([_, entries]) => - entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true), - ) - .map(([_, entries]) => { - return entries.find((entry) => entry.type === ArtifactMetadataEntryType.LOGICAL_ID)!.data! as string; - }); + get resourceLocations(): CfnResourceLocation[] { + // First, we need to filter the artifacts to only include CloudFormation stacks + const stackManifests = Object.entries(this.manifest.artifacts ?? {}).filter( + ([_, manifest]) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK, + ); + + const result: CfnResourceLocation[] = []; + for (let [stackName, manifest] of stackManifests) { + const locations = Object.values(manifest.metadata ?? {}) + // Then pick only the resources in each stack marked with SKIP_REFACTOR + .filter((entries) => + entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true), + ) + // Finally, get the logical ID of each resource + .map((entries) => { + const logicalIdEntry = entries.find((entry) => entry.type === ArtifactMetadataEntryType.LOGICAL_ID); + const location: CfnResourceLocation = { + StackName: stackName, + LogicalResourceId: logicalIdEntry!.data! as string, + }; + return location; + }); + result.push(...locations); + } + return result; } } -export class FileSkipList implements SkipList { +export class SkipFile implements SkipList { constructor(private readonly filePath?: string) { } - get resourceIds(): string[] { + get resourceLocations(): CfnResourceLocation[] { if (!this.filePath) { return []; } const parsedData = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')); - if (!Array.isArray(parsedData) || !parsedData.every((item) => typeof item === 'string')) { + if (!isValidSkipFileContent(parsedData)) { throw new ToolkitError('The content of a skip file must be a JSON array of strings'); } - return parsedData; + + const result: CfnResourceLocation[] = []; + parsedData.forEach((item: string) => { + const parts = item.split('.'); + if (parts.length !== 2) { + throw new ToolkitError(`Invalid resource location format: ${item}. Expected format: stackName.logicalId`); + } + const [stackName, logicalId] = parts; + result.push({ + StackName: stackName, + LogicalResourceId: logicalId, + }); + }); + return result; } } +function isValidSkipFileContent(data: any): data is string[] { + return Array.isArray(data) && data.every((item: any) => typeof item === 'string'); +} + export class UnionSkipList implements SkipList { constructor(private readonly skipLists: SkipList[]) { } - get resourceIds(): string[] { - return this.skipLists.flatMap((skipList) => skipList.resourceIds); + get resourceLocations(): CfnResourceLocation[] { + return this.skipLists.flatMap((skipList) => skipList.resourceLocations); } } diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 5fe8c7d6d..19196624c 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -9,7 +9,7 @@ import { NonInteractiveIoHost } from './non-interactive-io-host'; import type { ToolkitServices } from './private'; import { assemblyFromSource } from './private'; import type { DeployResult, DestroyResult, RollbackResult } from './types'; -import { FileSkipList, ManifestSkipList, UnionSkipList } from '../../../tmp-toolkit-helpers/src/api/refactoring/skip'; +import { SkipFile, ManifestSkipList, UnionSkipList } from '../../../tmp-toolkit-helpers/src/api/refactoring/skip'; import type { BootstrapEnvironments, BootstrapOptions, @@ -992,7 +992,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { const skipList = new UnionSkipList([ new ManifestSkipList(assembly.cloudAssembly.manifest), - new FileSkipList(options.skipFile), + new SkipFile(options.skipFile), ]); const mappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 20b2bdd7c..91abab0e0 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -20,7 +20,7 @@ import { import { asIoHelper } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/io/private'; import { AmbiguityError } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; import { - FileSkipList, + SkipFile, ManifestSkipList, UnionSkipList, } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; @@ -1244,7 +1244,7 @@ export class CdkToolkit { const assembly = await this.assembly(); const skipList = new UnionSkipList([ new ManifestSkipList(assembly.assembly.manifest), - new FileSkipList(options.skipFile), + new SkipFile(options.skipFile), ]); const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); diff --git a/packages/aws-cdk/test/api/refactoring/skip.test.ts b/packages/aws-cdk/test/api/refactoring/skip.test.ts index 7122b81cd..ee9e25047 100644 --- a/packages/aws-cdk/test/api/refactoring/skip.test.ts +++ b/packages/aws-cdk/test/api/refactoring/skip.test.ts @@ -1,10 +1,13 @@ import * as fs from 'node:fs'; -import { ManifestSkipList, FileSkipList, UnionSkipList } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; +import { + ManifestSkipList, + SkipFile, + UnionSkipList, +} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; -import { ToolkitError } from '@aws-cdk/toolkit-lib'; describe('ManifestSkipList', () => { - test('returns resource IDs marked with SKIP_REFACTOR in the manifest', () => { + test('returns resource locations marked with SKIP_REFACTOR in the manifest', () => { const manifest = { artifacts: { Stack1: { @@ -30,14 +33,23 @@ describe('ManifestSkipList', () => { properties: { file: 'Stack1.assets.json', requiresBootstrapStackVersion: 6, - bootstrapStackVersionSsmParameter: '/cdk-bootstrap/hnb659fds/version' - } + bootstrapStackVersionSsmParameter: '/cdk-bootstrap/hnb659fds/version', + }, }, }, }; const skipList = new ManifestSkipList(manifest as any); - expect(skipList.resourceIds).toEqual(['Resource1', 'Resource2']); + expect(skipList.resourceLocations).toEqual([ + { + StackName: 'Stack1', + LogicalResourceId: 'Resource1', + }, + { + StackName: 'Stack2', + LogicalResourceId: 'Resource2', + }, + ]); }); test('returns an empty array if no SKIP_REFACTOR entries exist', () => { @@ -53,54 +65,108 @@ describe('ManifestSkipList', () => { }; const skipList = new ManifestSkipList(manifest as any); - expect(skipList.resourceIds).toEqual([]); + expect(skipList.resourceLocations).toEqual([]); }); }); -describe('FileSkipList', () => { - test('returns resource IDs from a valid JSON file', () => { +describe('SkipFile', () => { + test('returns resource locations from a valid JSON file', () => { const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify(['Resource1', 'Resource2']); + const fileContent = JSON.stringify(['Stack1.Resource1', 'Stack2.Resource2']); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new FileSkipList(filePath); - expect(skipList.resourceIds).toEqual(['Resource1', 'Resource2']); + const skipList = new SkipFile(filePath); + expect(skipList.resourceLocations).toEqual([ + { + StackName: 'Stack1', + LogicalResourceId: 'Resource1', + }, + { + StackName: 'Stack2', + LogicalResourceId: 'Resource2', + }, + ]); }); test('returns an empty array if no file path is provided', () => { - const skipList = new FileSkipList(); - expect(skipList.resourceIds).toEqual([]); + const skipList = new SkipFile(); + expect(skipList.resourceLocations).toEqual([]); }); - test('throws an error if the file content is invalid', () => { + test('throws an error if the content is not an array', () => { const filePath = '/path/to/skip-list.json'; - jest.spyOn(fs, 'readFileSync').mockReturnValue('invalid-json'); + const fileContent = JSON.stringify({ spuriousKey: ['Stack1.Resource1', 'Stack2.Resource2'] }); + jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new FileSkipList(filePath); - expect(() => skipList.resourceIds).toThrow(SyntaxError); + const skipList = new SkipFile(filePath); + expect(() => skipList.resourceLocations).toThrow('The content of a skip file must be a JSON array of strings'); }); - test('throws an error if the content is not an array', () => { + test('throws an error if the content is an array but not of strings', () => { + const filePath = '/path/to/skip-list.json'; + const fileContent = JSON.stringify([1, 2, 3]); + jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); + + const skipList = new SkipFile(filePath); + expect(() => skipList.resourceLocations).toThrow('The content of a skip file must be a JSON array of strings'); + }); + + test('throws an error if the some entries are not valid resource locations', () => { const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify({ resourceIds: ['Resource1', 'Resource2'] }); + const fileContent = JSON.stringify(['Stack1.Resource1', 'InvalidResourceLocation']); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new FileSkipList(filePath); - expect(() => skipList.resourceIds).toThrow(ToolkitError); + const skipList = new SkipFile(filePath); + expect(() => skipList.resourceLocations).toThrow( + 'Invalid resource location format: InvalidResourceLocation. Expected format: stackName.logicalId', + ); }); }); describe('UnionSkipList', () => { test('combines resource IDs from multiple skip lists', () => { - const skipList1 = { resourceIds: ['Resource1', 'Resource2'] }; - const skipList2 = { resourceIds: ['Resource3'] }; + const skipList1 = { + resourceIds: ['Resource1', 'Resource2'], + resourceLocations: [ + { + StackName: 'Stack1', + LogicalResourceId: 'Resource1', + }, + { + StackName: 'Stack2', + LogicalResourceId: 'Resource2', + }, + ], + }; + const skipList2 = { + resourceIds: ['Resource3'], + resourceLocations: [ + { + StackName: 'Stack3', + LogicalResourceId: 'Resource3', + }, + ], + }; const unionSkipList = new UnionSkipList([skipList1, skipList2]); - expect(unionSkipList.resourceIds).toEqual(['Resource1', 'Resource2', 'Resource3']); + expect(unionSkipList.resourceLocations).toEqual([ + { + StackName: 'Stack1', + LogicalResourceId: 'Resource1', + }, + { + StackName: 'Stack2', + LogicalResourceId: 'Resource2', + }, + { + StackName: 'Stack3', + LogicalResourceId: 'Resource3', + }, + ]); }); test('returns an empty array if no skip lists are provided', () => { const unionSkipList = new UnionSkipList([]); - expect(unionSkipList.resourceIds).toEqual([]); + expect(unionSkipList.resourceLocations).toEqual([]); }); -}); \ No newline at end of file +}); From 3dee8733316577fa7c6c01a3f3b4e498eb8fe401 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 29 Apr 2025 15:19:57 +0100 Subject: [PATCH 05/16] Support construct paths in the skip file --- .../src/api/refactoring/cloudformation.ts | 52 +++++++ .../src/api/refactoring/index.ts | 77 ++-------- .../src/api/refactoring/skip.ts | 82 +++++++--- .../toolkit-lib/lib/actions/refactor/index.ts | 15 +- .../toolkit-lib/lib/toolkit/toolkit.ts | 13 +- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 34 ++--- .../test/api/refactoring/refactoring.test.ts | 26 ++-- .../aws-cdk/test/api/refactoring/skip.test.ts | 144 ++++++++---------- 8 files changed, 239 insertions(+), 204 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation.ts index 949171efc..6828996df 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation.ts @@ -1,3 +1,4 @@ +import type { TypedMapping } from '@aws-cdk/cloudformation-diff'; import type * as cxapi from '@aws-cdk/cx-api'; export interface CloudFormationTemplate { @@ -15,3 +16,54 @@ export interface CloudFormationStack { readonly stackName: string; readonly template: CloudFormationTemplate; } + +/** + * This class mirrors the `ResourceLocation` interface from CloudFormation, + * but is richer, since it has a reference to the stack object, rather than + * merely the stack name. + */ +export class ResourceLocation { + constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) { + } + + public toPath(): string { + const stack = this.stack; + const resource = stack.template.Resources?.[this.logicalResourceId]; + const result = resource?.Metadata?.['aws:cdk:path']; + + if (result != null) { + return result; + } + + // If the path is not available, we can use stack name and logical ID + return `${stack.stackName}.${this.logicalResourceId}`; + } + + public getType(): string { + const resource = this.stack.template.Resources?.[this.logicalResourceId ?? '']; + return resource?.Type ?? 'Unknown'; + } + + public equalTo(other: ResourceLocation): boolean { + return this.logicalResourceId === other.logicalResourceId && this.stack.stackName === other.stack.stackName; + } +} + +/** + * A mapping between a source and a destination location. + */ +export class ResourceMapping { + constructor(public readonly source: ResourceLocation, public readonly destination: ResourceLocation) { + } + + public toTypedMapping(): TypedMapping { + return { + // the type is the same in both source and destination, + // so we can use either one + type: this.source.getType(), + sourcePath: this.source.toPath(), + destinationPath: this.destination.toPath(), + }; + } +} + diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts index a4d330cfb..9260a63e6 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/index.ts @@ -10,8 +10,11 @@ import type { SdkProvider } from '../aws-auth'; import { Mode } from '../plugin'; import { StringWriteStream } from '../streams'; import type { CloudFormationStack } from './cloudformation'; +import { ResourceMapping, ResourceLocation } from './cloudformation'; import { computeResourceDigests, hashObject } from './digest'; -import type { SkipList } from './skip'; +import { NeverSkipList, type SkipList } from './skip'; + +export * from './skip'; /** * Represents a set of possible movements of a resource from one location @@ -34,56 +37,6 @@ export class AmbiguityError extends Error { } } -/** - * This class mirrors the `ResourceLocation` interface from CloudFormation, - * but is richer, since it has a reference to the stack object, rather than - * merely the stack name. - */ -export class ResourceLocation { - constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) { - } - - public toPath(): string { - const stack = this.stack; - const resource = stack.template.Resources?.[this.logicalResourceId]; - const result = resource?.Metadata?.['aws:cdk:path']; - - if (result != null) { - return result; - } - - // If the path is not available, we can use stack name and logical ID - return `${stack.stackName}.${this.logicalResourceId}`; - } - - public getType(): string { - const resource = this.stack.template.Resources?.[this.logicalResourceId ?? '']; - return resource?.Type ?? 'Unknown'; - } - - public equalTo(other: ResourceLocation): boolean { - return this.logicalResourceId === other.logicalResourceId && this.stack.stackName === other.stack.stackName; - } -} - -/** - * A mapping between a source and a destination location. - */ -export class ResourceMapping { - constructor(public readonly source: ResourceLocation, public readonly destination: ResourceLocation) { - } - - public toTypedMapping(): TypedMapping { - return { - // the type is the same in both source and destination, - // so we can use either one - type: this.source.getType(), - sourcePath: this.source.toPath(), - destinationPath: this.destination.toPath(), - }; - } -} - function groupByKey(entries: [string, A][]): Record { const result: Record = {}; @@ -122,7 +75,6 @@ export function ambiguousMovements(movements: ResourceMovement[]) { export function resourceMappings( movements: ResourceMovement[], stacks?: CloudFormationStack[], - skipList?: SkipList, ): ResourceMapping[] { const stacksPredicate = stacks == null @@ -134,22 +86,10 @@ export function resourceMappings( return stacks.some((stack) => stackNames.includes(stack.stackName)); }; - const logicalIdsPredicate = - skipList == null - ? () => true - : (m: ResourceMapping) => { - return !skipList!.resourceLocations.some( - (loc) => - loc.StackName === m.destination.stack.stackName && - loc.LogicalResourceId === m.destination.logicalResourceId, - ); - }; - return movements .filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0])) .map(([pre, post]) => new ResourceMapping(pre[0], post[0])) - .filter(stacksPredicate) - .filter(logicalIdsPredicate); + .filter(stacksPredicate); } function removeUnmovedResources(m: Record): Record { @@ -212,6 +152,7 @@ function resourceDigests(stack: CloudFormationStack): [string, ResourceLocation] export async function findResourceMovements( stacks: CloudFormationStack[], sdkProvider: SdkProvider, + skipList: SkipList = new NeverSkipList(), ): Promise { const stackGroups: Map = new Map(); @@ -232,7 +173,11 @@ export async function findResourceMovements( for (const [_, [before, after]] of stackGroups) { result.push(...resourceMovements(before, after)); } - return result; + + return result.filter(mov => { + const after = mov[1]; + return after.every(l => !skipList.isSkipped(l)); + }); } async function getDeployedStacks( diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts index 6e96f483e..fab766eec 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts @@ -3,18 +3,22 @@ import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; import type { ResourceLocation as CfnResourceLocation } from '@aws-sdk/client-cloudformation'; import { ToolkitError } from '../toolkit-error'; +import type { ResourceLocation } from './cloudformation'; export interface SkipList { - resourceLocations: CfnResourceLocation[]; + isSkipped(location: ResourceLocation): boolean; } export class ManifestSkipList implements SkipList { - constructor(private readonly manifest: AssemblyManifest) { + private readonly skippedLocations: CfnResourceLocation[]; + + constructor(manifest: AssemblyManifest) { + this.skippedLocations = this.getSkippedLocations(manifest); } - get resourceLocations(): CfnResourceLocation[] { + private getSkippedLocations(asmManifest: AssemblyManifest): CfnResourceLocation[] { // First, we need to filter the artifacts to only include CloudFormation stacks - const stackManifests = Object.entries(this.manifest.artifacts ?? {}).filter( + const stackManifests = Object.entries(asmManifest.artifacts ?? {}).filter( ([_, manifest]) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK, ); @@ -38,34 +42,58 @@ export class ManifestSkipList implements SkipList { } return result; } + + isSkipped(location: ResourceLocation): boolean { + return this.skippedLocations.some( + (loc) => loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId, + ); + } } export class SkipFile implements SkipList { + private readonly skippedLocations: CfnResourceLocation[]; + private readonly skippedPaths: string[]; + constructor(private readonly filePath?: string) { - } + this.skippedLocations = []; + this.skippedPaths = []; - get resourceLocations(): CfnResourceLocation[] { if (!this.filePath) { - return []; + return; } + const parsedData = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')); if (!isValidSkipFileContent(parsedData)) { throw new ToolkitError('The content of a skip file must be a JSON array of strings'); } - const result: CfnResourceLocation[] = []; + const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/; + const pathRegex = /^\w*(\/.*)*$/; + parsedData.forEach((item: string) => { - const parts = item.split('.'); - if (parts.length !== 2) { - throw new ToolkitError(`Invalid resource location format: ${item}. Expected format: stackName.logicalId`); + if (locationRegex.test(item)) { + const [stackName, logicalId] = item.split('.'); + this.skippedLocations.push({ + StackName: stackName, + LogicalResourceId: logicalId, + }); + } else if (pathRegex.test(item)) { + this.skippedPaths.push(item); + } else { + throw new ToolkitError( + `Invalid resource location format: ${item}. Expected formats: stackName.logicalId or a construct path`, + ); } - const [stackName, logicalId] = parts; - result.push({ - StackName: stackName, - LogicalResourceId: logicalId, - }); }); - return result; + } + + isSkipped(location: ResourceLocation): boolean { + const containsLocation = this.skippedLocations.some((loc) => { + return loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId; + }); + + const containsPath = this.skippedPaths.some((path) => location.toPath() === path); + return containsLocation || containsPath; } } @@ -77,7 +105,23 @@ export class UnionSkipList implements SkipList { constructor(private readonly skipLists: SkipList[]) { } - get resourceLocations(): CfnResourceLocation[] { - return this.skipLists.flatMap((skipList) => skipList.resourceLocations); + isSkipped(location: ResourceLocation): boolean { + return this.skipLists.some((skipList) => skipList.isSkipped(location)); + } +} + +export class NeverSkipList implements SkipList { + isSkipped(_location: ResourceLocation): boolean { + return false; } } + +export class AlwaysSkipList implements SkipList { + isSkipped(_location: ResourceLocation): boolean { + return true; + } +} + +export function fromManifestAndSkipFile(manifest: AssemblyManifest, skipFile?: string): SkipList { + return new UnionSkipList([new ManifestSkipList(manifest), new SkipFile(skipFile)]); +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts index 12635319d..c6f14dc95 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -16,11 +16,16 @@ export interface RefactorOptions { stacks?: StackSelector; /** - * The absolute path to a file that contains a list of - * resources to skip during the refactor. The file should - * be in JSON format and contain an array of _destination_ - * logical IDs, that is, the logical IDs of the resources - * as they would be after the refactor. + * The absolute path to a file that contains a list of resources to + * skip during the refactor. The file should be in JSON format and + * contain an array of _destination_ locations that should be skipped, + * i.e., the location to which a resource would be moved if the + * refactor were to happen. + * + * The format of the locations in the file can be either: + * + * - Stack name and logical ID (e.g. `Stack1.MyQueue`) + * - A construct path (e.g. `Stack1/Foo/Bar/Resource`). */ skipFile?: string; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 19196624c..546cfe607 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -9,7 +9,6 @@ import { NonInteractiveIoHost } from './non-interactive-io-host'; import type { ToolkitServices } from './private'; import { assemblyFromSource } from './private'; import type { DeployResult, DestroyResult, RollbackResult } from './types'; -import { SkipFile, ManifestSkipList, UnionSkipList } from '../../../tmp-toolkit-helpers/src/api/refactoring/skip'; import type { BootstrapEnvironments, BootstrapOptions, @@ -71,6 +70,7 @@ import { resourceMappings, WorkGraphBuilder, makeRequestHandler, + fromManifestAndSkipFile, } from '../api/shared-private'; import type { AssemblyData, StackDetails, ToolkitAction } from '../api/shared-public'; import { PermissionChangeType, PluginHost } from '../api/shared-public'; @@ -985,17 +985,12 @@ export class Toolkit extends CloudAssemblySourceBuilder { const stacks = await assembly.selectStacksV2(ALL_STACKS); const sdkProvider = await this.sdkProvider('refactor'); - const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider); + const skipList = fromManifestAndSkipFile(assembly.cloudAssembly.manifest, options.skipFile); + const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider, skipList); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); - - const skipList = new UnionSkipList([ - new ManifestSkipList(assembly.cloudAssembly.manifest), - new SkipFile(options.skipFile), - ]); - - const mappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); + const mappings = resourceMappings(movements, filteredStacks.stackArtifacts); const typedMappings = mappings.map(m => m.toTypedMapping()); await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatTypedMappings(typedMappings), { typedMappings, diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 91abab0e0..3b50adc2d 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -14,16 +14,12 @@ import type { ToolkitAction } from '../../../@aws-cdk/tmp-toolkit-helpers/src/ap import { ambiguousMovements, findResourceMovements, + fromManifestAndSkipFile, resourceMappings, ToolkitError, } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api'; import { asIoHelper } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/io/private'; import { AmbiguityError } from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; -import { - SkipFile, - ManifestSkipList, - UnionSkipList, -} from '../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; import { PermissionChangeType } from '../../../@aws-cdk/tmp-toolkit-helpers/src/payloads'; import type { ToolkitOptions } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; import { Toolkit } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; @@ -1233,21 +1229,16 @@ export class CdkToolkit { // Example: resource X was moved from Stack A to Stack B. If we only select Stack A, // we will only see a deletion of resource X, but not the creation of resource X in Stack B. const stacks = await this.selectStacksForList([]); - const movements = await findResourceMovements(stacks.stackArtifacts, this.props.sdkProvider); + const assembly = await this.assembly(); + const skipList = fromManifestAndSkipFile(assembly.assembly.manifest, options.skipFile); + const movements = await findResourceMovements(stacks.stackArtifacts, this.props.sdkProvider, skipList); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { // Now we can filter the stacks to only include the ones that are relevant for the user. const patterns = options.selector.allTopLevel ? [] : options.selector.patterns; const filteredStacks = await this.selectStacksForList(patterns); - - const assembly = await this.assembly(); - const skipList = new UnionSkipList([ - new ManifestSkipList(assembly.assembly.manifest), - new SkipFile(options.skipFile), - ]); - - const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts, skipList); + const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts); const typedMappings = selectedMappings.map(m => m.toTypedMapping()); formatTypedMappings(process.stdout, typedMappings); } else { @@ -1950,11 +1941,16 @@ export interface RefactorOptions { selector: StackSelector; /** - * The absolute path to a file that contains a list of - * resources to skip during the refactor. The file should - * be in JSON format and contain an array of _destination_ - * logical IDs, that is, the logical IDs of the resources - * as they would be after the refactor. + * The absolute path to a file that contains a list of resources to + * skip during the refactor. The file should be in JSON format and + * contain an array of _destination_ locations that should be skipped, + * i.e., the location to which a resource would be moved if the + * refactor were to happen. + * + * The format of the locations in the file can be either: + * + * - Stack name and logical ID (e.g. `Stack1.MyQueue`) + * - A construct path (e.g. `Stack1/Foo/Bar/Resource`). */ skipFile?: string; } diff --git a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts index 67b0a070d..9be79fa24 100644 --- a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts +++ b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts @@ -10,16 +10,21 @@ import { ResourceMapping as CfnResourceMapping, } from '@aws-sdk/client-cloudformation'; import { + AlwaysSkipList, ambiguousMovements, findResourceMovements, - ResourceLocation, - ResourceMapping, resourceMappings, resourceMovements, } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; +import { + type CloudFormationStack, + ResourceLocation, + ResourceMapping, +} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation'; import { computeResourceDigests } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/digest'; import { mockCloudFormationClient, MockSdkProvider } from '../../_helpers/mock-sdk'; import { expect } from '@jest/globals'; +import { SkipList } from '@aws-cdk/tmp-toolkit-helpers'; const cloudFormationClient = mockCloudFormationClient; @@ -1235,13 +1240,7 @@ describe('environment grouping', () => { }), }); - const provider = new MockSdkProvider(); - provider.returnsDefaultAccounts(environment.account); - - const movements = await findResourceMovements([stack1, stack2], provider); - expect(ambiguousMovements(movements)).toEqual([]); - - expect(resourceMappings(movements).map(toCfnMapping)).toEqual([ + expect(await mappings([stack1, stack2])).toEqual([ { Destination: { LogicalResourceId: 'Bucket', @@ -1253,6 +1252,15 @@ describe('environment grouping', () => { }, }, ]); + + expect(await mappings([stack1, stack2], new AlwaysSkipList())).toEqual([]); + + async function mappings(stacks: CloudFormationStack[], skipList?: SkipList) { + const provider = new MockSdkProvider(); + provider.returnsDefaultAccounts(environment.account); + const movements2 = await findResourceMovements(stacks, provider, skipList); + return resourceMappings(movements2).map(toCfnMapping); + } }); test('does not produce cross-environment mappings', async () => { diff --git a/packages/aws-cdk/test/api/refactoring/skip.test.ts b/packages/aws-cdk/test/api/refactoring/skip.test.ts index ee9e25047..4a208aab0 100644 --- a/packages/aws-cdk/test/api/refactoring/skip.test.ts +++ b/packages/aws-cdk/test/api/refactoring/skip.test.ts @@ -1,13 +1,49 @@ import * as fs from 'node:fs'; import { ManifestSkipList, + NeverSkipList, SkipFile, + AlwaysSkipList, UnionSkipList, -} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip'; +} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +import { + CloudFormationStack, + ResourceLocation, +} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation'; + +const environment = { + name: 'prod', + account: '123456789012', + region: 'us-east-1', +}; + +const stack1: CloudFormationStack = { + stackName: 'Stack1', + environment, + template: {}, +}; +const stack2: CloudFormationStack = { + stackName: 'Stack2', + environment, + template: { + Resources: { + Resource3: { + Type: 'AWS::S3::Bucket', + Metadata: { + 'aws:cdk:path': 'Stack2/Resource3', + }, + } + } + }, +}; + +const resource1 = new ResourceLocation(stack1, 'Resource1'); +const resource2 = new ResourceLocation(stack2, 'Resource2'); +const resource3 = new ResourceLocation(stack2, 'Resource3'); describe('ManifestSkipList', () => { - test('returns resource locations marked with SKIP_REFACTOR in the manifest', () => { + test('locations marked with SKIP_REFACTOR in the manifest are skipped', () => { const manifest = { artifacts: { Stack1: { @@ -40,19 +76,13 @@ describe('ManifestSkipList', () => { }; const skipList = new ManifestSkipList(manifest as any); - expect(skipList.resourceLocations).toEqual([ - { - StackName: 'Stack1', - LogicalResourceId: 'Resource1', - }, - { - StackName: 'Stack2', - LogicalResourceId: 'Resource2', - }, - ]); + + expect(skipList.isSkipped(resource1)).toBe(true); + expect(skipList.isSkipped(resource2)).toBe(true); + expect(skipList.isSkipped(resource3)).toBe(false); }); - test('returns an empty array if no SKIP_REFACTOR entries exist', () => { + test('nothing is skipped if no SKIP_REFACTOR entries exist', () => { const manifest = { artifacts: { Stack1: { @@ -65,32 +95,27 @@ describe('ManifestSkipList', () => { }; const skipList = new ManifestSkipList(manifest as any); - expect(skipList.resourceLocations).toEqual([]); + expect(skipList.isSkipped(resource1)).toBe(false); }); }); describe('SkipFile', () => { - test('returns resource locations from a valid JSON file', () => { + test('valid resources on a valid list are skipped', () => { const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify(['Stack1.Resource1', 'Stack2.Resource2']); + const fileContent = JSON.stringify(['Stack1.Resource1', 'Stack2/Resource3']); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); const skipList = new SkipFile(filePath); - expect(skipList.resourceLocations).toEqual([ - { - StackName: 'Stack1', - LogicalResourceId: 'Resource1', - }, - { - StackName: 'Stack2', - LogicalResourceId: 'Resource2', - }, - ]); + expect(skipList.isSkipped(resource1)).toBe(true); + expect(skipList.isSkipped(resource2)).toBe(false); + expect(skipList.isSkipped(resource3)).toBe(true); }); - test('returns an empty array if no file path is provided', () => { + test('nothing is skipped if no file path is provided', () => { const skipList = new SkipFile(); - expect(skipList.resourceLocations).toEqual([]); + expect(skipList.isSkipped(resource1)).toBe(false); + expect(skipList.isSkipped(resource2)).toBe(false); + expect(skipList.isSkipped(resource3)).toBe(false); }); test('throws an error if the content is not an array', () => { @@ -98,8 +123,7 @@ describe('SkipFile', () => { const fileContent = JSON.stringify({ spuriousKey: ['Stack1.Resource1', 'Stack2.Resource2'] }); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new SkipFile(filePath); - expect(() => skipList.resourceLocations).toThrow('The content of a skip file must be a JSON array of strings'); + expect(() => new SkipFile(filePath)).toThrow('The content of a skip file must be a JSON array of strings'); }); test('throws an error if the content is an array but not of strings', () => { @@ -107,66 +131,32 @@ describe('SkipFile', () => { const fileContent = JSON.stringify([1, 2, 3]); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new SkipFile(filePath); - expect(() => skipList.resourceLocations).toThrow('The content of a skip file must be a JSON array of strings'); + expect(() => new SkipFile(filePath)).toThrow('The content of a skip file must be a JSON array of strings'); }); test('throws an error if the some entries are not valid resource locations', () => { const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify(['Stack1.Resource1', 'InvalidResourceLocation']); + const fileContent = JSON.stringify(['Stack1.Resource1', 'Invalid-Resource-Location']); jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - const skipList = new SkipFile(filePath); - expect(() => skipList.resourceLocations).toThrow( - 'Invalid resource location format: InvalidResourceLocation. Expected format: stackName.logicalId', - ); + expect(() => new SkipFile(filePath)).toThrow(/Invalid resource location format: Invalid-Resource-Location/); }); }); describe('UnionSkipList', () => { - test('combines resource IDs from multiple skip lists', () => { - const skipList1 = { - resourceIds: ['Resource1', 'Resource2'], - resourceLocations: [ - { - StackName: 'Stack1', - LogicalResourceId: 'Resource1', - }, - { - StackName: 'Stack2', - LogicalResourceId: 'Resource2', - }, - ], - }; - const skipList2 = { - resourceIds: ['Resource3'], - resourceLocations: [ - { - StackName: 'Stack3', - LogicalResourceId: 'Resource3', - }, - ], - }; + test('skips a resource if at least one underlying list skips', () => { + const skipList1 = new AlwaysSkipList(); + const skipList2 = new NeverSkipList(); const unionSkipList = new UnionSkipList([skipList1, skipList2]); - expect(unionSkipList.resourceLocations).toEqual([ - { - StackName: 'Stack1', - LogicalResourceId: 'Resource1', - }, - { - StackName: 'Stack2', - LogicalResourceId: 'Resource2', - }, - { - StackName: 'Stack3', - LogicalResourceId: 'Resource3', - }, - ]); + expect(unionSkipList.isSkipped(resource1)).toBe(true); }); - test('returns an empty array if no skip lists are provided', () => { - const unionSkipList = new UnionSkipList([]); - expect(unionSkipList.resourceLocations).toEqual([]); + test('does not skip a resource if all underlying lists do not skip', () => { + const skipList1 = new NeverSkipList(); + const skipList2 = new NeverSkipList(); + + const unionSkipList = new UnionSkipList([skipList1, skipList2]); + expect(unionSkipList.isSkipped(resource1)).toBe(false); }); }); From b6b27837bc0e61ef13cb9ddc1350430e1bf0d90c Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 29 Apr 2025 16:41:37 +0100 Subject: [PATCH 06/16] regex --- .../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts index fab766eec..1bca4f0a9 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts @@ -68,7 +68,7 @@ export class SkipFile implements SkipList { } const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/; - const pathRegex = /^\w*(\/.*)*$/; + const pathRegex = /^\w*(\/\w)*$/; parsedData.forEach((item: string) => { if (locationRegex.test(item)) { From ba46c6b164776b9db98393a9be210a7a7d87b395 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 29 Apr 2025 16:51:40 +0100 Subject: [PATCH 07/16] regex --- .../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts index 1bca4f0a9..325dad64e 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/skip.ts @@ -68,7 +68,7 @@ export class SkipFile implements SkipList { } const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/; - const pathRegex = /^\w*(\/\w)*$/; + const pathRegex = /^\w*(\/\w*)*$/; parsedData.forEach((item: string) => { if (locationRegex.test(item)) { From c39e76980dcdd173c05d6a3c39797c44bf134da6 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 30 Apr 2025 10:47:40 +0100 Subject: [PATCH 08/16] Fixes after merge --- .../toolkit-lib/lib/api/refactoring/skip.ts | 127 ++++++++++++++++++ .../toolkit-lib/lib/toolkit/toolkit.ts | 2 +- .../test/api/refactoring/refactoring.test.ts | 4 +- 3 files changed, 130 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts index e69de29bb..325dad64e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts @@ -0,0 +1,127 @@ +import * as fs from 'node:fs'; +import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema'; +import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +import type { ResourceLocation as CfnResourceLocation } from '@aws-sdk/client-cloudformation'; +import { ToolkitError } from '../toolkit-error'; +import type { ResourceLocation } from './cloudformation'; + +export interface SkipList { + isSkipped(location: ResourceLocation): boolean; +} + +export class ManifestSkipList implements SkipList { + private readonly skippedLocations: CfnResourceLocation[]; + + constructor(manifest: AssemblyManifest) { + this.skippedLocations = this.getSkippedLocations(manifest); + } + + private getSkippedLocations(asmManifest: AssemblyManifest): CfnResourceLocation[] { + // First, we need to filter the artifacts to only include CloudFormation stacks + const stackManifests = Object.entries(asmManifest.artifacts ?? {}).filter( + ([_, manifest]) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK, + ); + + const result: CfnResourceLocation[] = []; + for (let [stackName, manifest] of stackManifests) { + const locations = Object.values(manifest.metadata ?? {}) + // Then pick only the resources in each stack marked with SKIP_REFACTOR + .filter((entries) => + entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true), + ) + // Finally, get the logical ID of each resource + .map((entries) => { + const logicalIdEntry = entries.find((entry) => entry.type === ArtifactMetadataEntryType.LOGICAL_ID); + const location: CfnResourceLocation = { + StackName: stackName, + LogicalResourceId: logicalIdEntry!.data! as string, + }; + return location; + }); + result.push(...locations); + } + return result; + } + + isSkipped(location: ResourceLocation): boolean { + return this.skippedLocations.some( + (loc) => loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId, + ); + } +} + +export class SkipFile implements SkipList { + private readonly skippedLocations: CfnResourceLocation[]; + private readonly skippedPaths: string[]; + + constructor(private readonly filePath?: string) { + this.skippedLocations = []; + this.skippedPaths = []; + + if (!this.filePath) { + return; + } + + const parsedData = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')); + if (!isValidSkipFileContent(parsedData)) { + throw new ToolkitError('The content of a skip file must be a JSON array of strings'); + } + + const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/; + const pathRegex = /^\w*(\/\w*)*$/; + + parsedData.forEach((item: string) => { + if (locationRegex.test(item)) { + const [stackName, logicalId] = item.split('.'); + this.skippedLocations.push({ + StackName: stackName, + LogicalResourceId: logicalId, + }); + } else if (pathRegex.test(item)) { + this.skippedPaths.push(item); + } else { + throw new ToolkitError( + `Invalid resource location format: ${item}. Expected formats: stackName.logicalId or a construct path`, + ); + } + }); + } + + isSkipped(location: ResourceLocation): boolean { + const containsLocation = this.skippedLocations.some((loc) => { + return loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId; + }); + + const containsPath = this.skippedPaths.some((path) => location.toPath() === path); + return containsLocation || containsPath; + } +} + +function isValidSkipFileContent(data: any): data is string[] { + return Array.isArray(data) && data.every((item: any) => typeof item === 'string'); +} + +export class UnionSkipList implements SkipList { + constructor(private readonly skipLists: SkipList[]) { + } + + isSkipped(location: ResourceLocation): boolean { + return this.skipLists.some((skipList) => skipList.isSkipped(location)); + } +} + +export class NeverSkipList implements SkipList { + isSkipped(_location: ResourceLocation): boolean { + return false; + } +} + +export class AlwaysSkipList implements SkipList { + isSkipped(_location: ResourceLocation): boolean { + return true; + } +} + +export function fromManifestAndSkipFile(manifest: AssemblyManifest, skipFile?: string): SkipList { + return new UnionSkipList([new ManifestSkipList(manifest), new SkipFile(skipFile)]); +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 4b67261b3..0a1ceb06c 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -48,7 +48,7 @@ import type { IIoHost, IoMessageLevel } from '../api/io'; import type { IoHelper } from '../api/io/private'; import { asIoHelper, asSdkLogger, IO, SPAN, withoutColor, withoutEmojis, withTrimmedWhitespace } from '../api/io/private'; import { CloudWatchLogEventMonitor, findCloudWatchLogGroups } from '../api/logs-monitor'; -import { AmbiguityError, ambiguousMovements, findResourceMovements, formatAmbiguousMappings, formatTypedMappings, resourceMappings } from '../api/refactoring'; +import { AmbiguityError, ambiguousMovements, findResourceMovements, formatAmbiguousMappings, formatTypedMappings, fromManifestAndSkipFile, resourceMappings } from '../api/refactoring'; import { ResourceMigrator } from '../api/resource-import'; import type { AssemblyData, StackDetails, SuccessfulDeployStackResult, ToolkitAction } from '../api/shared-public'; import { PermissionChangeType, PluginHost, ToolkitError } from '../api/shared-public'; diff --git a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts index e17931ecf..ecf74e802 100644 --- a/packages/aws-cdk/test/api/refactoring/refactoring.test.ts +++ b/packages/aws-cdk/test/api/refactoring/refactoring.test.ts @@ -20,11 +20,11 @@ import { type CloudFormationStack, ResourceLocation, ResourceMapping, -} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation'; +} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation'; import { computeResourceDigests } from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring/digest'; import { mockCloudFormationClient, MockSdkProvider } from '../../_helpers/mock-sdk'; import { expect } from '@jest/globals'; -import { SkipList } from '@aws-cdk/tmp-toolkit-helpers'; +import { SkipList } from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring'; const cloudFormationClient = mockCloudFormationClient; From 6df06900064b3a2b5dc6eb69a097683d3dbfd68d Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 30 Apr 2025 11:05:27 +0100 Subject: [PATCH 09/16] Fix import --- packages/aws-cdk/test/api/refactoring/skip.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/test/api/refactoring/skip.test.ts b/packages/aws-cdk/test/api/refactoring/skip.test.ts index 4a208aab0..fae7ba97c 100644 --- a/packages/aws-cdk/test/api/refactoring/skip.test.ts +++ b/packages/aws-cdk/test/api/refactoring/skip.test.ts @@ -5,12 +5,12 @@ import { SkipFile, AlwaysSkipList, UnionSkipList, -} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring'; +} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; import { CloudFormationStack, ResourceLocation, -} from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/cloudformation'; +} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation'; const environment = { name: 'prod', From 8cc3278b837276eb05ee57f74fdc947fc088c392 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 30 Apr 2025 13:03:53 +0100 Subject: [PATCH 10/16] Reuse toolkit library --- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 47 ++++++++----------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 051d864f0..9fd922445 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1,6 +1,5 @@ import * as path from 'path'; import { format } from 'util'; -import { formatAmbiguousMappings, formatTypedMappings } from '@aws-cdk/cloudformation-diff'; import * as cxapi from '@aws-cdk/cx-api'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; @@ -11,15 +10,8 @@ import { CliIoHost } from './io-host'; import type { Configuration } from './user-configuration'; import { PROJECT_CONFIG } from './user-configuration'; import type { ToolkitAction } from '../../../@aws-cdk/toolkit-lib/lib/api'; -import { - ambiguousMovements, - findResourceMovements, - fromManifestAndSkipFile, - resourceMappings, - ToolkitError, -} from '../../../@aws-cdk/toolkit-lib/lib/api'; +import { StackSelectionStrategy, ToolkitError } from '../../../@aws-cdk/toolkit-lib/lib/api'; import { asIoHelper } from '../../../@aws-cdk/toolkit-lib/lib/api/io/private'; -import { AmbiguityError } from '../../../@aws-cdk/toolkit-lib/lib/api/refactoring'; import { PermissionChangeType } from '../../../@aws-cdk/toolkit-lib/lib/payloads'; import type { ToolkitOptions } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; import { Toolkit } from '../../../@aws-cdk/toolkit-lib/lib/toolkit'; @@ -1219,32 +1211,23 @@ export class CdkToolkit { } public async refactor(options: RefactorOptions): Promise { - if (!options.dryRun) { - info('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); + const tk = new Toolkit(); + + const casmSource = await tk.fromAssemblyBuilder(() => this.assembly()); + try { + await tk.refactor(casmSource, { + dryRun: options.dryRun, + skipFile: options.skipFile, + stacks: { + patterns: options.selector.patterns, + strategy: options.selector.patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, + }, + }); + } catch (e) { + error((e as Error).message); return 1; } - // Initially, we select all stacks to find all resource movements. - // Otherwise, we might miss some resources that are not in the selected stacks. - // Example: resource X was moved from Stack A to Stack B. If we only select Stack A, - // we will only see a deletion of resource X, but not the creation of resource X in Stack B. - const stacks = await this.selectStacksForList([]); - const assembly = await this.assembly(); - const skipList = fromManifestAndSkipFile(assembly.assembly.manifest, options.skipFile); - const movements = await findResourceMovements(stacks.stackArtifacts, this.props.sdkProvider, skipList); - const ambiguous = ambiguousMovements(movements); - - if (ambiguous.length === 0) { - // Now we can filter the stacks to only include the ones that are relevant for the user. - const patterns = options.selector.allTopLevel ? [] : options.selector.patterns; - const filteredStacks = await this.selectStacksForList(patterns); - const selectedMappings = resourceMappings(movements, filteredStacks.stackArtifacts); - const typedMappings = selectedMappings.map(m => m.toTypedMapping()); - formatTypedMappings(process.stdout, typedMappings); - } else { - const e = new AmbiguityError(ambiguous); - formatAmbiguousMappings(process.stdout, e.paths()); - } return 0; } From 5d7561341c3cf9710ef2dd4f51431d9024af4f55 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 30 Apr 2025 13:41:11 +0100 Subject: [PATCH 11/16] Fix imports --- .../test/api/refactoring/refactoring.test.ts | 18 +++++------------- .../test/api/refactoring/skip.test.ts | 16 ++++++++-------- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts index 09960b960..73d2c65e0 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts @@ -7,15 +7,9 @@ import type { ResourceLocation as CfnResourceLocation, ResourceMapping as CfnResourceMapping, } from '@aws-sdk/client-cloudformation'; -import { - GetTemplateCommand, - ListStacksCommand, -} from '@aws-sdk/client-cloudformation'; +import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; import { expect } from '@jest/globals'; -import type { - ResourceLocation, - ResourceMapping, -} from '../../../lib/api/refactoring'; +import type { SkipList } from '../../../lib/api/refactoring'; import { AlwaysSkipList, ambiguousMovements, @@ -23,15 +17,13 @@ import { resourceMappings, resourceMovements, } from '../../../lib/api/refactoring'; -import { - type CloudFormationStack, +import type { ResourceLocation, ResourceMapping, -} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation'; + CloudFormationStack, +} from '../../../lib/api/refactoring/cloudformation'; import { computeResourceDigests } from '../../../lib/api/refactoring/digest'; import { mockCloudFormationClient, MockSdkProvider } from '../../_helpers/mock-sdk'; -import { expect } from '@jest/globals'; -import { SkipList } from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring'; const cloudFormationClient = mockCloudFormationClient; diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts index fae7ba97c..510b94635 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts @@ -1,16 +1,16 @@ import * as fs from 'node:fs'; +import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; import { ManifestSkipList, NeverSkipList, SkipFile, AlwaysSkipList, UnionSkipList, -} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring'; -import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; +} from '../../../lib/api/refactoring'; +import type { CloudFormationStack } from '../../../lib/api/refactoring/cloudformation'; import { - CloudFormationStack, ResourceLocation, -} from '../../../../@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation'; +} from '../../../lib/api/refactoring/cloudformation'; const environment = { name: 'prod', @@ -33,8 +33,8 @@ const stack2: CloudFormationStack = { Metadata: { 'aws:cdk:path': 'Stack2/Resource3', }, - } - } + }, + }, }, }; @@ -46,7 +46,7 @@ describe('ManifestSkipList', () => { test('locations marked with SKIP_REFACTOR in the manifest are skipped', () => { const manifest = { artifacts: { - Stack1: { + 'Stack1': { type: ArtifactType.AWS_CLOUDFORMATION_STACK, metadata: { LogicalId1: [ @@ -55,7 +55,7 @@ describe('ManifestSkipList', () => { ], }, }, - Stack2: { + 'Stack2': { type: ArtifactType.AWS_CLOUDFORMATION_STACK, metadata: { LogicalId2: [ From 8832adae6c9579e97a7213907619b79a8b96f5bd Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Thu, 1 May 2025 10:16:15 +0100 Subject: [PATCH 12/16] README --- packages/aws-cdk/README.md | 101 ++++++++++++++++++++++++++++++------- 1 file changed, 83 insertions(+), 18 deletions(-) diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index bfbd7ae04..2d517366b 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -11,24 +11,25 @@ The AWS CDK Toolkit provides the `cdk` command-line interface that can be used to work with AWS CDK applications. -| Command | Description | -| ------------------------------------- | ---------------------------------------------------------------------------------- | -| [`cdk docs`](#cdk-docs) | Access the online documentation | -| [`cdk init`](#cdk-init) | Start a new CDK project (app or library) | -| [`cdk list`](#cdk-list) | List stacks and their dependencies in an application | -| [`cdk synth`](#cdk-synthesize) | Synthesize a CDK app to CloudFormation template(s) | -| [`cdk diff`](#cdk-diff) | Diff stacks against current state | -| [`cdk deploy`](#cdk-deploy) | Deploy a stack into an AWS account | -| [`cdk rollback`](#cdk-rollback) | Roll back a failed deployment | -| [`cdk import`](#cdk-import) | Import existing AWS resources into a CDK stack | -| [`cdk migrate`](#cdk-migrate) | Migrate AWS resources, CloudFormation stacks, and CloudFormation templates to CDK | -| [`cdk watch`](#cdk-watch) | Watches a CDK app for deployable and hotswappable changes | -| [`cdk destroy`](#cdk-destroy) | Deletes a stack from an AWS account | -| [`cdk bootstrap`](#cdk-bootstrap) | Deploy a toolkit stack to support deploying large stacks & artifacts | -| [`cdk gc`](#cdk-gc) | Garbage collect assets associated with the bootstrapped stack | -| [`cdk doctor`](#cdk-doctor) | Inspect the environment and produce information useful for troubleshooting | -| [`cdk acknowledge`](#cdk-acknowledge) | Acknowledge (and hide) a notice by issue number | -| [`cdk notices`](#cdk-notices) | List all relevant notices for the application | +| Command | Description | +|---------------------------------------|-----------------------------------------------------------------------------------| +| [`cdk docs`](#cdk-docs) | Access the online documentation | +| [`cdk init`](#cdk-init) | Start a new CDK project (app or library) | +| [`cdk list`](#cdk-list) | List stacks and their dependencies in an application | +| [`cdk synth`](#cdk-synthesize) | Synthesize a CDK app to CloudFormation template(s) | +| [`cdk diff`](#cdk-diff) | Diff stacks against current state | +| [`cdk deploy`](#cdk-deploy) | Deploy a stack into an AWS account | +| [`cdk rollback`](#cdk-rollback) | Roll back a failed deployment | +| [`cdk import`](#cdk-import) | Import existing AWS resources into a CDK stack | +| [`cdk migrate`](#cdk-migrate) | Migrate AWS resources, CloudFormation stacks, and CloudFormation templates to CDK | +| [`cdk watch`](#cdk-watch) | Watches a CDK app for deployable and hotswappable changes | +| [`cdk destroy`](#cdk-destroy) | Deletes a stack from an AWS account | +| [`cdk bootstrap`](#cdk-bootstrap) | Deploy a toolkit stack to support deploying large stacks & artifacts | +| [`cdk gc`](#cdk-gc) | Garbage collect assets associated with the bootstrapped stack | +| [`cdk doctor`](#cdk-doctor) | Inspect the environment and produce information useful for troubleshooting | +| [`cdk acknowledge`](#cdk-acknowledge) | Acknowledge (and hide) a notice by issue number | +| [`cdk notices`](#cdk-notices) | List all relevant notices for the application | +| [`cdk refactor`](#cdk-refactor) | Moves resources between stacks or within the same stack | - [Bundling](#bundling) - [MFA Support](#mfa-support) @@ -1066,6 +1067,70 @@ $ cdk doctor - AWS_SDK_LOAD_CONFIG = 1 ``` +### `cdk refactor` + +⚠️**CAUTION**⚠️: CDK Refactor is currently experimental and may have +breaking changes in the future. Make sure to use the `--unstable=refactor` flag +when using this command. + +Compares the infrastructure specified in the current state of the CDK app with +the currently deployed application, to determine if any resource was moved +(to a different stack or to a different logical ID, or both). The CLI will +show the correspondence between the old and new locations in a table: + +``` +$ cdk refactor --unstable=refactor --dry-run + +The following resources were moved or renamed: + +┌───────────────────────────────┬───────────────────────────────┬───────────────────────────────────┐ +│ Resource Type │ Old Construct Path │ New Construct Path │ +├───────────────────────────────┼───────────────────────────────┼───────────────────────────────────┤ +│ AWS::S3::Bucket │ MyStack/Bucket/Resource │ Web/Website/Origin/Resource │ +├───────────────────────────────┼───────────────────────────────┼───────────────────────────────────┤ +│ AWS::CloudFront::Distribution │ MyStack/Distribution/Resource │ Web/Website/Distribution/Resource │ +├───────────────────────────────┼───────────────────────────────┼───────────────────────────────────┤ +│ AWS::Lambda::Function │ MyStack/Function/Resource │ Service/Function/Resource │ +└───────────────────────────────┴───────────────────────────────┴───────────────────────────────────┘ +``` + +Note the use of the `--dry-run` flag. When this flag is used, the CLI will +show this table and exit. Eventually, the CLI will also be able to automatically +apply the refactor on your CloudFormation stacks. But for now, only the dry-run +mode is supported. + +If you want to exclude some resources from the refactor, you can pass a skip +file, containing a list of destination locations to skip, which can be +either the stack name + logical ID, or the construct path. For example, if +you don't want to include the bucket and the distribution from the table +above in the refactor, you can create a file called `skip.json` with the +following content: + +```json +[ + "Web/Website/Origin/Resource", + "Web/Website/Distribution/Resource" +] +``` + +and pass it to the CLI via the `--skip-file` flag: + +```shell +$ cdk refactor --skip-file skip.json --unstable=refactor --dry-run +``` + +If your application has more than one stack, and you want the refactor +command to consider only a subset of them, you can pass a list of stack +patterns as a parameter: + +```shell +$ cdk refactor Web* --unstable=refactor --dry-run +``` + +The pattern language is the same as the one used in the `cdk deploy` command. +However, unlike `cdk deploy`, in the absence of this parameter, all stacks are +considered. + ## Notices CDK Notices are important messages regarding security vulnerabilities, regressions, and usage of unsupported From 807cd60a32458be31a217c157a87fec65d6f8473 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Thu, 1 May 2025 17:11:03 +0100 Subject: [PATCH 13/16] Addressed comments --- .../toolkit-lib/lib/actions/refactor/index.ts | 7 +- .../toolkit-lib/lib/api/refactoring/index.ts | 6 +- .../toolkit-lib/lib/api/refactoring/skip.ts | 57 +++++------- .../toolkit-lib/lib/toolkit/toolkit.ts | 4 +- .../test/api/refactoring/refactoring.test.ts | 8 +- .../test/api/refactoring/skip.test.ts | 89 ++++++------------- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 15 ++-- packages/aws-cdk/lib/cli/cli-config.ts | 2 +- packages/aws-cdk/lib/cli/cli.ts | 2 +- .../aws-cdk/lib/cli/convert-to-user-input.ts | 4 +- .../lib/cli/parse-command-line-arguments.ts | 2 +- packages/aws-cdk/lib/cli/user-input.ts | 2 +- 12 files changed, 78 insertions(+), 120 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts index c6f14dc95..fad93c3e1 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -16,9 +16,8 @@ export interface RefactorOptions { stacks?: StackSelector; /** - * The absolute path to a file that contains a list of resources to - * skip during the refactor. The file should be in JSON format and - * contain an array of _destination_ locations that should be skipped, + * A list of resources to skip during the refactor. Elements of + * this list should be _destination_ locations that should be skipped, * i.e., the location to which a resource would be moved if the * refactor were to happen. * @@ -27,5 +26,5 @@ export interface RefactorOptions { * - Stack name and logical ID (e.g. `Stack1.MyQueue`) * - A construct path (e.g. `Stack1/Foo/Bar/Resource`). */ - skipFile?: string; + exclude?: string[]; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts index a12ba5b73..118380c6b 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts @@ -12,7 +12,7 @@ import { StringWriteStream } from '../streams'; import type { CloudFormationStack } from './cloudformation'; import { ResourceMapping, ResourceLocation } from './cloudformation'; import { computeResourceDigests, hashObject } from './digest'; -import { NeverSkipList, type SkipList } from './skip'; +import { NeverExclude, type ExcludeList } from './skip'; export * from './skip'; @@ -152,7 +152,7 @@ function resourceDigests(stack: CloudFormationStack): [string, ResourceLocation] export async function findResourceMovements( stacks: CloudFormationStack[], sdkProvider: SdkProvider, - skipList: SkipList = new NeverSkipList(), + skipList: ExcludeList = new NeverExclude(), ): Promise { const stackGroups: Map = new Map(); @@ -176,7 +176,7 @@ export async function findResourceMovements( return result.filter(mov => { const after = mov[1]; - return after.every(l => !skipList.isSkipped(l)); + return after.every(l => !skipList.isExcluded(l)); }); } diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts index 325dad64e..eadf493c7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts @@ -1,15 +1,13 @@ -import * as fs from 'node:fs'; import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; import type { ResourceLocation as CfnResourceLocation } from '@aws-sdk/client-cloudformation'; -import { ToolkitError } from '../toolkit-error'; import type { ResourceLocation } from './cloudformation'; -export interface SkipList { - isSkipped(location: ResourceLocation): boolean; +export interface ExcludeList { + isExcluded(location: ResourceLocation): boolean; } -export class ManifestSkipList implements SkipList { +export class ManifestExcludeList implements ExcludeList { private readonly skippedLocations: CfnResourceLocation[]; constructor(manifest: AssemblyManifest) { @@ -43,51 +41,41 @@ export class ManifestSkipList implements SkipList { return result; } - isSkipped(location: ResourceLocation): boolean { + isExcluded(location: ResourceLocation): boolean { return this.skippedLocations.some( (loc) => loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId, ); } } -export class SkipFile implements SkipList { +export class InMemoryExcludeList implements ExcludeList { private readonly skippedLocations: CfnResourceLocation[]; private readonly skippedPaths: string[]; - constructor(private readonly filePath?: string) { + constructor(items: string[]) { this.skippedLocations = []; this.skippedPaths = []; - if (!this.filePath) { + if (items.length === 0) { return; } - const parsedData = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')); - if (!isValidSkipFileContent(parsedData)) { - throw new ToolkitError('The content of a skip file must be a JSON array of strings'); - } - const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/; - const pathRegex = /^\w*(\/\w*)*$/; - parsedData.forEach((item: string) => { + items.forEach((item: string) => { if (locationRegex.test(item)) { const [stackName, logicalId] = item.split('.'); this.skippedLocations.push({ StackName: stackName, LogicalResourceId: logicalId, }); - } else if (pathRegex.test(item)) { - this.skippedPaths.push(item); } else { - throw new ToolkitError( - `Invalid resource location format: ${item}. Expected formats: stackName.logicalId or a construct path`, - ); + this.skippedPaths.push(item); } }); } - isSkipped(location: ResourceLocation): boolean { + isExcluded(location: ResourceLocation): boolean { const containsLocation = this.skippedLocations.some((loc) => { return loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId; }); @@ -97,31 +85,28 @@ export class SkipFile implements SkipList { } } -function isValidSkipFileContent(data: any): data is string[] { - return Array.isArray(data) && data.every((item: any) => typeof item === 'string'); -} - -export class UnionSkipList implements SkipList { - constructor(private readonly skipLists: SkipList[]) { +export class UnionExcludeList implements ExcludeList { + constructor(private readonly skipLists: ExcludeList[]) { } - isSkipped(location: ResourceLocation): boolean { - return this.skipLists.some((skipList) => skipList.isSkipped(location)); + isExcluded(location: ResourceLocation): boolean { + return this.skipLists.some((skipList) => skipList.isExcluded(location)); } } -export class NeverSkipList implements SkipList { - isSkipped(_location: ResourceLocation): boolean { +export class NeverExclude implements ExcludeList { + isExcluded(_location: ResourceLocation): boolean { return false; } } -export class AlwaysSkipList implements SkipList { - isSkipped(_location: ResourceLocation): boolean { +export class AlwaysExclude implements ExcludeList { + isExcluded(_location: ResourceLocation): boolean { return true; } } -export function fromManifestAndSkipFile(manifest: AssemblyManifest, skipFile?: string): SkipList { - return new UnionSkipList([new ManifestSkipList(manifest), new SkipFile(skipFile)]); +export function fromManifestAndExclusionList(manifest: AssemblyManifest, exclude?: string[]): ExcludeList { + return new UnionExcludeList([new ManifestExcludeList(manifest), new InMemoryExcludeList(exclude ?? [])]); } + diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 0a1ceb06c..f4329e8b2 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -48,7 +48,7 @@ import type { IIoHost, IoMessageLevel } from '../api/io'; import type { IoHelper } from '../api/io/private'; import { asIoHelper, asSdkLogger, IO, SPAN, withoutColor, withoutEmojis, withTrimmedWhitespace } from '../api/io/private'; import { CloudWatchLogEventMonitor, findCloudWatchLogGroups } from '../api/logs-monitor'; -import { AmbiguityError, ambiguousMovements, findResourceMovements, formatAmbiguousMappings, formatTypedMappings, fromManifestAndSkipFile, resourceMappings } from '../api/refactoring'; +import { AmbiguityError, ambiguousMovements, findResourceMovements, formatAmbiguousMappings, formatTypedMappings, fromManifestAndExclusionList, resourceMappings } from '../api/refactoring'; import { ResourceMigrator } from '../api/resource-import'; import type { AssemblyData, StackDetails, SuccessfulDeployStackResult, ToolkitAction } from '../api/shared-public'; import { PermissionChangeType, PluginHost, ToolkitError } from '../api/shared-public'; @@ -967,7 +967,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { const stacks = await assembly.selectStacksV2(ALL_STACKS); const sdkProvider = await this.sdkProvider('refactor'); - const skipList = fromManifestAndSkipFile(assembly.cloudAssembly.manifest, options.skipFile); + const skipList = fromManifestAndExclusionList(assembly.cloudAssembly.manifest, options.exclude); const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider, skipList); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts index 73d2c65e0..29cb3f741 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts @@ -9,9 +9,9 @@ import type { } from '@aws-sdk/client-cloudformation'; import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; import { expect } from '@jest/globals'; -import type { SkipList } from '../../../lib/api/refactoring'; +import type { ExcludeList } from '../../../lib/api/refactoring'; import { - AlwaysSkipList, + AlwaysExclude, ambiguousMovements, findResourceMovements, resourceMappings, @@ -1253,9 +1253,9 @@ describe('environment grouping', () => { }, ]); - expect(await mappings([stack1, stack2], new AlwaysSkipList())).toEqual([]); + expect(await mappings([stack1, stack2], new AlwaysExclude())).toEqual([]); - async function mappings(stacks: CloudFormationStack[], skipList?: SkipList) { + async function mappings(stacks: CloudFormationStack[], skipList?: ExcludeList) { const provider = new MockSdkProvider(); provider.returnsDefaultAccounts(environment.account); const movements2 = await findResourceMovements(stacks, provider, skipList); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts index 510b94635..1aabca27f 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts @@ -1,16 +1,13 @@ -import * as fs from 'node:fs'; import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema'; import { - ManifestSkipList, - NeverSkipList, - SkipFile, - AlwaysSkipList, - UnionSkipList, + AlwaysExclude, + InMemoryExcludeList, + ManifestExcludeList, + NeverExclude, + UnionExcludeList, } from '../../../lib/api/refactoring'; import type { CloudFormationStack } from '../../../lib/api/refactoring/cloudformation'; -import { - ResourceLocation, -} from '../../../lib/api/refactoring/cloudformation'; +import { ResourceLocation } from '../../../lib/api/refactoring/cloudformation'; const environment = { name: 'prod', @@ -75,11 +72,11 @@ describe('ManifestSkipList', () => { }, }; - const skipList = new ManifestSkipList(manifest as any); + const skipList = new ManifestExcludeList(manifest as any); - expect(skipList.isSkipped(resource1)).toBe(true); - expect(skipList.isSkipped(resource2)).toBe(true); - expect(skipList.isSkipped(resource3)).toBe(false); + expect(skipList.isExcluded(resource1)).toBe(true); + expect(skipList.isExcluded(resource2)).toBe(true); + expect(skipList.isExcluded(resource3)).toBe(false); }); test('nothing is skipped if no SKIP_REFACTOR entries exist', () => { @@ -94,69 +91,41 @@ describe('ManifestSkipList', () => { }, }; - const skipList = new ManifestSkipList(manifest as any); - expect(skipList.isSkipped(resource1)).toBe(false); + const skipList = new ManifestExcludeList(manifest as any); + expect(skipList.isExcluded(resource1)).toBe(false); }); }); -describe('SkipFile', () => { +describe('InMemorySkipList', () => { test('valid resources on a valid list are skipped', () => { - const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify(['Stack1.Resource1', 'Stack2/Resource3']); - jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - - const skipList = new SkipFile(filePath); - expect(skipList.isSkipped(resource1)).toBe(true); - expect(skipList.isSkipped(resource2)).toBe(false); - expect(skipList.isSkipped(resource3)).toBe(true); + const skipList = new InMemoryExcludeList(['Stack1.Resource1', 'Stack2/Resource3']); + expect(skipList.isExcluded(resource1)).toBe(true); + expect(skipList.isExcluded(resource2)).toBe(false); + expect(skipList.isExcluded(resource3)).toBe(true); }); test('nothing is skipped if no file path is provided', () => { - const skipList = new SkipFile(); - expect(skipList.isSkipped(resource1)).toBe(false); - expect(skipList.isSkipped(resource2)).toBe(false); - expect(skipList.isSkipped(resource3)).toBe(false); - }); - - test('throws an error if the content is not an array', () => { - const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify({ spuriousKey: ['Stack1.Resource1', 'Stack2.Resource2'] }); - jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - - expect(() => new SkipFile(filePath)).toThrow('The content of a skip file must be a JSON array of strings'); - }); - - test('throws an error if the content is an array but not of strings', () => { - const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify([1, 2, 3]); - jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - - expect(() => new SkipFile(filePath)).toThrow('The content of a skip file must be a JSON array of strings'); - }); - - test('throws an error if the some entries are not valid resource locations', () => { - const filePath = '/path/to/skip-list.json'; - const fileContent = JSON.stringify(['Stack1.Resource1', 'Invalid-Resource-Location']); - jest.spyOn(fs, 'readFileSync').mockReturnValue(fileContent); - - expect(() => new SkipFile(filePath)).toThrow(/Invalid resource location format: Invalid-Resource-Location/); + const skipList = new InMemoryExcludeList([]); + expect(skipList.isExcluded(resource1)).toBe(false); + expect(skipList.isExcluded(resource2)).toBe(false); + expect(skipList.isExcluded(resource3)).toBe(false); }); }); describe('UnionSkipList', () => { test('skips a resource if at least one underlying list skips', () => { - const skipList1 = new AlwaysSkipList(); - const skipList2 = new NeverSkipList(); + const skipList1 = new AlwaysExclude(); + const skipList2 = new NeverExclude(); - const unionSkipList = new UnionSkipList([skipList1, skipList2]); - expect(unionSkipList.isSkipped(resource1)).toBe(true); + const unionSkipList = new UnionExcludeList([skipList1, skipList2]); + expect(unionSkipList.isExcluded(resource1)).toBe(true); }); test('does not skip a resource if all underlying lists do not skip', () => { - const skipList1 = new NeverSkipList(); - const skipList2 = new NeverSkipList(); + const skipList1 = new NeverExclude(); + const skipList2 = new NeverExclude(); - const unionSkipList = new UnionSkipList([skipList1, skipList2]); - expect(unionSkipList.isSkipped(resource1)).toBe(false); + const unionSkipList = new UnionExcludeList([skipList1, skipList2]); + expect(unionSkipList.isExcluded(resource1)).toBe(false); }); }); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 9fd922445..771a00460 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1211,13 +1211,18 @@ export class CdkToolkit { } public async refactor(options: RefactorOptions): Promise { - const tk = new Toolkit(); + let exclude: string[] = []; + if (options.excludeFile != null) { + if (!(await fs.pathExists(options.excludeFile))) { + throw new ToolkitError(`The exclude file ${options.excludeFile} does not exist`); + } + exclude = fs.readFileSync(options.excludeFile).toString('utf-8').split('\n'); + } - const casmSource = await tk.fromAssemblyBuilder(() => this.assembly()); try { - await tk.refactor(casmSource, { + await this.toolkit.refactor(this.props.cloudExecutable, { dryRun: options.dryRun, - skipFile: options.skipFile, + exclude, stacks: { patterns: options.selector.patterns, strategy: options.selector.patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, @@ -1935,7 +1940,7 @@ export interface RefactorOptions { * - Stack name and logical ID (e.g. `Stack1.MyQueue`) * - A construct path (e.g. `Stack1/Foo/Bar/Resource`). */ - skipFile?: string; + excludeFile?: string; } function buildParameterMap( diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index fb59fd107..88bf966cf 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -411,7 +411,7 @@ export async function makeConfig(): Promise { desc: 'Do not perform any changes, just show what would be done', default: false, }, - 'skip-file': { + 'exclude-file': { type: 'string', requiresArg: true, desc: 'If specified, CDK will use the given file to skip resources during the refactor', diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index f77b06165..5b40b8f5f 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -272,7 +272,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise): any { type: 'boolean', desc: 'Do not perform any changes, just show what would be done', }) - .option('skip-file', { + .option('exclude-file', { default: undefined, type: 'string', requiresArg: true, diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 19184fdc6..6c4fb04aa 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1358,7 +1358,7 @@ export interface RefactorOptions { * * @default - undefined */ - readonly skipFile?: string; + readonly excludeFile?: string; /** * Positional argument for refactor From f8f22133f5f6726f7fb1955c6526ec5ee8a3fd02 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 2 May 2025 12:57:12 +0100 Subject: [PATCH 14/16] SKIP_REFACTOR -> DO_NOT_REFACTOR --- .../lib/cloud-assembly/metadata-schema.ts | 4 ++-- .../@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts | 8 ++++---- .../lib/api/refactoring/{skip.ts => exclude.ts} | 2 +- .../@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts | 8 ++++---- packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts | 4 ++-- .../{skip-refactor => exclude-refactor}/index.ts | 2 +- .../@aws-cdk/toolkit-lib/test/actions/refactor.test.ts | 6 +++--- .../api/refactoring/{skip.test.ts => exclude.test.ts} | 8 ++++---- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 2 +- 9 files changed, 22 insertions(+), 22 deletions(-) rename packages/@aws-cdk/toolkit-lib/lib/api/refactoring/{skip.ts => exclude.ts} (98%) rename packages/@aws-cdk/toolkit-lib/test/_fixtures/{skip-refactor => exclude-refactor}/index.ts (78%) rename packages/@aws-cdk/toolkit-lib/test/api/refactoring/{skip.test.ts => exclude.test.ts} (92%) diff --git a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts index 2b7b0a769..f2b6affb7 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts +++ b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts @@ -314,9 +314,9 @@ export enum ArtifactMetadataEntryType { STACK_TAGS = 'aws:cdk:stack-tags', /** - * Whether the resource should be skipped during refactoring. + * Whether the resource should be excluded during refactoring. */ - SKIP_REFACTOR = 'aws:cdk:skip-refactor', + DO_NOT_REFACTOR = 'aws:cdk:do-not-refactor', } /** diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts index fad93c3e1..8fc9f06fe 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -16,10 +16,10 @@ export interface RefactorOptions { stacks?: StackSelector; /** - * A list of resources to skip during the refactor. Elements of - * this list should be _destination_ locations that should be skipped, - * i.e., the location to which a resource would be moved if the - * refactor were to happen. + * A list of resources that will not be part of the refactor. + * Elements of this list must be the _destination_ locations + * that should be excluded, i.e., the location to which a + * resource would be moved if the refactor were to happen. * * The format of the locations in the file can be either: * diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts similarity index 98% rename from packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts rename to packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts index eadf493c7..9473d9e6f 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts @@ -25,7 +25,7 @@ export class ManifestExcludeList implements ExcludeList { const locations = Object.values(manifest.metadata ?? {}) // Then pick only the resources in each stack marked with SKIP_REFACTOR .filter((entries) => - entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true), + entries.some((entry) => entry.type === ArtifactMetadataEntryType.DO_NOT_REFACTOR && entry.data === true), ) // Finally, get the logical ID of each resource .map((entries) => { diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts index 118380c6b..98e2b15ea 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts @@ -12,9 +12,9 @@ import { StringWriteStream } from '../streams'; import type { CloudFormationStack } from './cloudformation'; import { ResourceMapping, ResourceLocation } from './cloudformation'; import { computeResourceDigests, hashObject } from './digest'; -import { NeverExclude, type ExcludeList } from './skip'; +import { NeverExclude, type ExcludeList } from './exclude'; -export * from './skip'; +export * from './exclude'; /** * Represents a set of possible movements of a resource from one location @@ -152,7 +152,7 @@ function resourceDigests(stack: CloudFormationStack): [string, ResourceLocation] export async function findResourceMovements( stacks: CloudFormationStack[], sdkProvider: SdkProvider, - skipList: ExcludeList = new NeverExclude(), + exclude: ExcludeList = new NeverExclude(), ): Promise { const stackGroups: Map = new Map(); @@ -176,7 +176,7 @@ export async function findResourceMovements( return result.filter(mov => { const after = mov[1]; - return after.every(l => !skipList.isExcluded(l)); + return after.every(l => !exclude.isExcluded(l)); }); } diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index f4329e8b2..b656b6136 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -967,8 +967,8 @@ export class Toolkit extends CloudAssemblySourceBuilder { const stacks = await assembly.selectStacksV2(ALL_STACKS); const sdkProvider = await this.sdkProvider('refactor'); - const skipList = fromManifestAndExclusionList(assembly.cloudAssembly.manifest, options.exclude); - const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider, skipList); + const exclude = fromManifestAndExclusionList(assembly.cloudAssembly.manifest, options.exclude); + const movements = await findResourceMovements(stacks.stackArtifacts, sdkProvider, exclude); const ambiguous = ambiguousMovements(movements); if (ambiguous.length === 0) { const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); diff --git a/packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts b/packages/@aws-cdk/toolkit-lib/test/_fixtures/exclude-refactor/index.ts similarity index 78% rename from packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts rename to packages/@aws-cdk/toolkit-lib/test/_fixtures/exclude-refactor/index.ts index b4a9e80a8..6dbed6915 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_fixtures/skip-refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_fixtures/exclude-refactor/index.ts @@ -5,6 +5,6 @@ export default async () => { const app = new core.App({ autoSynth: false }); const stack = new core.Stack(app, 'Stack1'); const bucket = new s3.Bucket(stack, 'MyBucket'); - bucket.node.defaultChild?.node.addMetadata('aws:cdk:skip-refactor', true); + bucket.node.defaultChild?.node.addMetadata('aws:cdk:do-not-refactor', true); return app.synth(); }; diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts index cc10b53b8..bf50d87bf 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -248,7 +248,7 @@ test('filters stacks when stack selector is passed', async () => { ); }); -test('resource is marked to be skipped for refactoring in the cloud assembly', async () => { +test('resource is marked to be excluded for refactoring in the cloud assembly', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -269,7 +269,7 @@ test('resource is marked to be skipped for refactoring in the cloud assembly', a TemplateBody: JSON.stringify({ Resources: { // This would have caused a refactor to be detected, - // but the resource is marked to be skipped... + // but the resource is marked to be excluded... OldLogicalID: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', @@ -283,7 +283,7 @@ test('resource is marked to be skipped for refactoring in the cloud assembly', a }); // WHEN - const cx = await builderFixture(toolkit, 'skip-refactor'); + const cx = await builderFixture(toolkit, 'exclude-refactor'); await toolkit.refactor(cx, { dryRun: true, }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts similarity index 92% rename from packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts rename to packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts index 1aabca27f..c9a858e5b 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/skip.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts @@ -40,14 +40,14 @@ const resource2 = new ResourceLocation(stack2, 'Resource2'); const resource3 = new ResourceLocation(stack2, 'Resource3'); describe('ManifestSkipList', () => { - test('locations marked with SKIP_REFACTOR in the manifest are skipped', () => { + test('locations marked with DO_NOT_REFACTOR in the manifest are skipped', () => { const manifest = { artifacts: { 'Stack1': { type: ArtifactType.AWS_CLOUDFORMATION_STACK, metadata: { LogicalId1: [ - { type: ArtifactMetadataEntryType.SKIP_REFACTOR, data: true }, + { type: ArtifactMetadataEntryType.DO_NOT_REFACTOR, data: true }, { type: ArtifactMetadataEntryType.LOGICAL_ID, data: 'Resource1' }, ], }, @@ -56,7 +56,7 @@ describe('ManifestSkipList', () => { type: ArtifactType.AWS_CLOUDFORMATION_STACK, metadata: { LogicalId2: [ - { type: ArtifactMetadataEntryType.SKIP_REFACTOR, data: true }, + { type: ArtifactMetadataEntryType.DO_NOT_REFACTOR, data: true }, { type: ArtifactMetadataEntryType.LOGICAL_ID, data: 'Resource2' }, ], }, @@ -79,7 +79,7 @@ describe('ManifestSkipList', () => { expect(skipList.isExcluded(resource3)).toBe(false); }); - test('nothing is skipped if no SKIP_REFACTOR entries exist', () => { + test('nothing is skipped if no DO_NOT_REFACTOR entries exist', () => { const manifest = { artifacts: { Stack1: { diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 771a00460..165a60a1e 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1214,7 +1214,7 @@ export class CdkToolkit { let exclude: string[] = []; if (options.excludeFile != null) { if (!(await fs.pathExists(options.excludeFile))) { - throw new ToolkitError(`The exclude file ${options.excludeFile} does not exist`); + throw new ToolkitError(`The exclude file '${options.excludeFile}' does not exist`); } exclude = fs.readFileSync(options.excludeFile).toString('utf-8').split('\n'); } From d4c6dc85aae45fd788a24edbb4732b3f4804745d Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 2 May 2025 13:45:05 +0100 Subject: [PATCH 15/16] More skip -> exclude --- .../lib/api/refactoring/exclude.ts | 30 ++++----- .../test/api/refactoring/exclude.test.ts | 62 +++++++++---------- .../test/api/refactoring/refactoring.test.ts | 4 +- packages/aws-cdk/README.md | 23 ++++--- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 10 +-- packages/aws-cdk/lib/cli/cli-config.ts | 2 +- 6 files changed, 65 insertions(+), 66 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts index 9473d9e6f..72a88859d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts @@ -8,13 +8,13 @@ export interface ExcludeList { } export class ManifestExcludeList implements ExcludeList { - private readonly skippedLocations: CfnResourceLocation[]; + private readonly excludedLocations: CfnResourceLocation[]; constructor(manifest: AssemblyManifest) { - this.skippedLocations = this.getSkippedLocations(manifest); + this.excludedLocations = this.getExcludedLocations(manifest); } - private getSkippedLocations(asmManifest: AssemblyManifest): CfnResourceLocation[] { + private getExcludedLocations(asmManifest: AssemblyManifest): CfnResourceLocation[] { // First, we need to filter the artifacts to only include CloudFormation stacks const stackManifests = Object.entries(asmManifest.artifacts ?? {}).filter( ([_, manifest]) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK, @@ -23,7 +23,7 @@ export class ManifestExcludeList implements ExcludeList { const result: CfnResourceLocation[] = []; for (let [stackName, manifest] of stackManifests) { const locations = Object.values(manifest.metadata ?? {}) - // Then pick only the resources in each stack marked with SKIP_REFACTOR + // Then pick only the resources in each stack marked with DO_NOT_REFACTOR .filter((entries) => entries.some((entry) => entry.type === ArtifactMetadataEntryType.DO_NOT_REFACTOR && entry.data === true), ) @@ -42,19 +42,19 @@ export class ManifestExcludeList implements ExcludeList { } isExcluded(location: ResourceLocation): boolean { - return this.skippedLocations.some( + return this.excludedLocations.some( (loc) => loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId, ); } } export class InMemoryExcludeList implements ExcludeList { - private readonly skippedLocations: CfnResourceLocation[]; - private readonly skippedPaths: string[]; + private readonly excludedLocations: CfnResourceLocation[]; + private readonly excludedPaths: string[]; constructor(items: string[]) { - this.skippedLocations = []; - this.skippedPaths = []; + this.excludedLocations = []; + this.excludedPaths = []; if (items.length === 0) { return; @@ -65,32 +65,32 @@ export class InMemoryExcludeList implements ExcludeList { items.forEach((item: string) => { if (locationRegex.test(item)) { const [stackName, logicalId] = item.split('.'); - this.skippedLocations.push({ + this.excludedLocations.push({ StackName: stackName, LogicalResourceId: logicalId, }); } else { - this.skippedPaths.push(item); + this.excludedPaths.push(item); } }); } isExcluded(location: ResourceLocation): boolean { - const containsLocation = this.skippedLocations.some((loc) => { + const containsLocation = this.excludedLocations.some((loc) => { return loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId; }); - const containsPath = this.skippedPaths.some((path) => location.toPath() === path); + const containsPath = this.excludedPaths.some((path) => location.toPath() === path); return containsLocation || containsPath; } } export class UnionExcludeList implements ExcludeList { - constructor(private readonly skipLists: ExcludeList[]) { + constructor(private readonly excludeLists: ExcludeList[]) { } isExcluded(location: ResourceLocation): boolean { - return this.skipLists.some((skipList) => skipList.isExcluded(location)); + return this.excludeLists.some((excludeList) => excludeList.isExcluded(location)); } } diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts index c9a858e5b..b658ebc80 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/exclude.test.ts @@ -39,8 +39,8 @@ const resource1 = new ResourceLocation(stack1, 'Resource1'); const resource2 = new ResourceLocation(stack2, 'Resource2'); const resource3 = new ResourceLocation(stack2, 'Resource3'); -describe('ManifestSkipList', () => { - test('locations marked with DO_NOT_REFACTOR in the manifest are skipped', () => { +describe('ManifestExcludeList', () => { + test('locations marked with DO_NOT_REFACTOR in the manifest are excluded', () => { const manifest = { artifacts: { 'Stack1': { @@ -72,14 +72,14 @@ describe('ManifestSkipList', () => { }, }; - const skipList = new ManifestExcludeList(manifest as any); + const excludeList = new ManifestExcludeList(manifest as any); - expect(skipList.isExcluded(resource1)).toBe(true); - expect(skipList.isExcluded(resource2)).toBe(true); - expect(skipList.isExcluded(resource3)).toBe(false); + expect(excludeList.isExcluded(resource1)).toBe(true); + expect(excludeList.isExcluded(resource2)).toBe(true); + expect(excludeList.isExcluded(resource3)).toBe(false); }); - test('nothing is skipped if no DO_NOT_REFACTOR entries exist', () => { + test('nothing is excluded if no DO_NOT_REFACTOR entries exist', () => { const manifest = { artifacts: { Stack1: { @@ -91,41 +91,41 @@ describe('ManifestSkipList', () => { }, }; - const skipList = new ManifestExcludeList(manifest as any); - expect(skipList.isExcluded(resource1)).toBe(false); + const excludeList = new ManifestExcludeList(manifest as any); + expect(excludeList.isExcluded(resource1)).toBe(false); }); }); -describe('InMemorySkipList', () => { - test('valid resources on a valid list are skipped', () => { - const skipList = new InMemoryExcludeList(['Stack1.Resource1', 'Stack2/Resource3']); - expect(skipList.isExcluded(resource1)).toBe(true); - expect(skipList.isExcluded(resource2)).toBe(false); - expect(skipList.isExcluded(resource3)).toBe(true); +describe('InMemoryexcludeList', () => { + test('valid resources on a valid list are excluded', () => { + const excludeList = new InMemoryExcludeList(['Stack1.Resource1', 'Stack2/Resource3']); + expect(excludeList.isExcluded(resource1)).toBe(true); + expect(excludeList.isExcluded(resource2)).toBe(false); + expect(excludeList.isExcluded(resource3)).toBe(true); }); - test('nothing is skipped if no file path is provided', () => { - const skipList = new InMemoryExcludeList([]); - expect(skipList.isExcluded(resource1)).toBe(false); - expect(skipList.isExcluded(resource2)).toBe(false); - expect(skipList.isExcluded(resource3)).toBe(false); + test('nothing is excluded if no file path is provided', () => { + const excludeList = new InMemoryExcludeList([]); + expect(excludeList.isExcluded(resource1)).toBe(false); + expect(excludeList.isExcluded(resource2)).toBe(false); + expect(excludeList.isExcluded(resource3)).toBe(false); }); }); -describe('UnionSkipList', () => { - test('skips a resource if at least one underlying list skips', () => { - const skipList1 = new AlwaysExclude(); - const skipList2 = new NeverExclude(); +describe('UnionexcludeList', () => { + test('excludes a resource if at least one underlying list excludes', () => { + const excludeList1 = new AlwaysExclude(); + const excludeList2 = new NeverExclude(); - const unionSkipList = new UnionExcludeList([skipList1, skipList2]); - expect(unionSkipList.isExcluded(resource1)).toBe(true); + const unionexcludeList = new UnionExcludeList([excludeList1, excludeList2]); + expect(unionexcludeList.isExcluded(resource1)).toBe(true); }); - test('does not skip a resource if all underlying lists do not skip', () => { - const skipList1 = new NeverExclude(); - const skipList2 = new NeverExclude(); + test('does not exclude a resource if all underlying lists do not exclude', () => { + const excludeList1 = new NeverExclude(); + const excludeList2 = new NeverExclude(); - const unionSkipList = new UnionExcludeList([skipList1, skipList2]); - expect(unionSkipList.isExcluded(resource1)).toBe(false); + const unionExcludeList = new UnionExcludeList([excludeList1, excludeList2]); + expect(unionExcludeList.isExcluded(resource1)).toBe(false); }); }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts index 29cb3f741..899c4ecfa 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts @@ -1255,10 +1255,10 @@ describe('environment grouping', () => { expect(await mappings([stack1, stack2], new AlwaysExclude())).toEqual([]); - async function mappings(stacks: CloudFormationStack[], skipList?: ExcludeList) { + async function mappings(stacks: CloudFormationStack[], excludeList?: ExcludeList) { const provider = new MockSdkProvider(); provider.returnsDefaultAccounts(environment.account); - const movements2 = await findResourceMovements(stacks, provider, skipList); + const movements2 = await findResourceMovements(stacks, provider, excludeList); return resourceMappings(movements2).map(toCfnMapping); } }); diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 2d517366b..07b70dfce 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -1099,24 +1099,23 @@ show this table and exit. Eventually, the CLI will also be able to automatically apply the refactor on your CloudFormation stacks. But for now, only the dry-run mode is supported. -If you want to exclude some resources from the refactor, you can pass a skip -file, containing a list of destination locations to skip, which can be -either the stack name + logical ID, or the construct path. For example, if -you don't want to include the bucket and the distribution from the table -above in the refactor, you can create a file called `skip.json` with the -following content: +If you want to exclude some resources from the refactor, you can pass an +exclude file, containing a list of destination locations to exclude. A +location can be either the stack name + logical ID, or the construct path. For +example, if you don't want to include the bucket and the distribution from +the table above in the refactor, you can create a file called +`exclude.txt`with the following content: -```json -[ - "Web/Website/Origin/Resource", - "Web/Website/Distribution/Resource" +``` +Web/Website/Origin/Resource +Web/Website/Distribution/Resource ] ``` -and pass it to the CLI via the `--skip-file` flag: +and pass it to the CLI via the `--exclude-file` flag: ```shell -$ cdk refactor --skip-file skip.json --unstable=refactor --dry-run +$ cdk refactor --exclude-file exclude.txt --unstable=refactor --dry-run ``` If your application has more than one stack, and you want the refactor diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 165a60a1e..ee2f9ffce 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1929,11 +1929,11 @@ export interface RefactorOptions { selector: StackSelector; /** - * The absolute path to a file that contains a list of resources to - * skip during the refactor. The file should be in JSON format and - * contain an array of _destination_ locations that should be skipped, - * i.e., the location to which a resource would be moved if the - * refactor were to happen. + * The absolute path to a file that contains a list of resources that + * should be excluded during the refactor. This file should contain a + * newline separated list of _destination_ locations to exclude, i.e., + * the location to which a resource would be moved if the refactor + * were to happen. * * The format of the locations in the file can be either: * diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index 88bf966cf..0ff41c2d4 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -414,7 +414,7 @@ export async function makeConfig(): Promise { 'exclude-file': { type: 'string', requiresArg: true, - desc: 'If specified, CDK will use the given file to skip resources during the refactor', + desc: 'If specified, CDK will use the given file to exclude resources from the refactor', }, }, }, From 45ddacca82c10ec475426a43fdc8e7e45ea4263f Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 2 May 2025 13:48:56 +0100 Subject: [PATCH 16/16] More skip -> exclude --- packages/aws-cdk/lib/cli/parse-command-line-arguments.ts | 2 +- packages/aws-cdk/lib/cli/user-input.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts index f24d7b31d..0e231425f 100644 --- a/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/cli/parse-command-line-arguments.ts @@ -851,7 +851,7 @@ export function parseCommandLineArguments(args: Array): any { default: undefined, type: 'string', requiresArg: true, - desc: 'If specified, CDK will use the given file to skip resources during the refactor', + desc: 'If specified, CDK will use the given file to exclude resources from the refactor', }), ) .version(helpers.cliVersion()) diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 6c4fb04aa..5480e9e04 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1354,7 +1354,7 @@ export interface RefactorOptions { readonly dryRun?: boolean; /** - * If specified, CDK will use the given file to skip resources during the refactor + * If specified, CDK will use the given file to exclude resources from the refactor * * @default - undefined */