feat(refactor): support for stack filtering (#410)

Users can now pass the stacks they want to be considered for
refactoring:

    cdk refactor StackA StackB ...

The result will include any mapping that contains at least one of the
selected stacks. If no pattern is passed, all stacks are considered.

In this PR I also included the `refactor` command in the command line
parsing list, which was missing. It was already working without it, but
the command wouldn't show up in the help page, and it wouldn't recognize
positional arguments. But the command itself (and its options) could be
already used.

Closes #363.

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

---------

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>

Author: 3 people

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(

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,
       }));

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,20 +182,37 @@ 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',
             },
           },
         },
       }),
     });
 
   // 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/),
     }),
   );
 });

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);

packages/aws-cdk/lib/cli/cli-config.ts

@@ -399,6 +399,20 @@ export async function makeConfig(): Promise<CliConfig> {
       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,
+          },
+        },
+      },
     },
   };
 }

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;

packages/aws-cdk/lib/cli/parse-command-line-arguments.ts

@@ -840,6 +840,13 @@ export function parseCommandLineArguments(args: Array<string>): 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()

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 = [

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<string>;
+}