From 225591b1b57f38738aa07eb263733618153e9ab2 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 27 Jun 2025 14:02:59 +0100 Subject: [PATCH 01/13] feat!: modification is forbidden during a refactor --- .../cloudformation-diff/lib/mappings.ts | 30 +- .../toolkit-lib/docs/message-registry.md | 1 + .../toolkit-lib/lib/actions/refactor/index.ts | 22 +- .../lib/api/io/private/messages.ts | 6 + .../lib/api/refactoring/context.ts | 125 ++++---- .../toolkit-lib/lib/api/refactoring/digest.ts | 60 +--- .../toolkit-lib/lib/api/refactoring/index.ts | 104 ++++-- .../toolkit-lib/lib/toolkit/toolkit.ts | 109 +++---- .../_fixtures/stack-with-two-buckets/index.ts | 10 + .../toolkit-lib/test/actions/refactor.test.ts | 298 ++++++++++++++++-- .../test/api/refactoring/context.test.ts | 208 ++---------- .../test/api/refactoring/refactoring.test.ts | 130 -------- packages/aws-cdk/README.md | 18 +- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 27 +- packages/aws-cdk/lib/cli/cli-config.ts | 12 +- packages/aws-cdk/lib/cli/cli.ts | 3 +- .../aws-cdk/lib/cli/convert-to-user-input.ts | 5 +- .../lib/cli/parse-command-line-arguments.ts | 14 +- packages/aws-cdk/lib/cli/user-input.ts | 19 +- 19 files changed, 615 insertions(+), 586 deletions(-) create mode 100644 packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts diff --git a/packages/@aws-cdk/cloudformation-diff/lib/mappings.ts b/packages/@aws-cdk/cloudformation-diff/lib/mappings.ts index feda7f371..498590fc3 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/mappings.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/mappings.ts @@ -8,41 +8,33 @@ export interface TypedMapping { readonly destinationPath: string; } -export function formatMappingsHeader(stream: NodeJS.WritableStream) { +export function formatEnvironmentSectionHeader(stream: NodeJS.WritableStream, env: string) { const formatter = new Formatter(stream, {}); - formatter.printSectionHeader('The following resources were moved or renamed:\n'); + formatter.printSectionHeader(`${env}\n`); } -export function formatTypedMappings(stream: NodeJS.WritableStream, mappings: TypedMapping[], env: string) { - const header = [['Resource Type', 'Old Construct Path', 'New Construct Path']]; - const rows = mappings.map((m) => [m.type, m.sourcePath, m.destinationPath]); - - const formatter = new Formatter(stream, {}); - formatter.print(`${env}:`); +export function formatTypedMappings(stream: NodeJS.WritableStream, mappings: TypedMapping[]) { if (mappings.length > 0) { + const header = [['Resource Type', 'Old Construct Path', 'New Construct Path']]; + const rows = mappings.map((m) => [m.type, m.sourcePath, m.destinationPath]); + const formatter = new Formatter(stream, {}); + + formatter.print('The following resources were moved or renamed:'); formatter.print(chalk.green(formatTable(header.concat(rows), undefined))); - } else { - formatter.print('Nothing to refactor.'); + formatter.print(' '); } - formatter.print(' '); -} - -export function formatAmbiguitySectionHeader(stream: NodeJS.WritableStream) { - const formatter = new Formatter(stream, {}); - formatter.printSectionHeader('Ambiguous Resource Name Changes:\n'); } export function formatAmbiguousMappings( stream: NodeJS.WritableStream, pairs: [string[], string[]][], - env: string, ) { const tables = pairs.map(renderTable); const formatter = new Formatter(stream, {}); - formatter.print(`${env}:`); + formatter.print('Detected ambiguities:'); formatter.print(tables.join('\n\n')); - formatter.printSectionFooter(); + formatter.print(' '); function renderTable([removed, added]: [string[], string[]]) { return formatTable([['', 'Resource'], renderRemoval(removed), renderAddition(added)], undefined); diff --git a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md index 66f342305..faf35634d 100644 --- a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md +++ b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md @@ -125,6 +125,7 @@ Please let us know by [opening an issue](https://github.com/aws/aws-cdk-cli/issu | `CDK_TOOLKIT_I7900` | Stack deletion succeeded | `result` | [cxapi.CloudFormationStackArtifact](https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_cx-api.CloudFormationStackArtifact.html) | | `CDK_TOOLKIT_E7010` | Action was aborted due to negative confirmation of request | `error` | n/a | | `CDK_TOOLKIT_E7900` | Stack deletion failed | `error` | {@link ErrorPayload} | +| `CDK_TOOLKIT_E8900` | Stack refactor failed | `error` | {@link ErrorPayload} | | `CDK_TOOLKIT_I8900` | Refactor result | `result` | {@link RefactorResult} | | `CDK_TOOLKIT_W8010` | Refactor execution not yet supported | `warn` | n/a | | `CDK_TOOLKIT_I9000` | Provides bootstrap times | `info` | {@link Duration} | 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 743ed0618..4080c3959 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -1,4 +1,3 @@ -import type { StackSelector } from '../../api/cloud-assembly'; import type { ExcludeList } from '../../api/refactoring'; import { InMemoryExcludeList, NeverExclude } from '../../api/refactoring'; @@ -71,16 +70,25 @@ export interface RefactorOptions { readonly dryRun?: boolean; /** - * Criteria for selecting stacks to deploy - * - * @default - All stacks + * How the toolkit should obtain the mappings */ - stacks?: StackSelector; + mappingSource?: MappingSource; /** - * How the toolkit should obtain the mappings + * List of patterns for filtering local stacks. If no patterns are passed, + * then all stacks, except the bootstrap stacks are considered. If you want + * to consider all stacks (including bootstrap stacks), pass the wildcard + * '*'. */ - mappingSource?: MappingSource; + localStacks?: string[]; + + /** + * List of patterns for filtering deployed stacks. If no patterns are passed, + * then all stacks, except the bootstrap stacks are considered. If you want + * to consider all stacks (including bootstrap stacks), pass the wildcard + * '*'. + */ + deployedStacks?: string[]; } export interface MappingGroup { diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts index 105620e1f..9314e8530 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts @@ -369,6 +369,12 @@ export const IO = { }), // 8. Refactor (8xxx) + CDK_TOOLKIT_E8900: make.error({ + code: 'CDK_TOOLKIT_E8900', + description: 'Stack refactor failed', + interface: 'ErrorPayload', + }), + CDK_TOOLKIT_I8900: make.result({ code: 'CDK_TOOLKIT_I8900', description: 'Refactor result', diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index 963df434b..0de295785 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -16,7 +16,6 @@ export interface RefactorManagerOptions { localStacks: CloudFormationStack[]; deployedStacks: CloudFormationStack[]; mappings?: ResourceMapping[]; - filteredStacks?: CloudFormationStack[]; } /** @@ -33,18 +32,12 @@ export class RefactoringContext { this._mappings = props.mappings; } else { const moves = resourceMoves(props.deployedStacks, props.localStacks); - this.ambiguousMoves = ambiguousMoves(moves); - - if (!this.isAmbiguous) { - this._mappings = resourceMappings(resourceMoves(props.deployedStacks, props.localStacks), props.filteredStacks); - } + this.ambiguousMoves = moves.filter(isAmbiguousMove); + const nonAmbiguousMoves = moves.filter((move) => !isAmbiguousMove(move)); + this._mappings = resourceMappings(nonAmbiguousMoves); } } - public get isAmbiguous(): boolean { - return this.ambiguousMoves.length > 0; - } - public get ambiguousPaths(): [string[], string[]][] { return this.ambiguousMoves.map(([a, b]) => [convert(a), convert(b)]); @@ -54,24 +47,36 @@ export class RefactoringContext { } public get mappings(): ResourceMapping[] { - if (this.isAmbiguous) { - throw new ToolkitError( - 'Cannot access mappings when there are ambiguous resource moves. Please resolve the ambiguity first.', - ); - } return this._mappings; } } function resourceMoves(before: CloudFormationStack[], after: CloudFormationStack[]): ResourceMove[] { - return Object.values( - removeUnmovedResources(zip(groupByKey(resourceDigests(before)), groupByKey(resourceDigests(after)))), - ); + const digestsBefore = resourceDigests(before); + const digestsAfter = resourceDigests(after); + + if (!isomorphic(digestsBefore, digestsAfter)) { + const message = 'A refactor operation cannot add, remove or update resources. ' + + 'Only resource moves and renames are allowed. ' + + "Run 'cdk diff' to compare the local templates to the deployed stacks."; + throw new ToolkitError(message); + } + + return Object.values(removeUnmovedResources(zip(digestsBefore, digestsAfter))); } -function removeUnmovedResources(m: Record): Record { +/** + * Whether two sets of resources have the same elements (uniquely identified by the digest), and + * each element is in the same number of locations. The locations themselves may be different. + */ +function isomorphic(a: Record, b: Record): boolean { + const sameKeys = equalSets(new Set(Object.keys(a)), new Set(Object.keys(b))); + return sameKeys && Object.entries(a).every(([digest, locations]) => locations.length === b[digest].length); +} + +function removeUnmovedResources(moves: Record): Record { const result: Record = {}; - for (const [hash, [before, after]] of Object.entries(m)) { + for (const [hash, [before, after]] of Object.entries(moves)) { const common = before.filter((b) => after.some((a) => a.equalTo(b))); result[hash] = [ before.filter((b) => !common.some((c) => b.equalTo(c))), @@ -109,24 +114,10 @@ function zip( return result; } -function groupByKey(entries: [string, A][]): Record { - const result: Record = {}; - - for (const [hash, location] of entries) { - if (hash in result) { - result[hash].push(location); - } else { - result[hash] = [location]; - } - } - - return result; -} - /** * Computes a list of pairs [digest, location] for each resource in the stack. */ -function resourceDigests(stacks: CloudFormationStack[]): [string, ResourceLocation][] { +function resourceDigests(stacks: CloudFormationStack[]): Record { // index stacks by name const stacksByName = new Map(); for (const stack of stacks) { @@ -135,35 +126,55 @@ function resourceDigests(stacks: CloudFormationStack[]): [string, ResourceLocati const digests = computeResourceDigests(stacks); - return Object.entries(digests).map(([loc, digest]) => { - const [stackName, logicalId] = loc.split('.'); - const location: ResourceLocation = new ResourceLocation(stacksByName.get(stackName)!, logicalId); - return [digest, location]; - }); + return groupByKey( + Object.entries(digests).map(([loc, digest]) => { + const [stackName, logicalId] = loc.split('.'); + const location: ResourceLocation = new ResourceLocation(stacksByName.get(stackName)!, logicalId); + return [digest, location]; + }), + ); + + function groupByKey(entries: [string, A][]): Record { + const result: Record = {}; + + for (const [key, value] of entries) { + if (key in result) { + result[key].push(value); + } else { + result[key] = [value]; + } + } + + return result; + } } -function ambiguousMoves(movements: ResourceMove[]) { +function isAmbiguousMove(move: ResourceMove): boolean { + const [pre, post] = move; + // A move is considered ambiguous if two conditions are met: // 1. Both sides have at least one element (otherwise, it's just addition or deletion) // 2. At least one side has more than one element - return movements - .filter(([pre, post]) => pre.length > 0 && post.length > 0) - .filter(([pre, post]) => pre.length > 1 || post.length > 1); + return pre.length > 0 && post.length > 0 && (pre.length > 1 || post.length > 1); } -function resourceMappings(movements: ResourceMove[], stacks?: CloudFormationStack[]): 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)); - }; - +function resourceMappings(movements: ResourceMove[]): ResourceMapping[] { 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); + .map(([pre, post]) => new ResourceMapping(pre[0], post[0])); +} + +/** + * Are two sets equal to each other + */ +function equalSets(a: Set, b: Set) { + if (a.size !== b.size) { + return false; + } + for (const x of a) { + if (!b.has(x)) { + return false; + } + } + return true; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts index fe8072561..966596b18 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts @@ -1,5 +1,4 @@ import * as crypto from 'node:crypto'; -import { loadResourceModel } from '@aws-cdk/cloudformation-diff/lib/diff/util'; import type { CloudFormationResource, CloudFormationStack } from './cloudformation'; import { ResourceGraph } from './graph'; @@ -8,18 +7,12 @@ import { ResourceGraph } from './graph'; * * Conceptually, the digest is computed as: * - * d(resource) = hash(type + physicalId) , if physicalId is defined - * = hash(type + properties + dependencies.map(d)) , otherwise + * digest(resource) = hash(type + properties + dependencies.map(d)) * - * where `hash` is a cryptographic hash function. In other words, if a resource has - * a physical ID, we use the physical ID plus its type to uniquely identify - * that resource. In this case, the digest can be computed from these two fields - * alone. A corollary is that such resources can be renamed and have their - * properties updated at the same time, and still be considered equivalent. - * - * Otherwise, the digest is computed from its type, its own properties (that is, - * excluding properties that refer to other resources), and the digests of each of - * its dependencies. + * where `hash` is a cryptographic hash function. In other words, the digest of a + * resource is computed from its type, its own properties (that is, excluding + * properties that refer to other resources), and the digests of each of its + * dependencies. * * The digest of a resource, defined recursively this way, remains stable even if * one or more of its dependencies gets renamed. Since the resources in a @@ -36,41 +29,24 @@ export function computeResourceDigests(stacks: CloudFormationStack[]): Record - Object.entries(s.template.Resources ?? {}).map( - ([id, res]) => [`${s.stackName}.${id}`, res] as [string, CloudFormationResource], - ), - ), + stacks.flatMap((s) => { + return Object.entries(s.template.Resources ?? {}) + .filter(([_, res]) => res.Type !== 'AWS::CDK::Metadata') + .map( + ([id, res]) => [`${s.stackName}.${id}`, res] as [string, CloudFormationResource], + ); + }), ); const graph = new ResourceGraph(stacks); - const nodes = graph.sortedNodes; + const nodes = graph.sortedNodes.filter(n => resources[n] != null); // 4. Compute digests in sorted order const result: Record = {}; for (const id of nodes) { const resource = resources[id]; - const resourceProperties = resource.Properties ?? {}; - const model = loadResourceModel(resource.Type); - const identifier = intersection(Object.keys(resourceProperties), model?.primaryIdentifier ?? []); - let toHash: string; - - if (identifier.length === model?.primaryIdentifier?.length) { - // The resource has a physical ID defined, so we can - // use the ID and the type as the identity of the resource. - toHash = - resource.Type + - identifier - .sort() - .map((attr) => JSON.stringify(resourceProperties[attr])) - .join(''); - } else { - // The resource does not have a physical ID defined, so we need to - // compute the digest based on its properties and dependencies. - const depDigests = Array.from(graph.outNeighbors(id)).map((d) => result[d]); - const propertiesHash = hashObject(stripReferences(stripConstructPath(resource), exports)); - toHash = resource.Type + propertiesHash + depDigests.join(''); - } - + const depDigests = Array.from(graph.outNeighbors(id)).map((d) => result[d]); + const propertiesHash = hashObject(stripReferences(stripConstructPath(resource), exports)); + const toHash = resource.Type + propertiesHash + depDigests.join(''); result[id] = crypto.createHash('sha256').update(toHash).digest('hex'); } @@ -146,7 +122,3 @@ function stripConstructPath(resource: any): any { delete copy.Metadata['aws:cdk:path']; return copy; } - -function intersection(a: T[], b: T[]): T[] { - return a.filter((value) => b.includes(value)); -} 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 179eb8aed..792e22096 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts @@ -1,32 +1,40 @@ import type { TypedMapping } from '@aws-cdk/cloudformation-diff'; import { formatAmbiguousMappings as fmtAmbiguousMappings, - formatMappingsHeader as fmtMappingsHeader, + formatEnvironmentSectionHeader as fmtEnvironmentSectionHeader, formatTypedMappings as fmtTypedMappings, - formatAmbiguitySectionHeader as fmtAmbiguitySectionHeader, } from '@aws-cdk/cloudformation-diff'; import type * as cxapi from '@aws-cdk/cx-api'; import type { StackSummary } from '@aws-sdk/client-cloudformation'; +import { minimatch } from 'minimatch'; +import { major } from 'semver'; import { deserializeStructure } from '../../util'; import type { SdkProvider } from '../aws-auth/private'; import { Mode } from '../plugin'; import { StringWriteStream } from '../streams'; import type { CloudFormationStack } from './cloudformation'; import { ResourceLocation, ResourceMapping } from './cloudformation'; +import { hashObject } from './digest'; import type { MappingGroup } from '../../actions'; import { ToolkitError } from '../../toolkit/toolkit-error'; export * from './exclude'; +interface StackGroup { + environment: cxapi.Environment; + localStacks: CloudFormationStack[]; + deployedStacks: CloudFormationStack[]; +} + export async function usePrescribedMappings( mappingGroups: MappingGroup[], sdkProvider: SdkProvider, ): Promise { - interface StackGroup extends MappingGroup { + interface MappingGroupWithStacks extends MappingGroup { stacks: CloudFormationStack[]; } - const stackGroups: StackGroup[] = []; + const stackGroups: MappingGroupWithStacks[] = []; for (const group of mappingGroups) { stackGroups.push({ ...group, @@ -135,26 +143,17 @@ export async function getDeployedStacks( return Promise.all(summaries.map(normalize)); } -export function formatMappingsHeader(): string { - return formatToStream(fmtMappingsHeader); -} - -export function formatTypedMappings(environment: cxapi.Environment, mappings: TypedMapping[]): string { - return formatToStream((stream) => { - const env = `aws://${environment.account}/${environment.region}`; - fmtTypedMappings(stream, mappings, env); - }); +export function formatEnvironmentSectionHeader(environment: cxapi.Environment) { + const env = `aws://${environment.account}/${environment.region}`; + return formatToStream(stream => fmtEnvironmentSectionHeader(stream, env)); } -export function formatAmbiguitySectionHeader(): string { - return formatToStream(fmtAmbiguitySectionHeader); +export function formatTypedMappings(mappings: TypedMapping[]): string { + return formatToStream((stream) => fmtTypedMappings(stream, mappings)); } -export function formatAmbiguousMappings(environment: cxapi.Environment, paths: [string[], string[]][]): string { - return formatToStream((stream) => { - const env = `aws://${environment.account}/${environment.region}`; - fmtAmbiguousMappings(stream, paths, env); - }); +export function formatAmbiguousMappings(paths: [string[], string[]][]): string { + return formatToStream((stream) => fmtAmbiguousMappings(stream, paths)); } function formatToStream(cb: (stream: NodeJS.WritableStream) => void): string { @@ -162,3 +161,68 @@ function formatToStream(cb: (stream: NodeJS.WritableStream) => void): string { cb(stream); return stream.toString(); } + +/** + * Returns a list of stack groups, each containing the local stacks and the deployed stacks that match the given patterns. + */ +export async function groupStacks(sdkProvider: SdkProvider, cloudAssembly: cxapi.CloudAssembly, localPatterns: string[], deployedPatterns: string[]) { + const localStacks = getLocalStacks(cloudAssembly, localPatterns); + + const environments: Map = new Map(); + + for (const stack of localStacks) { + const environment = await sdkProvider.resolveEnvironment(stack.environment); + const key = hashObject(environment); + environments.set(key, environment); + } + + const localByEnvironment = await indexBy(localStacks, + async (s) => hashObject(await sdkProvider.resolveEnvironment(s.environment)), + ); + + const groups: StackGroup[] = []; + const deployedFilter = matchingPatterns(deployedPatterns, (s) => s.stackName); + for (let key of localByEnvironment.keys()) { + const environment = environments.get(key)!; + groups.push({ + environment, + deployedStacks: (await getDeployedStacks(sdkProvider, environment)).filter(deployedFilter), + localStacks: localByEnvironment.get(key)!, + }); + } + + return groups; +} + +function getLocalStacks(cloudAssembly: cxapi.CloudAssembly, patterns: string[]) { + const all = major(cloudAssembly.version) < 10 ? cloudAssembly.stacks : cloudAssembly.stacksRecursively; + return all.filter(matchingPatterns(patterns ?? [], (s) => s.hierarchicalId)); +} + +function matchingPatterns(patterns: string[], id: (a: S) => string): (s: S) => boolean { + if (patterns.length === 0) { + return (s) => !Object.entries(s.template?.Resources ?? {}) + .some(([logicalId, resource]: [string, any]) => logicalId === 'CdkBootstrapVersion' && resource.Type === 'AWS::SSM::Parameter'); + } + + if (patterns.includes('*')) { + return () => true; + } + + return (s: S) => patterns.some(pattern => minimatch(id(s), pattern)); +} + +async function indexBy(xs: A[], fn: (a: A) => Promise): Promise> { + const ret = new Map(); + for (const x of xs) { + const key = await fn(x); + const group = ret.get(key); + if (group) { + group.push(x); + } else { + ret.set(key, [x]); + } + } + return ret; +} + diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 7ec6e770c..3d5438546 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -59,17 +59,15 @@ import { asIoHelper, IO, SPAN, withoutColor, withoutEmojis, withTrimmedWhitespac import { CloudWatchLogEventMonitor, findCloudWatchLogGroups } from '../api/logs-monitor'; import { Mode, PluginHost } from '../api/plugin'; import { - formatAmbiguitySectionHeader, formatAmbiguousMappings, - formatMappingsHeader, + formatEnvironmentSectionHeader, formatTypedMappings, - getDeployedStacks, + groupStacks, ManifestExcludeList, usePrescribedMappings, } from '../api/refactoring'; -import type { CloudFormationStack, ResourceMapping } from '../api/refactoring/cloudformation'; +import type { ResourceMapping } from '../api/refactoring/cloudformation'; import { RefactoringContext } from '../api/refactoring/context'; -import { hashObject } from '../api/refactoring/digest'; import { ResourceMigrator } from '../api/resource-import'; import { tagsForStack } from '../api/tags/private'; import { DEFAULT_TOOLKIT_STACK_NAME } from '../api/toolkit-info'; @@ -142,12 +140,6 @@ export interface ToolkitOptions { readonly unstableFeatures?: Array; } -interface StackGroup { - environment: cxapi.Environment; - localStacks: CloudFormationStack[]; - deployedStacks: CloudFormationStack[]; -} - /** * Names of toolkit features that are still under development, and may change in * the future. @@ -1072,73 +1064,52 @@ export class Toolkit extends CloudAssemblySourceBuilder { } const sdkProvider = await this.sdkProvider('refactor'); - const stacks = await assembly.selectStacksV2(ALL_STACKS); + + const groups = await groupStacks(sdkProvider, assembly.cloudAssembly, options.localStacks ?? [], options.deployedStacks ?? []); + const mappingSource = options.mappingSource ?? MappingSource.auto(); const exclude = mappingSource.exclude.union(new ManifestExcludeList(assembly.cloudAssembly.manifest)); - const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); - - const refactoringContexts: RefactoringContext[] = []; - for (let { environment, localStacks, deployedStacks } of await groupStacksByEnvironment()) { - refactoringContexts.push(new RefactoringContext({ - environment, - deployedStacks, - localStacks, - filteredStacks: filteredStacks.stackArtifacts, - mappings: await getUserProvidedMappings(environment), - })); - } - const nonAmbiguousContexts = refactoringContexts.filter(c => !c.isAmbiguous); - if (nonAmbiguousContexts.length > 0) { - await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatMappingsHeader(), {})); - } - for (const context of nonAmbiguousContexts) { - const mappings = context.mappings.filter((m) => !exclude.isExcluded(m.destination)); - const typedMappings = mappings.map(m => m.toTypedMapping()); - const environment = context.environment; - await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatTypedMappings(environment, typedMappings), { - typedMappings, - })); - } + for (let { environment, localStacks, deployedStacks } of groups) { + await notifyInfo(formatEnvironmentSectionHeader(environment)); - const ambiguousContexts = refactoringContexts.filter(c => c.isAmbiguous); - if (ambiguousContexts.length > 0) { - await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatAmbiguitySectionHeader(), {})); - } - for (const context of ambiguousContexts) { - const paths = context.ambiguousPaths; - const environment = context.environment; - await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(formatAmbiguousMappings(environment, paths), { - ambiguousPaths: paths, - })); - } + try { + const context = new RefactoringContext({ + environment, + deployedStacks, + localStacks, + mappings: await getUserProvidedMappings(environment), + }); - async function groupStacksByEnvironment(): Promise { - const stackGroups: Map = new Map(); - const environments: Map = new Map(); + const mappings = context.mappings.filter((m) => !exclude.isExcluded(m.destination)); - for (const stack of stacks.stackArtifacts) { - const environment = await sdkProvider.resolveEnvironment(stack.environment); - const key = hashObject(environment); - environments.set(key, environment); - if (stackGroups.has(key)) { - stackGroups.get(key)![1].push(stack); - } else { - // The first time we see an environment, we need to fetch all stacks deployed to it. - const before = await getDeployedStacks(sdkProvider, environment); - stackGroups.set(key, [before, [stack]]); + if (context.ambiguousPaths.length > 0) { + const paths = context.ambiguousPaths; + await notifyInfo(formatAmbiguousMappings(paths), { ambiguousPaths: paths }); + continue; } - } - const result: StackGroup[] = []; - for (const [hash, [deployedStacks, localStacks]] of stackGroups) { - result.push({ - environment: environments.get(hash)!, - localStacks, - deployedStacks, - }); + if (mappings.length === 0) { + await notifyInfo('Nothing to refactor.'); + continue; + } + + const typedMappings = mappings + .map(m => m.toTypedMapping()) + .filter(m => m.type !== 'AWS::CDK::Metadata'); + + await notifyInfo(formatTypedMappings(typedMappings), { typedMappings }); + } catch (e: any) { + await notifyError(e.message, e); } - return result; + } + + async function notifyInfo(message: string, data: any = {}) { + await ioHelper.notify(IO.CDK_TOOLKIT_I8900.msg(message, data)); + } + + async function notifyError(message: string, error: Error) { + await ioHelper.notify(IO.CDK_TOOLKIT_E8900.msg(message, { error })); } async function getUserProvidedMappings(environment: cxapi.Environment): Promise { diff --git a/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts b/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts new file mode 100644 index 000000000..dbd2a4c51 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/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'); + new s3.Bucket(stack, 'MyBucket1'); + new s3.Bucket(stack, 'MyBucket2'); + 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 50a0b54cd..2e6e15764 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -1,5 +1,5 @@ import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; -import { MappingSource, StackSelectionStrategy, Toolkit } from '../../lib'; +import { MappingSource, type RefactorOptions, Toolkit } from '../../lib'; import { SdkProvider } from '../../lib/api/aws-auth/private'; import { builderFixture, TestIoHost } from '../_helpers'; import { mockCloudFormationClient, MockSdk } from '../_helpers/mock-sdk'; @@ -86,6 +86,166 @@ test('detects the same resource in different locations', async () => { ); }); +test('only considers deployed stacks that match the given filter', 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(), + }, + { + StackName: 'Stack2', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/Stack2', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, + { + StackName: 'CDKToolkit', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/CDKToolkit', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, + ], + }); + + mockCloudFormationClient + .on(GetTemplateCommand, { + StackName: 'Stack1', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + OldLogicalID: { + Type: 'AWS::S3::Bucket', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { + 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + }, + }, + }, + }), + }); + + mockCloudFormationClient + .on(GetTemplateCommand, { + StackName: 'Stack2', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + Queue: { + Type: 'AWS::SQS::Queue', + UpdateReplacePolicy: 'Delete', + DeletionPolicy: 'Delete', + Metadata: { + 'aws:cdk:path': 'Stack2/Queue/Resource', + }, + }, + }, + }), + }); + + mockCloudFormationClient + .on(GetTemplateCommand, { + StackName: 'CDKToolkit', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: { + CdkBootstrapVersion: { + Type: 'AWS::SSM::Parameter', + Properties: { + Type: 'String', + Name: { + 'Fn::Sub': '/cdk-bootstrap/${Qualifier}/version', + }, + Value: '1', + }, + }, + }, + }), + }); + + await expectRefactorBehavior('stack-with-bucket', + { + dryRun: true, + // We are not passing any filter, which means that Stack2 will also be included in the comparison. + // This results in the set of deployed resources being different from the local resources, which + // results in an error. + }, + { + action: 'refactor', + level: 'error', + code: 'CDK_TOOLKIT_E8900', + message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), + }, + ); + + await expectRefactorBehavior('stack-with-bucket', + { + dryRun: true, + // To avoid the error, we tell the toolkit to only consider Stack1 in the deployed stacks. + deployedStacks: ['Stack1'], + }, + { + action: 'refactor', + level: 'result', + code: 'CDK_TOOLKIT_I8900', + message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldLogicalID\/Resource.*Stack1\/MyBucket\/Resource/), + data: expect.objectContaining({ + typedMappings: [ + { + sourcePath: 'Stack1/OldLogicalID/Resource', + destinationPath: 'Stack1/MyBucket/Resource', + type: 'AWS::S3::Bucket', + }, + ], + }), + }, + ); + + await expectRefactorBehavior('two-different-stacks', + { + dryRun: true, + // In this case, we are not passing any filter, either, but local and deployed are + // the same, except for the bootstrap stack. But that is ignored by default. + }, { + action: 'refactor', + level: 'result', + code: 'CDK_TOOLKIT_I8900', + data: expect.objectContaining({ + typedMappings: [ + { + sourcePath: 'Stack1/OldLogicalID/Resource', + destinationPath: 'Stack1/MyBucket/Resource', + type: 'AWS::S3::Bucket', + }, + { + sourcePath: 'Stack2/Queue/Resource', + destinationPath: 'Stack2/MyQueue/Resource', + type: 'AWS::SQS::Queue', + }, + ], + }), + }); + + await expectRefactorBehavior('two-different-stacks', + { + dryRun: true, + // But if we pass a wildcard, even the bootstrap stack will be included in the comparison. + deployedStacks: ['*'], + }, { + action: 'refactor', + level: 'error', + code: 'CDK_TOOLKIT_E8900', + message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), + }); +}); + test('detects ambiguous mappings', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ @@ -106,6 +266,7 @@ test('detects ambiguous mappings', async () => { .resolves({ TemplateBody: JSON.stringify({ Resources: { + // These two buckets were replaced with two other buckets CatPhotos: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', @@ -127,7 +288,7 @@ test('detects ambiguous mappings', async () => { }); // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const cx = await builderFixture(toolkit, 'stack-with-two-buckets'); await toolkit.refactor(cx, { dryRun: true, }); @@ -145,19 +306,84 @@ test('detects ambiguous mappings', async () => { │ - │ Stack1/CatPhotos/Resource │ │ │ Stack1/DogPhotos/Resource │ ├───┼───────────────────────────┤ - │ + │ Stack1/Bucket/Resource │ + │ + │ Stack1/MyBucket1/Resource │ + │ │ Stack1/MyBucket2/Resource │ └───┴───────────────────────────┘ */ message: expect.stringMatching( - /-.*Stack1\/CatPhotos\/Resource.*\s+.*Stack1\/DogPhotos\/Resource.*\s+.*\s+.*\+.*Stack1\/MyBucket\/Resource/gm, + /-.*Stack1\/CatPhotos\/Resource.*\s+.*Stack1\/DogPhotos\/Resource.*\s+.*\s+.*\+.*Stack1\/MyBucket1\/Resource.*\s+.*Stack1\/MyBucket2\/Resource/gm, ), data: { - ambiguousPaths: [[['Stack1/CatPhotos/Resource', 'Stack1/DogPhotos/Resource'], ['Stack1/MyBucket/Resource']]], + ambiguousPaths: [ + [ + ['Stack1/CatPhotos/Resource', 'Stack1/DogPhotos/Resource'], + ['Stack1/MyBucket1/Resource', 'Stack1/MyBucket2/Resource'], + ], + ], }, }), ); }); +test('detects modifications to the infrastructure', 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 resource would be refactored + OldName: { + Type: 'AWS::S3::Bucket', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { + 'aws:cdk:path': 'Stack1/OldName/Resource', + }, + }, + // But there is an additional resource that will prevent it + Queue: { + Type: 'AWS::S3::Queue', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { + 'aws:cdk:path': 'Stack1/Queue/Resource', + }, + }, + }, + }), + }); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.refactor(cx, { + dryRun: true, + }); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'refactor', + level: 'error', + code: 'CDK_TOOLKIT_E8900', + message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), + }), + ); +}); + test('fails when dry-run is false', async () => { const cx = await builderFixture(toolkit, 'stack-with-bucket'); await expect( @@ -226,10 +452,8 @@ test('filters stacks when stack selector is passed', async () => { const cx = await builderFixture(toolkit, 'two-different-stacks'); await toolkit.refactor(cx, { dryRun: true, - stacks: { - strategy: StackSelectionStrategy.PATTERN_MATCH, - patterns: ['Stack1'], - }, + localStacks: ['Stack1'], + deployedStacks: ['Stack1'], }); // Resources were renamed in both stacks, but we are only including Stack1. @@ -416,13 +640,15 @@ test('uses the reverse of an explicit mapping when provided', async () => { await toolkit.refactor(cx, { dryRun: true, // ... this is the mapping we used, and now we want to revert it - mappingSource: MappingSource.reverse([{ - account: '123456789012', - region: 'us-east-1', - resources: { - 'Stack1.OldLogicalID': 'Stack1.NewLogicalID', + mappingSource: MappingSource.reverse([ + { + account: '123456789012', + region: 'us-east-1', + resources: { + 'Stack1.OldLogicalID': 'Stack1.NewLogicalID', + }, }, - }]), + ]), }); // THEN @@ -519,18 +745,24 @@ test('computes one set of mappings per environment', async () => { }); // THEN - expect(ioHost.notifySpy).toHaveBeenCalledTimes(3); + expect(ioHost.notifySpy).toHaveBeenCalledTimes(4); - expect(ioHost.notifySpy).toHaveBeenNthCalledWith(2, expect.objectContaining({ - message: expect.stringMatching('aws://123456789012/us-east-1'), - })); + expect(ioHost.notifySpy).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + message: expect.stringMatching('aws://123456789012/us-east-1'), + }), + ); - expect(ioHost.notifySpy).toHaveBeenNthCalledWith(2, + expect(ioHost.notifySpy).toHaveBeenNthCalledWith( + 2, expect.objectContaining({ action: 'refactor', level: 'result', code: 'CDK_TOOLKIT_I8900', - message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldBucketName\/Resource.*Stack1\/NewBucketNameInStack1\/Resource/), + message: expect.stringMatching( + /AWS::S3::Bucket.*Stack1\/OldBucketName\/Resource.*Stack1\/NewBucketNameInStack1\/Resource/, + ), data: expect.objectContaining({ typedMappings: [ { @@ -543,16 +775,22 @@ test('computes one set of mappings per environment', async () => { }), ); - expect(ioHost.notifySpy).toHaveBeenNthCalledWith(3, expect.objectContaining({ - message: expect.stringMatching('aws://123456789012/us-east-2'), - })); + expect(ioHost.notifySpy).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ + message: expect.stringMatching('aws://123456789012/us-east-2'), + }), + ); - expect(ioHost.notifySpy).toHaveBeenNthCalledWith(3, + expect(ioHost.notifySpy).toHaveBeenNthCalledWith( + 4, expect.objectContaining({ action: 'refactor', level: 'result', code: 'CDK_TOOLKIT_I8900', - message: expect.stringMatching(/AWS::S3::Bucket.*Stack2\/OldBucketName\/Resource.*Stack2\/NewBucketNameInStack2\/Resource/), + message: expect.stringMatching( + /AWS::S3::Bucket.*Stack2\/OldBucketName\/Resource.*Stack2\/NewBucketNameInStack2\/Resource/, + ), data: expect.objectContaining({ typedMappings: [ { @@ -565,3 +803,11 @@ test('computes one set of mappings per environment', async () => { }), ); }); + +async function expectRefactorBehavior(fixtureName: string, input: RefactorOptions, output: E) { + const host = new TestIoHost(); + const tk = new Toolkit({ ioHost: host, unstableFeatures: ['refactor'] }); + await tk.refactor(await builderFixture(tk, fixtureName), input); + expect(host.notifySpy).toHaveBeenCalledWith(expect.objectContaining(output)); +} + diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts index e7c895a5e..17051183f 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts @@ -50,7 +50,7 @@ describe('typed mappings', () => { expect(context.mappings).toEqual([]); }); - test('returns empty mappings when there are only removals', () => { + test('resource removal is not allowed', () => { const stack1 = { environment, stackName: 'Foo', @@ -73,15 +73,14 @@ describe('typed mappings', () => { }, }; - const context = new RefactoringContext({ + expect(() => new RefactoringContext({ environment, deployedStacks: [stack1], localStacks: [stack2], - }); - expect(context.mappings).toEqual([]); + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); - test('returns empty mappings when there are only additions', () => { + test('resource addition is not allowed', () => { const stack1 = { environment, stackName: 'Foo', @@ -104,15 +103,14 @@ describe('typed mappings', () => { }, }; - const context = new RefactoringContext({ + expect(() => new RefactoringContext({ environment, deployedStacks: [stack1], localStacks: [stack2], - }); - expect(context.mappings).toEqual([]); + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); - test('normal updates are not mappings', () => { + test('resource update is not allowed', () => { const stack1 = { environment, stackName: 'Foo', @@ -136,17 +134,17 @@ describe('typed mappings', () => { Bucket1: { Type: 'AWS::S3::Bucket', // Updated property - Properties: { Prop: 'old value' }, + Properties: { Prop: 'new value' }, }, }, }, }; - const context = new RefactoringContext({ + + expect(() => new RefactoringContext({ environment, deployedStacks: [stack1], localStacks: [stack2], - }); - expect(context.mappings).toEqual([]); + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); test('moving resources across stacks', () => { @@ -299,16 +297,14 @@ describe('typed mappings', () => { }, }; - const context = new RefactoringContext({ - environment, - deployedStacks: [stack1], - localStacks: [stack2], - }); - // We don't consider that a resource was moved from Foo.OldName to Bar.NewName, // even though they have the same properties. Since they have different types, // they are considered different resources. - expect(context.mappings).toEqual([]); + expect(() => new RefactoringContext({ + environment, + deployedStacks: [stack1], + localStacks: [stack2], + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); test('ambiguous resources from multiple stacks', () => { @@ -351,12 +347,11 @@ describe('typed mappings', () => { }, }; - const context = new RefactoringContext({ + expect(() => new RefactoringContext({ environment, deployedStacks: [stack1, stack2], localStacks: [stack3], - }); - expect(context.ambiguousPaths).toEqual([[['Stack1.Bucket1', 'Stack2.Bucket2'], ['Stack3.Bucket3']]]); + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); test('ambiguous pairs', () => { @@ -450,174 +445,11 @@ describe('typed mappings', () => { }, }; - const context = new RefactoringContext({ + expect(() => new RefactoringContext({ environment, deployedStacks: [stack1], localStacks: [stack2], - }); - expect(context.mappings.map(toCfnMapping)).toEqual([ - { - Source: { LogicalResourceId: 'OldName', StackName: 'Foo' }, - Destination: { LogicalResourceId: 'NewName', StackName: 'Foo' }, - }, - ]); - }); - - test('stack filtering', () => { - // eslint-disable-next-line @typescript-eslint/no-shadow - 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' }, - }, - }, - }, - }; - - // Testing different filters: - - // Only Foo. Should include Foo and Bar - let context = new RefactoringContext({ - environment, - deployedStacks: [stack1, stack3], - localStacks: [stack2, stack4], - filteredStacks: [stack1], - }); - expect(context.mappings.map(toCfnMapping)).toEqual([ - { - Destination: { - LogicalResourceId: 'Bucket1', - StackName: 'Bar', - }, - Source: { - LogicalResourceId: 'Bucket1', - StackName: 'Foo', - }, - }, - ]); - - // Only Bar. Should include Foo and Bar - context = new RefactoringContext({ - environment, - deployedStacks: [stack1, stack3], - localStacks: [stack2, stack4], - filteredStacks: [stack2], - }); - expect(context.mappings.map(toCfnMapping)).toEqual([ - { - Destination: { - LogicalResourceId: 'Bucket1', - StackName: 'Bar', - }, - Source: { - LogicalResourceId: 'Bucket1', - StackName: 'Foo', - }, - }, - ]); - - // Only Zee. Should include Zee - context = new RefactoringContext({ - environment, - deployedStacks: [stack1, stack3], - localStacks: [stack2, stack4], - filteredStacks: [stack3], - }); - expect(context.mappings.map(toCfnMapping)).toEqual([ - { - Destination: { - LogicalResourceId: 'NewName', - StackName: 'Zee', - }, - Source: { - LogicalResourceId: 'OldName', - StackName: 'Zee', - }, - }, - ]); - - // Foo and Zee. Should include all - context = new RefactoringContext({ - environment, - deployedStacks: [stack1, stack3], - localStacks: [stack2, stack4], - filteredStacks: [stack1, stack3], - }); - expect(context.mappings.map(toCfnMapping)).toEqual([ - { - Destination: { - LogicalResourceId: 'Bucket1', - StackName: 'Bar', - }, - Source: { - LogicalResourceId: 'Bucket1', - StackName: 'Foo', - }, - }, - { - Destination: { - LogicalResourceId: 'NewName', - StackName: 'Zee', - }, - Source: { - LogicalResourceId: 'OldName', - StackName: 'Zee', - }, - }, - ]); + })).toThrow(/A refactor operation cannot add, remove or update resources/); }); }); 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 113f1a2c8..367f2f593 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 @@ -334,74 +334,6 @@ describe(computeResourceDigests, () => { expect(result['Stack1.Q1']).toBe(result['Stack1.Q2']); }); - test('uses physical ID if present', () => { - mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'], - }); - - const template = { - Resources: { - Foo1: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldBeIgnored: true, - }, - }, - Foo2: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldAlsoBeIgnored: true, - }, - }, - }, - }; - - const result = computeResourceDigests(makeStacks([template])); - expect(result['Stack1.Foo1']).toBeDefined(); - expect(result['Stack1.Foo2']).toBeDefined(); - expect(result['Stack1.Foo1']).toEqual(result['Stack1.Foo2']); - }); - - test('uses physical ID if present - with dependencies', () => { - mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName'], - }); - - const template = { - Resources: { - Foo1: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldBeIgnored: true, - }, - }, - Foo2: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldAlsoBeIgnored: true, - }, - }, - Bucket1: { - Type: 'AWS::S3::Bucket', - Properties: { Dep: { Ref: 'Foo1' } }, - }, - Bucket2: { - Type: 'AWS::S3::Bucket', - Properties: { Dep: { Ref: 'Foo2' } }, - }, - }, - }; - - const result = computeResourceDigests(makeStacks([template])); - expect(result['Stack1.Bucket1']).toBeDefined(); - expect(result['Stack1.Bucket2']).toBeDefined(); - expect(result['Stack1.Bucket1']).toEqual(result['Stack1.Bucket2']); - }); - test('different physical IDs lead to different digests', () => { mockLoadResourceModel.mockReturnValue({ primaryIdentifier: ['FooName'], @@ -464,68 +396,6 @@ describe(computeResourceDigests, () => { expect(result['Stack1.Foo1']).not.toEqual(result['Stack1.Foo2']); }); - test('primaryIdentifier is a composite field - same value', () => { - mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'], - }); - - const template = { - Resources: { - Foo1: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - BarName: 'YYYYYYYYY', - ShouldBeIgnored: true, - }, - }, - Foo2: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - BarName: 'YYYYYYYYY', - ShouldAlsoBeIgnored: true, - }, - }, - }, - }; - - const result = computeResourceDigests(makeStacks([template])); - expect(result['Stack1.Foo1']).toBeDefined(); - expect(result['Stack1.Foo2']).toBeDefined(); - expect(result['Stack1.Foo1']).toEqual(result['Stack1.Foo2']); - }); - - test('primaryIdentifier is a composite field but not all of them are set in the resource', () => { - mockLoadResourceModel.mockReturnValue({ - primaryIdentifier: ['FooName', 'BarName'], - }); - - const template = { - Resources: { - Foo1: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldBeIgnored: true, - }, - }, - Foo2: { - Type: 'AWS::S3::Foo', - Properties: { - FooName: 'XXXXXXXXX', - ShouldAlsoBeIgnored: true, - }, - }, - }, - }; - - const result = computeResourceDigests(makeStacks([template])); - expect(result['Stack1.Foo1']).toBeDefined(); - expect(result['Stack1.Foo2']).toBeDefined(); - expect(result['Stack1.Foo1']).not.toEqual(result['Stack1.Foo2']); - }); - test('resource properties does not contain primaryIdentifier - different values', () => { mockLoadResourceModel.mockReturnValue({ primaryIdentifier: ['FooName'], diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index b1dcf95b3..d966a250d 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -1101,8 +1101,12 @@ 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: +(to a different stack or to a different logical ID, or both). In keeping with +the CloudFormation API, you are not allowed to modify the set of resources +as part of a refactor. In other words, adding, deleting or updating resources +is considered an error. + +The CLI will show the correspondence between the old and new locations in a table: ``` $ cdk refactor --unstable=refactor --dry-run @@ -1145,13 +1149,17 @@ $ cdk refactor --exclude-file exclude.txt --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: +command to consider only a subset of them, you can specify the stacks you +want, both local and deployed: ```shell -$ cdk refactor Web* --unstable=refactor --dry-run +$ cdk refactor --local-stack Foo --local-stack Bar --deployed-stack Foo --unstable=refactor --dry-run ``` +This is useful if, for example, you have more than one CDK application deployed +to a given environment, and you want to only include the deployed stacks that +belong to the application that you are refactoring. + 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. diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 2ac0c21c6..3e48e8c7d 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1227,10 +1227,8 @@ export class CdkToolkit { try { await this.toolkit.refactor(this.props.cloudExecutable, { dryRun: options.dryRun, - stacks: { - patterns: options.selector.patterns, - strategy: options.selector.patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, - }, + localStacks: options.localStacks, + deployedStacks: options.deployedStacks, mappingSource: await mappingSource(), }); } catch (e) { @@ -1960,11 +1958,6 @@ export interface RefactorOptions { */ readonly dryRun: boolean; - /** - * Criteria for selecting stacks to deploy - */ - selector: StackSelector; - /** * The absolute path to a file that contains a list of resources that * should be excluded during the refactor. This file should contain a @@ -2012,6 +2005,22 @@ export interface RefactorOptions { * that was previously applied. */ revert?: boolean; + + /** + * List of patterns for filtering local stacks. If no patterns are passed, + * then all stacks, except the bootstrap stacks are considered. If you want + * to consider all stacks (including bootstrap stacks), pass the wildcard + * '*'. + */ + localStacks?: string[]; + + /** + * List of patterns for filtering deployed stacks. If no patterns are passed, + * then all stacks, except the bootstrap stacks are considered. If you want + * to consider all stacks (including bootstrap stacks), pass the wildcard + * '*'. + */ + deployedStacks?: string[]; } /** diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index 3ea32e677..0975f86a7 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -436,11 +436,15 @@ export async function makeConfig(): Promise { }, refactor: { description: 'Moves resources between stacks or within the same stack', - arg: { - name: 'STACKS', - variadic: true, - }, options: { + 'local-stack': { + type: 'array', + desc: 'Filter to apply for stacks in the cloud assembly', + }, + 'deployed-stack': { + type: 'array', + desc: 'Filter to apply for stacks deployed to the AWS account', + }, 'dry-run': { type: 'boolean', desc: 'Do not perform any changes, just show what would be done', diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index 32dfdbea6..a72d9bd33 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -276,10 +276,11 @@ 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) => + .command('refactor', 'Moves resources between stacks or within the same stack', (yargs: Argv) => yargs + .option('local-stack', { + type: 'array', + desc: 'Filter to apply for stacks in the cloud assembly', + nargs: 1, + requiresArg: true, + }) + .option('deployed-stack', { + type: 'array', + desc: 'Filter to apply for stacks deployed to the AWS account', + nargs: 1, + requiresArg: true, + }) .option('dry-run', { default: false, type: 'boolean', diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 0aa41a901..344e438d2 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1419,6 +1419,20 @@ export interface DocsOptions { * @struct */ export interface RefactorOptions { + /** + * Filter to apply for stacks in the cloud assembly + * + * @default - undefined + */ + readonly localStack?: Array; + + /** + * Filter to apply for stacks deployed to the AWS account + * + * @default - undefined + */ + readonly deployedStack?: Array; + /** * Do not perform any changes, just show what would be done * @@ -1446,9 +1460,4 @@ export interface RefactorOptions { * @default - false */ readonly revert?: boolean; - - /** - * Positional argument for refactor - */ - readonly STACKS?: Array; } From f28a5f31d1b3652a45f127bdcac31ff94b3d5e15 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 2 Jul 2025 11:49:02 +0100 Subject: [PATCH 02/13] feat: overrides to resolve mapping ambiguities --- .../toolkit-lib/lib/actions/refactor/index.ts | 42 ++- .../lib/api/refactoring/context.ts | 61 +++- .../toolkit-lib/lib/toolkit/toolkit.ts | 60 ++-- .../_fixtures/stack-with-two-buckets/index.ts | 6 +- .../toolkit-lib/test/actions/refactor.test.ts | 295 ++++++------------ packages/aws-cdk/README.md | 43 ++- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 82 ++--- packages/aws-cdk/lib/cli/cli-config.ts | 9 +- packages/aws-cdk/lib/cli/cli.ts | 3 +- .../aws-cdk/lib/cli/convert-to-user-input.ts | 6 +- .../lib/cli/parse-command-line-arguments.ts | 10 +- packages/aws-cdk/lib/cli/user-input.ts | 13 +- 12 files changed, 287 insertions(+), 343 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 4080c3959..9d5ef1bed 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -1,5 +1,6 @@ import type { ExcludeList } from '../../api/refactoring'; import { InMemoryExcludeList, NeverExclude } from '../../api/refactoring'; +import { ToolkitError } from '../../toolkit/toolkit-error'; type MappingType = 'auto' | 'explicit'; @@ -69,11 +70,6 @@ export interface RefactorOptions { */ readonly dryRun?: boolean; - /** - * How the toolkit should obtain the mappings - */ - mappingSource?: MappingSource; - /** * List of patterns for filtering local stacks. If no patterns are passed, * then all stacks, except the bootstrap stacks are considered. If you want @@ -89,6 +85,12 @@ export interface RefactorOptions { * '*'. */ deployedStacks?: string[]; + + /** + * List of overrides to be applied to resolve possible ambiguities in the + * computed list of mappings. + */ + overrides?: MappingGroup[]; } export interface MappingGroup { @@ -114,3 +116,33 @@ export interface MappingGroup { [key: string]: string; }; } + +export function parseMappingGroups(s: string) { + const mappingGroups = doParse(); + + // Validate that there are no duplicate destinations. + // By construction, there are no duplicate sources, already. + for (let group of mappingGroups) { + const destinations = new Set(); + + for (const destination of Object.values(group.resources)) { + if (destinations.has(destination)) { + throw new ToolkitError( + `Duplicate destination resource '${destination}' in environment ${group.account}/${group.region}`, + ); + } + destinations.add(destination); + } + } + + return mappingGroups; + + function doParse(): MappingGroup[] { + const content = JSON.parse(s); + if (content.environments || !Array.isArray(content.environments)) { + return content.environments; + } else { + throw new ToolkitError("Expected an 'environments' array"); + } + } +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index 0de295785..c05898c3d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -15,7 +15,7 @@ export interface RefactorManagerOptions { environment: Environment; localStacks: CloudFormationStack[]; deployedStacks: CloudFormationStack[]; - mappings?: ResourceMapping[]; + overrides?: ResourceMapping[]; } /** @@ -28,14 +28,10 @@ export class RefactoringContext { constructor(props: RefactorManagerOptions) { this.environment = props.environment; - if (props.mappings != null) { - this._mappings = props.mappings; - } else { - const moves = resourceMoves(props.deployedStacks, props.localStacks); - this.ambiguousMoves = moves.filter(isAmbiguousMove); - const nonAmbiguousMoves = moves.filter((move) => !isAmbiguousMove(move)); - this._mappings = resourceMappings(nonAmbiguousMoves); - } + const moves = resourceMoves(props.deployedStacks, props.localStacks); + const [nonAmbiguousMoves, ambiguousMoves] = partitionByAmbiguity(props.overrides ?? [], moves); + this.ambiguousMoves = ambiguousMoves; + this._mappings = resourceMappings(nonAmbiguousMoves); } public get ambiguousPaths(): [string[], string[]][] { @@ -178,3 +174,50 @@ function equalSets(a: Set, b: Set) { } return true; } + +/** + * Partitions a list of moves into non-ambiguous and ambiguous moves. + * @param overrides - The list of overrides to disambiguate moves + * @param moves - a pair of lists of moves. First: non-ambiguous, second: ambiguous + */ +function partitionByAmbiguity(overrides: ResourceMapping[], moves: ResourceMove[]): [ResourceMove[], ResourceMove[]] { + const ambiguous: ResourceMove[] = []; + const nonAmbiguous: ResourceMove[] = []; + + for (let move of moves) { + if (!isAmbiguousMove(move)) { + nonAmbiguous.push(move); + } else { + for (const override of overrides) { + const resolvedMove = resolve(override, move); + if (resolvedMove != null) { + nonAmbiguous.push(resolvedMove); + move = remove(override, move); + } + } + // One last chance to be non-ambiguous + if (!isAmbiguousMove(move)) { + nonAmbiguous.push(move); + } else { + ambiguous.push(move); + } + } + } + + function resolve(override: ResourceMapping, move: ResourceMove): ResourceMove | undefined { + const [pre, post] = move; + const source = pre.find((loc) => loc.equalTo(override.source)); + const destination = post.find((loc) => loc.equalTo(override.destination)); + return (source && destination) ? [[source], [destination]] : undefined; + } + + function remove(override: ResourceMapping, move: ResourceMove): ResourceMove { + const [pre, post] = move; + return [ + pre.filter(loc => !loc.equalTo(override.source)), + post.filter(loc => !loc.equalTo(override.destination)), + ]; + } + + return [nonAmbiguous, ambiguous]; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 3d5438546..5b1d78dd5 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -28,8 +28,7 @@ import type { DiffOptions } from '../actions/diff'; import { appendObject, prepareDiff } from '../actions/diff/private'; import type { DriftOptions, DriftResult } from '../actions/drift'; import { type ListOptions } from '../actions/list'; -import type { MappingGroup, RefactorOptions } from '../actions/refactor'; -import { MappingSource } from '../actions/refactor'; +import type { RefactorOptions } from '../actions/refactor'; import { type RollbackOptions } from '../actions/rollback'; import { type SynthOptions } from '../actions/synth'; import type { IWatcher, WatchOptions } from '../actions/watch'; @@ -63,10 +62,8 @@ import { formatEnvironmentSectionHeader, formatTypedMappings, groupStacks, - ManifestExcludeList, - usePrescribedMappings, } from '../api/refactoring'; -import type { ResourceMapping } from '../api/refactoring/cloudformation'; +import { type CloudFormationStack, ResourceLocation, ResourceMapping } from '../api/refactoring/cloudformation'; import { RefactoringContext } from '../api/refactoring/context'; import { ResourceMigrator } from '../api/resource-import'; import { tagsForStack } from '../api/tags/private'; @@ -1064,12 +1061,8 @@ export class Toolkit extends CloudAssemblySourceBuilder { } const sdkProvider = await this.sdkProvider('refactor'); - const groups = await groupStacks(sdkProvider, assembly.cloudAssembly, options.localStacks ?? [], options.deployedStacks ?? []); - const mappingSource = options.mappingSource ?? MappingSource.auto(); - const exclude = mappingSource.exclude.union(new ManifestExcludeList(assembly.cloudAssembly.manifest)); - for (let { environment, localStacks, deployedStacks } of groups) { await notifyInfo(formatEnvironmentSectionHeader(environment)); @@ -1078,23 +1071,21 @@ export class Toolkit extends CloudAssemblySourceBuilder { environment, deployedStacks, localStacks, - mappings: await getUserProvidedMappings(environment), + overrides: getOverrides(environment, deployedStacks, localStacks), }); - const mappings = context.mappings.filter((m) => !exclude.isExcluded(m.destination)); - if (context.ambiguousPaths.length > 0) { const paths = context.ambiguousPaths; await notifyInfo(formatAmbiguousMappings(paths), { ambiguousPaths: paths }); continue; } - if (mappings.length === 0) { + if (context.mappings.length === 0) { await notifyInfo('Nothing to refactor.'); continue; } - const typedMappings = mappings + const typedMappings = context.mappings .map(m => m.toTypedMapping()) .filter(m => m.type !== 'AWS::CDK::Metadata'); @@ -1112,13 +1103,42 @@ export class Toolkit extends CloudAssemblySourceBuilder { await ioHelper.notify(IO.CDK_TOOLKIT_E8900.msg(message, { error })); } - async function getUserProvidedMappings(environment: cxapi.Environment): Promise { - return mappingSource.source == 'explicit' - ? usePrescribedMappings(mappingSource.groups.filter(matchesEnvironment), sdkProvider) - : undefined; + function getOverrides(environment: cxapi.Environment, deployedStacks: CloudFormationStack[], localStacks: CloudFormationStack[]) { + const mappingGroup = options.overrides + ?.find(g => g.region === environment.region && g.account === environment.account); + + let overrides: ResourceMapping[] = []; + if (mappingGroup != null) { + overrides = Object.entries(mappingGroup.resources ?? {}).map(([source, destination]) => { + const sourceStack = findStack(source, deployedStacks); + const sourceLogicalId = source.split('.')[1]; + + const destinationStack = findStack(destination, localStacks); + const destinationLogicalId = destination.split('.')[1]; + + return new ResourceMapping( + new ResourceLocation(sourceStack, sourceLogicalId), + new ResourceLocation(destinationStack, destinationLogicalId), + ); + }); + } + + return overrides; + + function findStack(location: string, stacks: CloudFormationStack[]): CloudFormationStack { + const result = stacks.find(stack => { + const [stackName, logicalId] = location.split('.'); + if (stackName == null || logicalId == null) { + throw new ToolkitError(`Invalid location '${location}'`); + } + return stack.stackName === stackName && stack.template.Resources?.[logicalId] != null; + }); + + if (result == null) { + throw new ToolkitError(`Cannot find resource in location ${location}`); + } - function matchesEnvironment(g: MappingGroup): boolean { - return g.account === environment.account && g.region === environment.region; + return result; } } } diff --git a/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts b/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts index dbd2a4c51..ca75970c9 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_fixtures/stack-with-two-buckets/index.ts @@ -3,7 +3,11 @@ 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 stack = new core.Stack(app, 'Stack1', { + env: { + account: '123456789012', region: 'us-east-1', + }, + }); new s3.Bucket(stack, 'MyBucket1'); new s3.Bucket(stack, 'MyBucket2'); 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 2e6e15764..160fb9524 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -1,5 +1,5 @@ import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; -import { MappingSource, type RefactorOptions, Toolkit } from '../../lib'; +import { type RefactorOptions, Toolkit } from '../../lib'; import { SdkProvider } from '../../lib/api/aws-auth/private'; import { builderFixture, TestIoHost } from '../_helpers'; import { mockCloudFormationClient, MockSdk } from '../_helpers/mock-sdk'; @@ -170,7 +170,8 @@ test('only considers deployed stacks that match the given filter', async () => { }), }); - await expectRefactorBehavior('stack-with-bucket', + await expectRefactorBehavior( + 'stack-with-bucket', { dryRun: true, // We are not passing any filter, which means that Stack2 will also be included in the comparison. @@ -185,7 +186,8 @@ test('only considers deployed stacks that match the given filter', async () => { }, ); - await expectRefactorBehavior('stack-with-bucket', + await expectRefactorBehavior( + 'stack-with-bucket', { dryRun: true, // To avoid the error, we tell the toolkit to only consider Stack1 in the deployed stacks. @@ -208,12 +210,14 @@ test('only considers deployed stacks that match the given filter', async () => { }, ); - await expectRefactorBehavior('two-different-stacks', + await expectRefactorBehavior( + 'two-different-stacks', { dryRun: true, // In this case, we are not passing any filter, either, but local and deployed are // the same, except for the bootstrap stack. But that is ignored by default. - }, { + }, + { action: 'refactor', level: 'result', code: 'CDK_TOOLKIT_I8900', @@ -231,19 +235,23 @@ test('only considers deployed stacks that match the given filter', async () => { }, ], }), - }); + }, + ); - await expectRefactorBehavior('two-different-stacks', + await expectRefactorBehavior( + 'two-different-stacks', { dryRun: true, // But if we pass a wildcard, even the bootstrap stack will be included in the comparison. deployedStacks: ['*'], - }, { + }, + { action: 'refactor', level: 'error', code: 'CDK_TOOLKIT_E8900', message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), - }); + }, + ); }); test('detects ambiguous mappings', async () => { @@ -325,7 +333,7 @@ test('detects ambiguous mappings', async () => { ); }); -test('detects modifications to the infrastructure', async () => { +test('overrides can be used to resolve ambiguities', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -345,22 +353,21 @@ test('detects modifications to the infrastructure', async () => { .resolves({ TemplateBody: JSON.stringify({ Resources: { - // This resource would be refactored - OldName: { + // These two buckets were replaced with two other buckets + CatPhotos: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/OldName/Resource', + 'aws:cdk:path': 'Stack1/CatPhotos/Resource', }, }, - // But there is an additional resource that will prevent it - Queue: { - Type: 'AWS::S3::Queue', + DogPhotos: { + Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/Queue/Resource', + 'aws:cdk:path': 'Stack1/DogPhotos/Resource', }, }, }, @@ -368,32 +375,46 @@ test('detects modifications to the infrastructure', async () => { }); // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const cx = await builderFixture(toolkit, 'stack-with-two-buckets'); await toolkit.refactor(cx, { dryRun: true, + overrides: [ + { + account: '123456789012', + region: 'us-east-1', + resources: { + // Only one override is enough in this case + 'Stack1.CatPhotos': 'Stack1.MyBucket1553EAA46', + }, + }, + ], }); // THEN expect(ioHost.notifySpy).toHaveBeenCalledWith( expect.objectContaining({ action: 'refactor', - level: 'error', - code: 'CDK_TOOLKIT_E8900', - message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), + level: 'result', + code: 'CDK_TOOLKIT_I8900', + data: { + typedMappings: [ + { + destinationPath: 'Stack1/MyBucket1/Resource', + sourcePath: 'Stack1/CatPhotos/Resource', + type: 'AWS::S3::Bucket', + }, + { + destinationPath: 'Stack1/MyBucket2/Resource', + sourcePath: 'Stack1/DogPhotos/Resource', + type: 'AWS::S3::Bucket', + }, + ], + }, }), ); }); -test('fails when dry-run is false', async () => { - const cx = await builderFixture(toolkit, 'stack-with-bucket'); - await expect( - toolkit.refactor(cx, { - dryRun: false, - }), - ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); -}); - -test('filters stacks when stack selector is passed', async () => { +test('detects modifications to the infrastructure', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -403,12 +424,6 @@ test('filters stacks 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(), - }, ], }); @@ -419,29 +434,22 @@ test('filters stacks when stack selector is passed', async () => { .resolves({ TemplateBody: JSON.stringify({ Resources: { - OldBucketName: { + // This resource would be refactored + OldName: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/OldBucketName/Resource', + 'aws:cdk:path': 'Stack1/OldName/Resource', }, }, - }, - }), - }) - .on(GetTemplateCommand, { - StackName: 'Stack2', - }) - .resolves({ - TemplateBody: JSON.stringify({ - Resources: { - OldQueueName: { - Type: 'AWS::SQS::Queue', - UpdateReplacePolicy: 'Delete', - DeletionPolicy: 'Delete', + // But there is an additional resource that will prevent it + Queue: { + Type: 'AWS::S3::Queue', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack2/OldQueueName/Resource', + 'aws:cdk:path': 'Stack1/Queue/Resource', }, }, }, @@ -449,41 +457,32 @@ test('filters stacks when stack selector is passed', async () => { }); // WHEN - const cx = await builderFixture(toolkit, 'two-different-stacks'); + const cx = await builderFixture(toolkit, 'stack-with-bucket'); await toolkit.refactor(cx, { dryRun: true, - localStacks: ['Stack1'], - deployedStacks: ['Stack1'], }); - // Resources were renamed in both stacks, but we are only including Stack1. - // So expect to see only changes for Stack1. + // THEN expect(ioHost.notifySpy).toHaveBeenCalledWith( expect.objectContaining({ action: 'refactor', - 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', - }, - ], - }), + level: 'error', + code: 'CDK_TOOLKIT_E8900', + message: expect.stringMatching(/A refactor operation cannot add, remove or update resources/), }), ); +}); - expect(ioHost.notifySpy).toHaveBeenCalledWith( - expect.objectContaining({ - message: expect.not.stringMatching(/OldQueueName/), +test('fails when dry-run is false', async () => { + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await expect( + toolkit.refactor(cx, { + dryRun: false, }), - ); + ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); }); -test('resource is marked to be excluded for refactoring in the cloud assembly', async () => { +test('filters stacks when stack selector is passed', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ StackSummaries: [ @@ -493,6 +492,12 @@ test('resource is marked to be excluded for refactoring in the cloud assembly', StackStatus: 'CREATE_COMPLETE', CreationTime: new Date(), }, + { + StackName: 'Stack2', + StackId: 'arn:aws:cloudformation:us-east-1:999999999999:stack/Stack2', + StackStatus: 'CREATE_COMPLETE', + CreationTime: new Date(), + }, ], }); @@ -503,64 +508,29 @@ test('resource is marked to be excluded for refactoring in the cloud assembly', .resolves({ TemplateBody: JSON.stringify({ Resources: { - // This would have caused a refactor to be detected, - // but the resource is marked to be excluded... - OldLogicalID: { + OldBucketName: { Type: 'AWS::S3::Bucket', UpdateReplacePolicy: 'Retain', DeletionPolicy: 'Retain', Metadata: { - 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + 'aws:cdk:path': 'Stack1/OldBucketName/Resource', }, }, }, }), - }); - - // WHEN - const cx = await builderFixture(toolkit, 'exclude-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/), - }), - ); -}); - -test('uses the explicit mapping when provided, instead of computing it on-the-fly', async () => { - // GIVEN - mockCloudFormationClient.on(ListStacksCommand).resolves({ - StackSummaries: [ - { - StackName: 'Stack1', - StackId: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1', - StackStatus: 'CREATE_COMPLETE', - CreationTime: new Date(), - }, - ], - }); - - mockCloudFormationClient + }) .on(GetTemplateCommand, { - StackName: 'Stack1', + StackName: 'Stack2', }) .resolves({ TemplateBody: JSON.stringify({ Resources: { - OldLogicalID: { - Type: 'AWS::S3::Bucket', - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', + OldQueueName: { + Type: 'AWS::SQS::Queue', + UpdateReplacePolicy: 'Delete', + DeletionPolicy: 'Delete', Metadata: { - 'aws:cdk:path': 'Stack1/OldLogicalID/Resource', + 'aws:cdk:path': 'Stack2/OldQueueName/Resource', }, }, }, @@ -568,105 +538,36 @@ test('uses the explicit mapping when provided, instead of computing it on-the-fl }); // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const cx = await builderFixture(toolkit, 'two-different-stacks'); await toolkit.refactor(cx, { dryRun: true, - mappingSource: MappingSource.explicit([ - { - account: '123456789012', - region: 'us-east-1', - resources: { - 'Stack1.OldLogicalID': 'Stack1.NewLogicalID', - }, - }, - ]), + localStacks: ['Stack1'], + deployedStacks: ['Stack1'], }); - // THEN + // 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: 'result', code: 'CDK_TOOLKIT_I8900', - message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldLogicalID\/Resource.*Stack1\.NewLogicalID/), + message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/OldBucketName\/Resource.*Stack1\/MyBucket\/Resource/), data: expect.objectContaining({ typedMappings: [ { - sourcePath: 'Stack1/OldLogicalID/Resource', - destinationPath: 'Stack1.NewLogicalID', + sourcePath: 'Stack1/OldBucketName/Resource', + destinationPath: 'Stack1/MyBucket/Resource', type: 'AWS::S3::Bucket', }, ], }), }), ); -}); -test('uses the reverse of an explicit mapping when provided', async () => { - // GIVEN - mockCloudFormationClient.on(ListStacksCommand).resolves({ - StackSummaries: [ - { - StackName: 'Stack1', - StackId: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1', - StackStatus: 'CREATE_COMPLETE', - CreationTime: new Date(), - }, - ], - }); - - mockCloudFormationClient - .on(GetTemplateCommand, { - StackName: 'Stack1', - }) - .resolves({ - TemplateBody: JSON.stringify({ - Resources: { - // Suppose we had already mapped OldLogicalID -> NewLogicalID... - NewLogicalID: { - Type: 'AWS::S3::Bucket', - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', - Metadata: { - 'aws:cdk:path': 'Stack1/NewLogicalID/Resource', - }, - }, - }, - }), - }); - - // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); - await toolkit.refactor(cx, { - dryRun: true, - // ... this is the mapping we used, and now we want to revert it - mappingSource: MappingSource.reverse([ - { - account: '123456789012', - region: 'us-east-1', - resources: { - 'Stack1.OldLogicalID': 'Stack1.NewLogicalID', - }, - }, - ]), - }); - - // THEN expect(ioHost.notifySpy).toHaveBeenCalledWith( expect.objectContaining({ - action: 'refactor', - level: 'result', - code: 'CDK_TOOLKIT_I8900', - message: expect.stringMatching(/AWS::S3::Bucket.*Stack1\/NewLogicalID\/Resource.*Stack1\.OldLogicalID/), - data: expect.objectContaining({ - typedMappings: [ - { - sourcePath: 'Stack1/NewLogicalID/Resource', - destinationPath: 'Stack1.OldLogicalID', - type: 'AWS::S3::Bucket', - }, - ], - }), + message: expect.not.stringMatching(/OldQueueName/), }), ); }); diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index d966a250d..cbcfa0d8b 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -1129,25 +1129,6 @@ 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 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 (destination locations separated by -newlines): - -``` -Web/Website/Origin/Resource -Web/Website/Distribution/Resource -``` - -and pass it to the CLI via the `--exclude-file` flag: - -```shell -$ cdk refactor --exclude-file exclude.txt --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 specify the stacks you want, both local and deployed: @@ -1164,10 +1145,24 @@ 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. -If, instead of letting the CLI decide which resources to move, you want to -provide your own mapping of old to new locations, you can do so by passing a -mapping file to the CLI via the `--mapping-file` flag. This file should -contain a JSON object with the following format: +In case of ambiguities, the CLI will display a table like this: + +``` +Detected ambiguities: +┌───┬──────────────────────┐ +│ │ Resource │ +├───┼──────────────────────┤ +│ - │ Stack2/DLQ/Resource │ +│ │ Stack2/DLQ2/Resource │ +├───┼──────────────────────┤ +│ + │ Stack1/DLQ/Resource │ +│ │ Stack1/DLQ2/Resource │ +└───┴──────────────────────┘ +``` + +You can resolve this ambiguity manually, by passing an override file via the +`--override-file=` CLI option. This file should contain a JSON object +with the following structure: ```json { @@ -1190,7 +1185,7 @@ resource currently deployed, while the destination must refer to a location that is not already occupied by any resource. If you want to undo a refactor, you can use the `--revert` option in -conjunction with the `--mapping-file` option. It will apply the mapping in +conjunction with the `--override-file` option. It will apply the mapping in reverse order (source becomes destination and vice versa). ### `cdk drift` diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 3e48e8c7d..8b4c9b5a3 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -2,8 +2,15 @@ import * as path from 'path'; import { format } from 'util'; import { RequireApproval } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; -import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib'; -import { StackSelectionStrategy, ToolkitError, PermissionChangeType, Toolkit, MappingSource } from '@aws-cdk/toolkit-lib'; +import type { + DeploymentMethod, + ToolkitAction, + ToolkitOptions, +} from '@aws-cdk/toolkit-lib'; +import { + parseMappingGroups, + StackSelectionStrategy, ToolkitError, PermissionChangeType, Toolkit, +} from '@aws-cdk/toolkit-lib'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; import * as fs from 'fs-extra'; @@ -1216,12 +1223,8 @@ export class CdkToolkit { } public async refactor(options: RefactorOptions): Promise { - if (options.mappingFile && options.excludeFile) { - throw new ToolkitError('Cannot use both --exclude-file and mapping-file.'); - } - - if (options.revert && !options.mappingFile) { - throw new ToolkitError('The --revert option can only be used with the --mapping-file option.'); + if (options.revert && !options.overrideFile) { + throw new ToolkitError('The --revert option can only be used with the --override-file option.'); } try { @@ -1229,7 +1232,7 @@ export class CdkToolkit { dryRun: options.dryRun, localStacks: options.localStacks, deployedStacks: options.deployedStacks, - mappingSource: await mappingSource(), + overrides: readOverrides(options.revert ?? false, options.overrideFile), }); } catch (e) { error((e as Error).message); @@ -1238,39 +1241,21 @@ export class CdkToolkit { return 0; - async function readMappingFile(filePath: string | undefined) { + function readOverrides(revert: boolean, filePath: string | undefined) { if (filePath == null) { - return undefined; + return []; } - if (!(await fs.pathExists(filePath))) { + if (!fs.pathExistsSync(filePath)) { throw new ToolkitError(`The mapping file ${filePath} does not exist`); } - const content = JSON.parse(fs.readFileSync(filePath).toString('utf-8')); - if (content.environments) { - return content.environments; - } else { - throw new ToolkitError(`The mapping file ${filePath} does not contain an \`environments\` array`); - } - } - - async function readExcludeFile(filePath: string | undefined) { - if (filePath != null) { - if (!(await fs.pathExists(filePath))) { - throw new ToolkitError(`The exclude file '${filePath}' does not exist`); - } - return fs.readFileSync(filePath).toString('utf-8').split('\n'); - } - return undefined; - } - - async function mappingSource(): Promise { - if (options.mappingFile != null) { - return MappingSource.explicit(await readMappingFile(options.mappingFile)); - } - if (options.revert) { - return MappingSource.reverse(await readMappingFile(options.mappingFile)); - } - return MappingSource.auto((await readExcludeFile(options.excludeFile)) ?? []); + const groups = parseMappingGroups(fs.readFileSync(filePath).toString('utf-8')); + + return revert + ? groups.map((group) => ({ + ...group, + resources: Object.fromEntries(Object.entries(group.resources ?? {}).map(([src, dst]) => [dst, src])), + })) + : groups; } } @@ -1959,23 +1944,8 @@ export interface RefactorOptions { readonly dryRun: boolean; /** - * 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: - * - * - Stack name and logical ID (e.g. `Stack1.MyQueue`) - * - A construct path (e.g. `Stack1/Foo/Bar/Resource`). - */ - excludeFile?: string; - - /** - * The absolute path to a file that contains an explicit mapping to - * be used by the toolkit (as opposed to letting the toolkit itself - * compute the mapping). This file should contain a JSON object with + * The absolute path to a file that contains overrides to the mappings + * computed by the CLI. This file should contain a JSON object with * the following format: * * { @@ -1997,7 +1967,7 @@ export interface RefactorOptions { * deployed, while the destination must refer to a location that is not already * occupied by any resource. */ - mappingFile?: string; + overrideFile?: string; /** * Modifies the behavior of the `mappingFile` option by swapping source and diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index 0975f86a7..f01e23c47 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -450,15 +450,10 @@ export async function makeConfig(): Promise { desc: 'Do not perform any changes, just show what would be done', default: false, }, - 'exclude-file': { + 'override-file': { type: 'string', requiresArg: true, - desc: 'If specified, CDK will use the given file to exclude resources from the refactor', - }, - 'mapping-file': { - type: 'string', - requiresArg: true, - desc: 'A file that declares an explicit mapping to be applied. If provided, the command will use it instead of computing the mapping.', + desc: 'A file that declares overrides to be applied to the list of mappings computed by the CLI.', }, 'revert': { type: 'boolean', diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index a72d9bd33..e0352591b 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -276,8 +276,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('exclude-file', { + .option('override-file', { default: undefined, type: 'string', requiresArg: true, - desc: 'If specified, CDK will use the given file to exclude resources from the refactor', - }) - .option('mapping-file', { - default: undefined, - type: 'string', - requiresArg: true, - desc: 'A file that declares an explicit mapping to be applied. If provided, the command will use it instead of computing the mapping.', + desc: 'A file that declares overrides to be applied to the list of mappings computed by the CLI.', }) .option('revert', { default: false, diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 344e438d2..4c347bf3c 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1441,18 +1441,11 @@ export interface RefactorOptions { readonly dryRun?: boolean; /** - * If specified, CDK will use the given file to exclude resources from the refactor + * A file that declares overrides to be applied to the list of mappings computed by the CLI. * - * @default - undefined - */ - readonly excludeFile?: string; - - /** - * A file that declares an explicit mapping to be applied. If provided, the command will use it instead of computing the mapping. - * - * @default - undefined + * @default - no overrides */ - readonly mappingFile?: string; + readonly overrideFile?: string; /** * If specified, the command will revert the refactor operation. This is only valid if a mapping file was provided. From 9d57cf472b6e8868ea150f1e0ae4299e94701ff6 Mon Sep 17 00:00:00 2001 From: github-actions Date: Wed, 2 Jul 2025 12:36:21 +0000 Subject: [PATCH 03/13] chore: self mutation Signed-off-by: github-actions --- packages/aws-cdk/lib/cli/user-input.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 4c347bf3c..b835e6685 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1443,7 +1443,7 @@ export interface RefactorOptions { /** * A file that declares overrides to be applied to the list of mappings computed by the CLI. * - * @default - no overrides + * @default - undefined */ readonly overrideFile?: string; From d3fdb2b7928b949afef6daece23f404f0cd8183b Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Mon, 7 Jul 2025 10:24:02 +0100 Subject: [PATCH 04/13] Better message in case of modification; Show both mappings and ambiguities; --- .../toolkit-lib/lib/api/refactoring/context.ts | 13 +++++++++---- .../@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts | 13 ++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index 0de295785..60e991631 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -55,11 +55,16 @@ function resourceMoves(before: CloudFormationStack[], after: CloudFormationStack const digestsBefore = resourceDigests(before); const digestsAfter = resourceDigests(after); + const stackNames = (stacks: CloudFormationStack[]) => stacks.map((s) => s.stackName).sort().join(', '); if (!isomorphic(digestsBefore, digestsAfter)) { - const message = 'A refactor operation cannot add, remove or update resources. ' + - 'Only resource moves and renames are allowed. ' + - "Run 'cdk diff' to compare the local templates to the deployed stacks."; - throw new ToolkitError(message); + const message = [ + 'A refactor operation cannot add, remove or update resources. Only resource moves and renames are allowed. ', + 'Run \'cdk diff\' to compare the local templates to the deployed stacks.\n', + `Deployed stacks: ${stackNames(before)}`, + `Local stacks: ${stackNames(after)}`, + ]; + + throw new ToolkitError(message.join('\n')); } return Object.values(removeUnmovedResources(zip(digestsBefore, digestsAfter))); diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 3d5438546..524370d1e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -1083,13 +1083,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { const mappings = context.mappings.filter((m) => !exclude.isExcluded(m.destination)); - if (context.ambiguousPaths.length > 0) { - const paths = context.ambiguousPaths; - await notifyInfo(formatAmbiguousMappings(paths), { ambiguousPaths: paths }); - continue; - } - - if (mappings.length === 0) { + if (mappings.length === 0 && context.ambiguousPaths.length === 0) { await notifyInfo('Nothing to refactor.'); continue; } @@ -1099,6 +1093,11 @@ export class Toolkit extends CloudAssemblySourceBuilder { .filter(m => m.type !== 'AWS::CDK::Metadata'); await notifyInfo(formatTypedMappings(typedMappings), { typedMappings }); + + if (context.ambiguousPaths.length > 0) { + const paths = context.ambiguousPaths; + await notifyInfo(formatAmbiguousMappings(paths), { ambiguousPaths: paths }); + } } catch (e: any) { await notifyError(e.message, e); } From efe780b43e5ee37286c7993f4ec2b1c994f0548f Mon Sep 17 00:00:00 2001 From: github-actions Date: Tue, 8 Jul 2025 10:07:35 +0000 Subject: [PATCH 05/13] chore: self mutation Signed-off-by: github-actions --- packages/aws-cdk/lib/cli/cli-type-registry.json | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/lib/cli/cli-type-registry.json b/packages/aws-cdk/lib/cli/cli-type-registry.json index 8d397ac86..b3e67e3e2 100644 --- a/packages/aws-cdk/lib/cli/cli-type-registry.json +++ b/packages/aws-cdk/lib/cli/cli-type-registry.json @@ -896,11 +896,15 @@ }, "refactor": { "description": "Moves resources between stacks or within the same stack", - "arg": { - "name": "STACKS", - "variadic": true - }, "options": { + "local-stack": { + "type": "array", + "desc": "Filter to apply for stacks in the cloud assembly" + }, + "deployed-stack": { + "type": "array", + "desc": "Filter to apply for stacks deployed to the AWS account" + }, "dry-run": { "type": "boolean", "desc": "Do not perform any changes, just show what would be done", From 46be32195181872c3e67a9aa8a32eb6e6ae30191 Mon Sep 17 00:00:00 2001 From: github-actions Date: Tue, 8 Jul 2025 13:36:04 +0000 Subject: [PATCH 06/13] chore: self mutation Signed-off-by: github-actions --- packages/aws-cdk/lib/cli/cli-type-registry.json | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/aws-cdk/lib/cli/cli-type-registry.json b/packages/aws-cdk/lib/cli/cli-type-registry.json index b3e67e3e2..c50254a76 100644 --- a/packages/aws-cdk/lib/cli/cli-type-registry.json +++ b/packages/aws-cdk/lib/cli/cli-type-registry.json @@ -910,15 +910,10 @@ "desc": "Do not perform any changes, just show what would be done", "default": false }, - "exclude-file": { + "override-file": { "type": "string", "requiresArg": true, - "desc": "If specified, CDK will use the given file to exclude resources from the refactor" - }, - "mapping-file": { - "type": "string", - "requiresArg": true, - "desc": "A file that declares an explicit mapping to be applied. If provided, the command will use it instead of computing the mapping." + "desc": "A file that declares overrides to be applied to the list of mappings computed by the CLI." }, "revert": { "type": "boolean", From 282aed4c7c0a51382f492a4d946fc7b10dddf4db Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Thu, 10 Jul 2025 17:20:55 +0100 Subject: [PATCH 07/13] Resolve conflicts --- .../toolkit-lib/lib/actions/refactor/index.ts | 15 ---- .../toolkit-lib/lib/api/refactoring/index.ts | 2 - .../toolkit-lib/lib/toolkit/toolkit.ts | 24 +----- .../toolkit-lib/test/actions/refactor.test.ts | 76 ++----------------- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 23 +----- .../aws-cdk/lib/cli/cli-type-registry.json | 4 - packages/aws-cdk/lib/cli/cli.ts | 1 - .../aws-cdk/lib/cli/convert-to-user-input.ts | 1 - 8 files changed, 13 insertions(+), 133 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 9766f377b..be8091380 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -71,21 +71,6 @@ export interface RefactorOptions { */ readonly dryRun?: boolean; - /** - * Criteria for selecting stacks to deploy - * - * @default - All stacks - */ - stacks?: StackSelector; - - /** - * List of patterns for filtering deployed stacks. If no patterns are passed, - * then all stacks, except the bootstrap stacks are considered. If you want - * to consider all stacks (including bootstrap stacks), pass the wildcard - * '*'. - */ - deployedStacks?: string[]; - /** * List of overrides to be applied to resolve possible ambiguities in the * computed list of mappings. 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 711ab0ffc..a9b2f3dd2 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts @@ -6,8 +6,6 @@ import { } from '@aws-cdk/cloudformation-diff'; import type * as cxapi from '@aws-cdk/cx-api'; import type { StackSummary } from '@aws-sdk/client-cloudformation'; -import { minimatch } from 'minimatch'; -import { major } from 'semver'; import { deserializeStructure, indexBy } from '../../util'; import type { SdkProvider } from '../aws-auth/private'; import { Mode } from '../plugin'; diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 8132c5b94..408c44bb8 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -62,12 +62,9 @@ import { formatEnvironmentSectionHeader, formatTypedMappings, groupStacks, - groupStacks, - ManifestExcludeList, - usePrescribedMappings, } from '../api/refactoring'; -import { type CloudFormationStack, ResourceLocation, ResourceMapping } from '../api/refactoring/cloudformation'; -import type { ResourceMapping } from '../api/refactoring/cloudformation'; +import type { CloudFormationStack } from '../api/refactoring/cloudformation'; +import { ResourceMapping, ResourceLocation } from '../api/refactoring/cloudformation'; import { RefactoringContext } from '../api/refactoring/context'; import { ResourceMigrator } from '../api/resource-import'; import { tagsForStack } from '../api/tags/private'; @@ -1068,9 +1065,6 @@ export class Toolkit extends CloudAssemblySourceBuilder { const selectedStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS); const groups = await groupStacks(sdkProvider, selectedStacks.stackArtifacts, options.additionalStackNames ?? []); - const mappingSource = options.mappingSource ?? MappingSource.auto(); - const exclude = mappingSource.exclude.union(new ManifestExcludeList(assembly.cloudAssembly.manifest)); - for (let { environment, localStacks, deployedStacks } of groups) { await ioHelper.defaults.info(formatEnvironmentSectionHeader(environment)); @@ -1079,10 +1073,10 @@ export class Toolkit extends CloudAssemblySourceBuilder { environment, deployedStacks, localStacks, - mappings: await getUserProvidedMappings(environment), + overrides: getOverrides(environment, deployedStacks, localStacks), }); - const mappings = context.mappings.filter((m) => !exclude.isExcluded(m.destination)); + const mappings = context.mappings; if (mappings.length === 0 && context.ambiguousPaths.length === 0) { await ioHelper.defaults.info('Nothing to refactor.'); @@ -1108,16 +1102,6 @@ export class Toolkit extends CloudAssemblySourceBuilder { } } - async function getUserProvidedMappings(environment: cxapi.Environment): Promise { - return mappingSource.source == 'explicit' - ? usePrescribedMappings(mappingSource.groups.filter(matchesEnvironment), sdkProvider) - : undefined; - - function matchesEnvironment(g: MappingGroup): boolean { - return g.account === environment.account && g.region === environment.region; - } - } - function getOverrides(environment: cxapi.Environment, deployedStacks: CloudFormationStack[], localStacks: CloudFormationStack[]) { const mappingGroup = options.overrides ?.find(g => g.region === environment.region && g.account === environment.account); 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 1a11cbd43..2d0da9f51 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -1,6 +1,5 @@ import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; -import { MappingSource, type RefactorOptions, StackSelectionStrategy, Toolkit } from '../../lib'; -import { type RefactorOptions, Toolkit } from '../../lib'; +import { type RefactorOptions, StackSelectionStrategy, Toolkit } from '../../lib'; import { SdkProvider } from '../../lib/api/aws-auth/private'; import { builderFixture, TestIoHost } from '../_helpers'; import { mockCloudFormationClient, MockSdk } from '../_helpers/mock-sdk'; @@ -434,65 +433,6 @@ test('overrides can be used to resolve ambiguities', async () => { ); }); -test('detects modifications to the infrastructure', 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 resource would be refactored - OldName: { - Type: 'AWS::S3::Bucket', - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', - Metadata: { - 'aws:cdk:path': 'Stack1/OldName/Resource', - }, - }, - // But there is an additional resource that will prevent it - Queue: { - Type: 'AWS::S3::Queue', - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', - Metadata: { - 'aws:cdk:path': 'Stack1/Queue/Resource', - }, - }, - }, - }), - }); - - // WHEN - const cx = await builderFixture(toolkit, 'stack-with-bucket'); - await toolkit.refactor(cx, { - dryRun: true, - }); - - // THEN - expect(ioHost.notifySpy).toHaveBeenCalledWith( - expect.objectContaining({ - action: 'refactor', - level: 'info', - // ...so we don't see it in the output - message: expect.stringMatching(/Nothing to refactor/), - }), - ); -}); - test('fails when dry-run is false', async () => { const cx = await builderFixture(toolkit, 'stack-with-bucket'); await expect( @@ -561,16 +501,10 @@ test('filters stacks when stack selector is passed', async () => { const cx = await builderFixture(toolkit, 'two-different-stacks'); await toolkit.refactor(cx, { dryRun: true, - // ... this is the mapping we used, and now we want to revert it - mappingSource: MappingSource.reverse([ - { - account: '123456789012', - region: 'us-east-1', - resources: { - 'Stack1.OldLogicalID': 'Stack1.NewLogicalID', - }, - }, - ]), + stacks: { + patterns: ['Stack1'], + strategy: StackSelectionStrategy.PATTERN_MATCH, + }, }); // Resources were renamed in both stacks, but we are only including Stack1. diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 6f797b39d..0a355f69a 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -2,17 +2,8 @@ import * as path from 'path'; import { format } from 'util'; import { RequireApproval } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; -import type { - DeploymentMethod, - ToolkitAction, - ToolkitOptions, -} from '@aws-cdk/toolkit-lib'; -import { - parseMappingGroups, - StackSelectionStrategy, ToolkitError, PermissionChangeType, Toolkit, -} from '@aws-cdk/toolkit-lib'; import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib'; -import { MappingSource, PermissionChangeType, Toolkit, ToolkitError } from '@aws-cdk/toolkit-lib'; +import { parseMappingGroups, PermissionChangeType, Toolkit, ToolkitError } from '@aws-cdk/toolkit-lib'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; import * as fs from 'fs-extra'; @@ -22,13 +13,7 @@ import { CliIoHost } from './io-host'; import type { Configuration } from './user-configuration'; import { PROJECT_CONFIG } from './user-configuration'; import { asIoHelper, cfnApi, tagsForStack } from '../../lib/api-private'; -import type { - AssetBuildNode, - AssetPublishNode, - Concurrency, - StackNode, - WorkGraph, -} from '../api'; +import type { AssetBuildNode, AssetPublishNode, Concurrency, StackNode, WorkGraph } from '../api'; import { CloudWatchLogEventMonitor, DEFAULT_TOOLKIT_STACK_NAME, @@ -1257,8 +1242,8 @@ export class CdkToolkit { await this.toolkit.refactor(this.props.cloudExecutable, { dryRun: options.dryRun, stacks: { - patterns: options.selector.patterns, - strategy: options.selector.patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, + patterns: patterns, + strategy: patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, }, overrides: readOverrides(options.revert ?? false, options.overrideFile), }); diff --git a/packages/aws-cdk/lib/cli/cli-type-registry.json b/packages/aws-cdk/lib/cli/cli-type-registry.json index 7c910e2e6..ae18038c7 100644 --- a/packages/aws-cdk/lib/cli/cli-type-registry.json +++ b/packages/aws-cdk/lib/cli/cli-type-registry.json @@ -896,10 +896,6 @@ }, "refactor": { "description": "Moves resources between stacks or within the same stack", - "arg": { - "name": "STACKS", - "variadic": true - }, "options": { "additional-stack-name": { "type": "array", diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index f627b9065..e0efada18 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -276,7 +276,6 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise Date: Fri, 11 Jul 2025 09:04:11 +0100 Subject: [PATCH 08/13] Fix missing property after conflict resolution --- .../toolkit-lib/test/actions/refactor.test.ts | 18 +++++++++--------- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) 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 2d0da9f51..01b99a1ce 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -348,6 +348,15 @@ test('detects modifications to the infrastructure', async () => { ); }); +test('fails when dry-run is false', async () => { + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await expect( + toolkit.refactor(cx, { + dryRun: false, + }), + ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); +}); + test('overrides can be used to resolve ambiguities', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ @@ -433,15 +442,6 @@ test('overrides can be used to resolve ambiguities', async () => { ); }); -test('fails when dry-run is false', async () => { - const cx = await builderFixture(toolkit, 'stack-with-bucket'); - await expect( - toolkit.refactor(cx, { - dryRun: false, - }), - ).rejects.toThrow('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.'); -}); - test('filters stacks when stack selector is passed', async () => { // GIVEN mockCloudFormationClient.on(ListStacksCommand).resolves({ diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 0a355f69a..2c1dd8e80 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1245,6 +1245,7 @@ export class CdkToolkit { patterns: patterns, strategy: patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, }, + additionalStackNames: options.additionalStackNames, overrides: readOverrides(options.revert ?? false, options.overrideFile), }); } catch (e) { From 66454f37e5b5bbf86ca65c551d901bd25215e729 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 11 Jul 2025 09:19:50 +0100 Subject: [PATCH 09/13] README --- packages/aws-cdk/README.md | 47 ++++++++++--------------- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 4 +-- 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 91df966ec..ba02d61ef 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -1129,22 +1129,27 @@ 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 your application has more than one stack, and you want the `refactor` -command to consider only a subset of them, you can specify the stacks you -want, both local and deployed: +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 --local-stack Foo --local-stack Bar --deployed-stack Foo --unstable=refactor --dry-run +$ cdk refactor Web* --unstable=refactor --dry-run ``` -This is useful if, for example, you have more than one CDK application deployed -to a given environment, and you want to only include the deployed stacks that -belong to the application that you are refactoring. - 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. +The CLI's default behavior is to include in the comparison only the deployed +stacks that have a counterpart (stack with the same name) locally. If you want +to include additional deployed stacks in the comparison, pass their names using +the `--additional-stack-name` option: + +```shell +$ cdk refactor --unstable=refactor --dry-run --additional-stack-name=Foo --additional-stack-name=Bar +``` + In case of ambiguities, the CLI will display a table like this: ``` @@ -1163,19 +1168,6 @@ Detected ambiguities: You can resolve this ambiguity manually, by passing an override file via the `--override-file=` CLI option. This file should contain a JSON object with the following structure: -The CLI's default behavior is to include in the comparison only the deployed -stacks that have a counterpart (stack with the same name) locally. If you want -to include additional deployed stacks in the comparison, pass their names using -the `--additional-stack-name` option: - -```shell -$ cdk refactor --unstable=refactor --dry-run --additional-stack-name=Foo --additional-stack-name=Bar -``` - -If, instead of letting the CLI decide which resources to move, you want to -provide your own mapping of old to new locations, you can do so by passing a -mapping file to the CLI via the `--mapping-file` flag. This file should -contain a JSON object with the following format: ```json { @@ -1184,22 +1176,19 @@ contain a JSON object with the following format: "account": "123456789012", "region": "us-east-1", "resources": { - "Foo.OldName": "Bar.NewName" + "Stack2.OldName": "Stack2.NewName" } } ] } ``` -where `resources` is a mapping of resources from source to destination -locations for a given environment. Resource locations are in the format -`StackName.LogicalId`.The source must refer to a location where there is a -resource currently deployed, while the destination must refer to a location +where `resources` is a mapping of resources from source to destination +locations for a given environment. Resource locations are in the format +`StackName.LogicalId`.The source must refer to a location where there is a +resource currently deployed, while the destination must refer to a location that is not already occupied by any resource. -If you want to undo a refactor, you can use the `--revert` option in -conjunction with the `--override-file` option. It will apply the mapping in -reverse order (source becomes destination and vice versa). ### `cdk drift` diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 2c1dd8e80..a9a4283f0 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -1246,7 +1246,7 @@ export class CdkToolkit { strategy: patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS, }, additionalStackNames: options.additionalStackNames, - overrides: readOverrides(options.revert ?? false, options.overrideFile), + overrides: readOverrides(options.overrideFile, options.revert), }); } catch (e) { error((e as Error).message); @@ -1255,7 +1255,7 @@ export class CdkToolkit { return 0; - function readOverrides(revert: boolean, filePath: string | undefined) { + function readOverrides(filePath: string | undefined, revert: boolean = false) { if (filePath == null) { return []; } From f16deb6ca939fa6f461bfa4abd478ecb1a147a09 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 11 Jul 2025 13:23:33 +0100 Subject: [PATCH 10/13] feat: `diff` shows moved resources --- .../cloudformation-diff/lib/diff/types.ts | 8 ++ .../cloudformation-diff/lib/format.ts | 17 ++++- .../toolkit-lib/lib/actions/diff/index.ts | 8 ++ .../lib/actions/diff/private/helpers.ts | 10 +++ .../toolkit-lib/lib/actions/refactor/index.ts | 29 ++++++- .../lib/api/diff/diff-formatter.ts | 42 ++++++++++- .../lib/api/refactoring/cloudformation.ts | 9 ++- .../lib/api/refactoring/context.ts | 23 ++++-- .../toolkit-lib/lib/api/refactoring/index.ts | 1 + .../toolkit-lib/test/actions/diff.test.ts | 75 ++++++++++++++++++- .../toolkit-lib/test/api/diff/diff.test.ts | 23 ++++++ packages/aws-cdk/lib/cli/cdk-toolkit.ts | 18 ++++- packages/aws-cdk/lib/cli/cli-config.ts | 1 + .../aws-cdk/lib/cli/cli-type-registry.json | 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 | 5 ++ packages/aws-cdk/lib/cli/user-input.ts | 7 ++ 18 files changed, 265 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 52df47ff9..baa9dc062 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -547,6 +547,12 @@ export interface Resource { [key: string]: any; } +export interface Move { + readonly direction: 'from' | 'to'; + readonly stackName: string; + readonly resourceLogicalId: string; +} + /** * Change to a single resource between two CloudFormation templates * @@ -568,6 +574,8 @@ export class ResourceDifference implements IDifference { */ public isImport?: boolean; + public move?: Move; + /** Property-level changes on the resource */ private readonly propertyDiffs: { [key: string]: PropertyDifference }; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index dd76f92ca..a4331ff5b 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -1,6 +1,6 @@ import { format } from 'util'; import * as chalk from 'chalk'; -import type { DifferenceCollection, TemplateDiff } from './diff/types'; +import type { DifferenceCollection, Move, TemplateDiff } from './diff/types'; import { deepEqual } from './diff/util'; import type { Difference, ResourceDifference } from './diff-template'; import { isPropertyDifference, ResourceImpact } from './diff-template'; @@ -166,8 +166,15 @@ export class Formatter { const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType; - // eslint-disable-next-line @stylistic/max-len - this.print(`${this.formatResourcePrefix(diff)} ${this.formatValue(resourceType, chalk.cyan)} ${this.formatLogicalId(logicalId)} ${this.formatImpact(diff.changeImpact)}`.trimEnd()); + const message = [ + this.formatResourcePrefix(diff), + this.formatValue(resourceType, chalk.cyan), + this.formatLogicalId(logicalId), + this.formatImpact(diff.changeImpact), + this.formatMove(diff.move), + ].filter(Boolean).join(' '); + + this.print(message); if (diff.isUpdate) { const differenceCount = diff.differenceCount; @@ -239,6 +246,10 @@ export class Formatter { } } + private formatMove(move?: Move): string { + return !move ? '' : chalk.gray(`(moved ${move.direction} ${move.stackName}.${move.resourceLogicalId})`); + } + /** * Renders a tree of differences under a particular name. * @param name - the name of the root of the tree. diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts index 439c5ab9e..9e200d645 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts @@ -129,4 +129,12 @@ export interface DiffOptions { * @default 3 */ readonly contextLines?: number; + + /** + * Whether to include resource moves in the diff. These are the same moves that are detected + * by the `refactor` command. + * + * @default false + */ + readonly includeMoves?: boolean; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts index e790a61d5..af528c15c 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts @@ -13,6 +13,7 @@ import type { ResourcesToImport } from '../../../api/resource-import'; import { removeNonImportResources, ResourceMigrator } from '../../../api/resource-import'; import { ToolkitError } from '../../../toolkit/toolkit-error'; import { deserializeStructure, formatErrorMessage } from '../../../util'; +import { mappingsByEnvironment } from '../../refactor/index'; export function prepareDiff( ioHelper: IoHelper, @@ -67,6 +68,10 @@ async function cfnDiff( const templateInfos = []; const methodOptions = (options.method?.options ?? {}) as ChangeSetDiffOptions; + const allMappings = options.includeMoves + ? await mappingsByEnvironment(stacks.stackArtifacts, sdkProvider, true) + : []; + // Compare N stacks against deployed templates for (const stack of stacks.stackArtifacts) { const templateWithNestedStacks = await deployments.readCurrentTemplateWithNestedStacks( @@ -93,12 +98,17 @@ async function cfnDiff( methodOptions.importExistingResources, ) : undefined; + const mappings = allMappings.find(m => + m.environment.region === stack.environment.region && m.environment.account === stack.environment.account, + )?.mappings ?? {}; + templateInfos.push({ oldTemplate: currentTemplate, newTemplate: stack, isImport: !!resourcesToImport, nestedStacks, changeSet, + mappings, }); } 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 be8091380..64da26a38 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts @@ -1,6 +1,8 @@ +import type * as cxapi from '@aws-cdk/cx-api'; import type { StackSelector } from '../../api'; +import type { SdkProvider } from '../../api/aws-auth/sdk-provider'; import type { ExcludeList } from '../../api/refactoring'; -import { InMemoryExcludeList, NeverExclude } from '../../api/refactoring'; +import { groupStacks, InMemoryExcludeList, NeverExclude, RefactoringContext } from '../../api/refactoring'; import { ToolkitError } from '../../toolkit/toolkit-error'; type MappingType = 'auto' | 'explicit'; @@ -142,3 +144,28 @@ export function parseMappingGroups(s: string) { } } } + +export interface EnvironmentSpecificMappings { + readonly environment: cxapi.Environment; + readonly mappings: Record; +} + +export async function mappingsByEnvironment( + stackArtifacts: cxapi.CloudFormationStackArtifact[], + sdkProvider: SdkProvider, + ignoreModifications?: boolean, +): Promise { + const groups = await groupStacks(sdkProvider, stackArtifacts, []); + return groups.map((group) => { + const context = new RefactoringContext({ + ...group, + ignoreModifications, + }); + return { + environment: context.environment, + mappings: Object.fromEntries( + context.mappings.map((m) => [m.source.toLocationString(), m.destination.toLocationString()]), + ), + }; + }); +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/diff/diff-formatter.ts b/packages/@aws-cdk/toolkit-lib/lib/api/diff/diff-formatter.ts index efc777ffa..6adc19273 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/diff/diff-formatter.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/diff/diff-formatter.ts @@ -5,6 +5,7 @@ import { formatSecurityChanges, fullDiff, mangleLikeCloudFormation, + type ResourceDifference, type TemplateDiff, } from '@aws-cdk/cloudformation-diff'; import type * as cxapi from '@aws-cdk/cx-api'; @@ -122,6 +123,14 @@ export interface TemplateInfo { readonly nestedStacks?: { [nestedStackLogicalId: string]: NestedStackTemplates; }; + + /** + * Mappings of old locations to new locations. If these are provided, + * for all resources that were moved, their corresponding addition + * and removal lines will be augmented with the location they were + * moved fom and to, respectively. + */ + readonly mappings?: Record; } /** @@ -134,6 +143,7 @@ export class DiffFormatter { private readonly changeSet?: any; private readonly nestedStacks: { [nestedStackLogicalId: string]: NestedStackTemplates } | undefined; private readonly isImport: boolean; + private readonly mappings: Record; /** * Stores the TemplateDiffs that get calculated in this DiffFormatter, @@ -148,6 +158,7 @@ export class DiffFormatter { this.changeSet = props.templateInfo.changeSet; this.nestedStacks = props.templateInfo.nestedStacks; this.isImport = props.templateInfo.isImport ?? false; + this.mappings = props.templateInfo.mappings ?? {}; } public get diffs() { @@ -159,16 +170,38 @@ export class DiffFormatter { * If it creates the diff, it stores the result in a map for * easier retrieval later. */ - private diff(stackName?: string, oldTemplate?: any) { + private diff(stackName?: string, oldTemplate?: any, mappings: Record = {}) { const realStackName = stackName ?? this.stackName; if (!this._diffs[realStackName]) { - this._diffs[realStackName] = fullDiff( + const templateDiff = fullDiff( oldTemplate ?? this.oldTemplate, this.newTemplate.template, this.changeSet, this.isImport, ); + + const setMove = (change: ResourceDifference, direction: 'from' | 'to', location?: string)=> { + if (location != null) { + const [sourceStackName, sourceLogicalId] = location.split('.'); + change.move = { + direction, + stackName: sourceStackName, + resourceLogicalId: sourceLogicalId, + }; + } + }; + + templateDiff.resources.forEachDifference((id, change) => { + const location = `${realStackName}.${id}`; + if (change.isAddition && Object.values(mappings).includes(location)) { + setMove(change, 'from', Object.keys(mappings).find(k => mappings[k] === location)); + } else if (change.isRemoval && Object.keys(mappings).includes(location)) { + setMove(change, 'to', mappings[location]); + } + }); + + this._diffs[realStackName] = templateDiff; } return this._diffs[realStackName]; } @@ -199,6 +232,7 @@ export class DiffFormatter { this.stackName, this.nestedStacks, options, + this.mappings, ); } @@ -207,8 +241,9 @@ export class DiffFormatter { stackName: string, nestedStackTemplates: { [nestedStackLogicalId: string]: NestedStackTemplates } | undefined, options: ReusableStackDiffOptions, + mappings: Record = {}, ) { - let diff = this.diff(stackName, oldTemplate); + let diff = this.diff(stackName, oldTemplate, mappings); // The stack diff is formatted via `Formatter`, which takes in a stream // and sends its output directly to that stream. To facilitate use of the @@ -279,6 +314,7 @@ export class DiffFormatter { nestedStack.physicalName ?? nestedStackLogicalId, nestedStack.nestedStackTemplates, options, + this.mappings, ); numStacksWithChanges += nextDiff.numStacksWithChanges; formattedDiff += nextDiff.formattedDiff; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation.ts index 5f2c51461..2c06e4e8d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/cloudformation.ts @@ -31,8 +31,7 @@ export class ResourceLocation { } public toPath(): string { - const stack = this.stack; - const resource = stack.template.Resources?.[this.logicalResourceId]; + const resource = this.stack.template.Resources?.[this.logicalResourceId]; const result = resource?.Metadata?.['aws:cdk:path']; if (result != null) { @@ -40,7 +39,11 @@ export class ResourceLocation { } // If the path is not available, we can use stack name and logical ID - return `${stack.stackName}.${this.logicalResourceId}`; + return this.toLocationString(); + } + + public toLocationString() { + return `${this.stack.stackName}.${this.logicalResourceId}`; } public getType(): string { diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index 76418bdfe..af9c0ce18 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -12,11 +12,12 @@ import { equalSets } from '../../util/sets'; */ type ResourceMove = [ResourceLocation[], ResourceLocation[]]; -export interface RefactorManagerOptions { +export interface RefactoringContextOptions { environment: Environment; localStacks: CloudFormationStack[]; deployedStacks: CloudFormationStack[]; overrides?: ResourceMapping[]; + ignoreModifications?: boolean; } /** @@ -27,9 +28,9 @@ export class RefactoringContext { private readonly ambiguousMoves: ResourceMove[] = []; public readonly environment: Environment; - constructor(props: RefactorManagerOptions) { + constructor(props: RefactoringContextOptions) { this.environment = props.environment; - const moves = resourceMoves(props.deployedStacks, props.localStacks); + const moves = resourceMoves(props.deployedStacks, props.localStacks, props.ignoreModifications); const [nonAmbiguousMoves, ambiguousMoves] = partitionByAmbiguity(props.overrides ?? [], moves); this.ambiguousMoves = ambiguousMoves; this._mappings = resourceMappings(nonAmbiguousMoves); @@ -48,15 +49,23 @@ export class RefactoringContext { } } -function resourceMoves(before: CloudFormationStack[], after: CloudFormationStack[]): ResourceMove[] { +function resourceMoves( + before: CloudFormationStack[], + after: CloudFormationStack[], + ignoreModifications: boolean = false, +): ResourceMove[] { const digestsBefore = resourceDigests(before); const digestsAfter = resourceDigests(after); - const stackNames = (stacks: CloudFormationStack[]) => stacks.map((s) => s.stackName).sort().join(', '); - if (!isomorphic(digestsBefore, digestsAfter)) { + const stackNames = (stacks: CloudFormationStack[]) => + stacks + .map((s) => s.stackName) + .sort() + .join(', '); + if (!(ignoreModifications || isomorphic(digestsBefore, digestsAfter))) { const message = [ 'A refactor operation cannot add, remove or update resources. Only resource moves and renames are allowed. ', - 'Run \'cdk diff\' to compare the local templates to the deployed stacks.\n', + "Run 'cdk diff' to compare the local templates to the deployed stacks.\n", `Deployed stacks: ${stackNames(before)}`, `Local stacks: ${stackNames(after)}`, ]; 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 a9b2f3dd2..5ed26b49a 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts @@ -17,6 +17,7 @@ import type { MappingGroup } from '../../actions'; import { ToolkitError } from '../../toolkit/toolkit-error'; export * from './exclude'; +export * from './context'; interface StackGroup { environment: cxapi.Environment; diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts index 4634fa2fc..1662b5812 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -1,4 +1,5 @@ import * as path from 'path'; +import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; import * as chalk from 'chalk'; import { DiffMethod } from '../../lib/actions/diff'; import * as awsauth from '../../lib/api/aws-auth/private'; @@ -7,7 +8,7 @@ import * as deployments from '../../lib/api/deployments'; import * as cfnApi from '../../lib/api/deployments/cfn-api'; import { Toolkit } from '../../lib/toolkit'; import { builderFixture, disposableCloudAssemblySource, TestIoHost } from '../_helpers'; -import { MockSdk, restoreSdkMocksToDefault, setDefaultSTSMocks } from '../_helpers/mock-sdk'; +import { mockCloudFormationClient, MockSdk, restoreSdkMocksToDefault, setDefaultSTSMocks } from '../_helpers/mock-sdk'; let ioHost: TestIoHost; let toolkit: Toolkit; @@ -87,6 +88,78 @@ describe('diff', () => { })); }); + const resources = { + OldLogicalID: { + Type: 'AWS::S3::Bucket', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { 'aws:cdk:path': 'Stack1/OldLogicalID/Resource' }, + }, + }; + + test('returns diff augmented with moves', 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(), + }, + ], + }); + + jest.spyOn(deployments.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockResolvedValue({ + deployedRootTemplate: { + Parameters: {}, + resources, + }, + nestedStacks: [] as any, + }); + + mockCloudFormationClient + .on(GetTemplateCommand, { + StackName: 'Stack1', + }) + .resolves({ + TemplateBody: JSON.stringify({ + Resources: resources, + }), + }); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + const result = await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + includeMoves: true, + }); + + // THEN + expect(result.Stack1).toMatchObject(expect.objectContaining({ + resources: { + diffs: expect.objectContaining({ + MyBucketF68F3FF0: expect.objectContaining({ + isAddition: true, + isRemoval: false, + oldValue: undefined, + move: { + direction: 'from', + resourceLogicalId: 'OldLogicalID', + stackName: 'Stack1', + }, + newValue: { + Type: 'AWS::S3::Bucket', + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { 'aws:cdk:path': 'Stack1/MyBucket/Resource' }, + }, + }), + }), + }, + })); + }); + test('returns multiple template diffs', async () => { // WHEN const cx = await builderFixture(toolkit, 'two-different-stacks'); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts index 9580ca7c2..b12b313cf 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts @@ -92,6 +92,29 @@ describe('formatStackDiff', () => { ); }); + test('formats differences showing resource moves', () => { + // WHEN + const formatter = new DiffFormatter({ + templateInfo: { + oldTemplate: {}, + newTemplate: mockNewTemplate, + mappings: { + 'test-stack.OldName': 'test-stack.Func', + }, + }, + }); + const result = formatter.formatStackDiff(); + + // THEN + expect(result.formattedDiff).toBeDefined(); + const sanitizedDiff = result.formattedDiff!.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').trim(); + expect(sanitizedDiff).toBe( + 'Stack test-stack\n' + + 'Resources\n' + + '[+] AWS::Lambda::Function Func (moved from test-stack.OldName)', + ); + }); + test('handles nested stack templates', () => { // GIVEN const nestedStacks = { diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index a9a4283f0..cf8405971 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -3,7 +3,7 @@ import { format } from 'util'; import { RequireApproval } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib'; -import { parseMappingGroups, PermissionChangeType, Toolkit, ToolkitError } from '@aws-cdk/toolkit-lib'; +import { parseMappingGroups, PermissionChangeType, Toolkit, ToolkitError, mappingsByEnvironment } from '@aws-cdk/toolkit-lib'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; import * as fs from 'fs-extra'; @@ -266,6 +266,10 @@ export class CdkToolkit { info(diff.formattedDiff); } } else { + const allMappings = options.includeMoves + ? await mappingsByEnvironment(stacks.stackArtifacts, this.props.sdkProvider, true) + : []; + // Compare N stacks against deployed templates for (const stack of stacks.stackArtifacts) { const templateWithNestedStacks = await this.props.deployments.readCurrentTemplateWithNestedStacks( @@ -322,6 +326,10 @@ export class CdkToolkit { } } + const mappings = allMappings.find(m => + m.environment.region === stack.environment.region && m.environment.account === stack.environment.account, + )?.mappings ?? {}; + const formatter = new DiffFormatter({ templateInfo: { oldTemplate: currentTemplate, @@ -329,6 +337,7 @@ export class CdkToolkit { changeSet, isImport: !!resourcesToImport, nestedStacks, + mappings, }, }); @@ -1537,6 +1546,13 @@ export interface DiffOptions { * @default false */ readonly importExistingResources?: boolean; + + /** + * Whether to include resource moves in the diff + * + * @default false + */ + readonly includeMoves?: boolean; } interface CfnDeployOptions { diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index 618bac9a2..8c84b4d1d 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -337,6 +337,7 @@ export async function makeConfig(): Promise { 'quiet': { type: 'boolean', alias: 'q', desc: 'Do not print stack name and default message when there is no diff to stdout', default: false }, 'change-set': { type: 'boolean', alias: 'changeset', desc: 'Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role.', default: true }, 'import-existing-resources': { type: 'boolean', desc: 'Whether or not the change set imports resources that already exist', default: false }, + 'include-moves': { type: 'boolean', desc: 'Whether to include moves in the diff', default: false }, }, }, 'drift': { diff --git a/packages/aws-cdk/lib/cli/cli-type-registry.json b/packages/aws-cdk/lib/cli/cli-type-registry.json index ae18038c7..313247409 100644 --- a/packages/aws-cdk/lib/cli/cli-type-registry.json +++ b/packages/aws-cdk/lib/cli/cli-type-registry.json @@ -719,6 +719,11 @@ "type": "boolean", "desc": "Whether or not the change set imports resources that already exist", "default": false + }, + "include-moves": { + "type": "boolean", + "desc": "Whether to include moves in the diff", + "default": false } } }, diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index e0efada18..9c42b0fa3 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -259,6 +259,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise): any { default: false, type: 'boolean', desc: 'Whether or not the change set imports resources that already exist', + }) + .option('include-moves', { + default: false, + type: 'boolean', + desc: 'Whether to include moves in the diff', }), ) .command('drift [STACKS..]', 'Detect drifts in the given CloudFormation stack(s)', (yargs: Argv) => diff --git a/packages/aws-cdk/lib/cli/user-input.ts b/packages/aws-cdk/lib/cli/user-input.ts index 52a6f5d7c..a55e109bb 100644 --- a/packages/aws-cdk/lib/cli/user-input.ts +++ b/packages/aws-cdk/lib/cli/user-input.ts @@ -1169,6 +1169,13 @@ export interface DiffOptions { */ readonly importExistingResources?: boolean; + /** + * Whether to include moves in the diff + * + * @default - false + */ + readonly includeMoves?: boolean; + /** * Positional argument for diff */ From 2275ef8cc56b723fd94c782354403a546d97a2b8 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 15 Jul 2025 14:43:00 +0100 Subject: [PATCH 11/13] Improve message --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 2 +- packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index a4331ff5b..6ce1bfa4b 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -247,7 +247,7 @@ export class Formatter { } private formatMove(move?: Move): string { - return !move ? '' : chalk.gray(`(moved ${move.direction} ${move.stackName}.${move.resourceLogicalId})`); + return !move ? '' : chalk.gray('(OR', chalk.italic(chalk.bold('move')), `${move.direction} ${move.stackName}.${move.resourceLogicalId} via refactoring)`); } /** diff --git a/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts index b12b313cf..8214e7493 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/diff/diff.test.ts @@ -111,7 +111,7 @@ describe('formatStackDiff', () => { expect(sanitizedDiff).toBe( 'Stack test-stack\n' + 'Resources\n' + - '[+] AWS::Lambda::Function Func (moved from test-stack.OldName)', + '[+] AWS::Lambda::Function Func (OR move from test-stack.OldName via refactoring)', ); }); From 7c6a3be9e62165b68a5d7c9816315e42b15b5d5b Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 15 Jul 2025 15:14:03 +0100 Subject: [PATCH 12/13] Fix after merge --- packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index d8e64352c..5ad66625b 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -71,7 +71,7 @@ export class RefactoringContext { * */ function structuralOverrides(deployedStacks: CloudFormationStack[], localStacks: CloudFormationStack[]): ResourceMapping[] { - const moves = resourceMoves(deployedStacks, localStacks, 'opposite'); + const moves = resourceMoves(deployedStacks, localStacks, 'opposite', true); const [nonAmbiguousMoves] = partitionByAmbiguity([], moves); return resourceMappings(nonAmbiguousMoves); } From 09951f96b54a4a2549308c811ea828f7946b6674 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 15 Jul 2025 16:05:32 +0100 Subject: [PATCH 13/13] gray -> yellow --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 6ce1bfa4b..4686debee 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -247,7 +247,7 @@ export class Formatter { } private formatMove(move?: Move): string { - return !move ? '' : chalk.gray('(OR', chalk.italic(chalk.bold('move')), `${move.direction} ${move.stackName}.${move.resourceLogicalId} via refactoring)`); + return !move ? '' : chalk.yellow('(OR', chalk.italic(chalk.bold('move')), `${move.direction} ${move.stackName}.${move.resourceLogicalId} via refactoring)`); } /**