Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,11 @@ export enum ArtifactMetadataEntryType {
* Represents tags of a stack.
*/
STACK_TAGS = 'aws:cdk:stack-tags',

/**
* Whether the resource should be skipped during refactoring.
*/
SKIP_REFACTOR = 'aws:cdk:skip-refactor',
Copy link
Contributor

@mrgrain mrgrain May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called exclude now? Or

Suggested change
SKIP_REFACTOR = 'aws:cdk:skip-refactor',
NO_REFACTOR = 'aws:cdk:no-refactor',

or

Suggested change
SKIP_REFACTOR = 'aws:cdk:skip-refactor',
DO_NOT_REFACTOR = 'aws:cdk:do-not-refactor',

}

/**
Expand Down
13 changes: 13 additions & 0 deletions packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,17 @@ export interface RefactorOptions {
* @default - all stacks
*/
stacks?: StackSelector;

/**
* A list of resources to skip during the refactor. Elements of
* this list should be _destination_ locations that should be skipped,
* i.e., the location to which a resource would be moved if the
* refactor were to happen.
*
* 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`).
*/
Copy link
Contributor

@mrgrain mrgrain May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also see other conversation around destination.

Suggested change
/**
* A list of resources to skip during the refactor. Elements of
* this list should be _destination_ locations that should be skipped,
* i.e., the location to which a resource would be moved if the
* refactor were to happen.
*
* 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`).
*/
/**
* A list of resources that will not be part of the refactor. Elements of
* this list must be the _destination_ locations that should be excluded,
* i.e., the location to which a resource would be moved if the
* refactor were to happen. This is how the resource is identified
* in the current local code base.
*
* 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`).
*/

exclude?: string[];
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { TypedMapping } from '@aws-cdk/cloudformation-diff';
import type * as cxapi from '@aws-cdk/cx-api';

export interface CloudFormationTemplate {
Expand All @@ -15,3 +16,54 @@ export interface CloudFormationStack {
readonly stackName: string;
readonly template: CloudFormationTemplate;
}

/**
* This class mirrors the `ResourceLocation` interface from CloudFormation,
* but is richer, since it has a reference to the stack object, rather than
* merely the stack name.
*/
export class ResourceLocation {
constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) {
}

public toPath(): string {
const stack = this.stack;
const resource = stack.template.Resources?.[this.logicalResourceId];
const result = resource?.Metadata?.['aws:cdk:path'];

if (result != null) {
return result;
}

// If the path is not available, we can use stack name and logical ID
return `${stack.stackName}.${this.logicalResourceId}`;
}

public getType(): string {
const resource = this.stack.template.Resources?.[this.logicalResourceId ?? ''];
return resource?.Type ?? 'Unknown';
}

public equalTo(other: ResourceLocation): boolean {
return this.logicalResourceId === other.logicalResourceId && this.stack.stackName === other.stack.stackName;
}
}

/**
* A mapping between a source and a destination location.
*/
export class ResourceMapping {
constructor(public readonly source: ResourceLocation, public readonly destination: ResourceLocation) {
}

public toTypedMapping(): TypedMapping {
return {
// the type is the same in both source and destination,
// so we can use either one
type: this.source.getType(),
sourcePath: this.source.toPath(),
destinationPath: this.destination.toPath(),
};
}
}

89 changes: 25 additions & 64 deletions packages/@aws-cdk/toolkit-lib/lib/api/refactoring/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import type { SdkProvider } from '../aws-auth/private';
import { Mode } from '../plugin';
import { StringWriteStream } from '../streams';
import type { CloudFormationStack } from './cloudformation';
import { ResourceMapping, ResourceLocation } from './cloudformation';
import { computeResourceDigests, hashObject } from './digest';
import { NeverExclude, type ExcludeList } from './skip';

export * from './skip';

/**
* Represents a set of possible movements of a resource from one location
Expand All @@ -33,56 +37,6 @@ export class AmbiguityError extends Error {
}
}

/**
* This class mirrors the `ResourceLocation` interface from CloudFormation,
* but is richer, since it has a reference to the stack object, rather than
* merely the stack name.
*/
export class ResourceLocation {
constructor(public readonly stack: CloudFormationStack, public readonly logicalResourceId: string) {
}

public toPath(): string {
const stack = this.stack;
const resource = stack.template.Resources?.[this.logicalResourceId];
const result = resource?.Metadata?.['aws:cdk:path'];

if (result != null) {
return result;
}

// If the path is not available, we can use stack name and logical ID
return `${stack.stackName}.${this.logicalResourceId}`;
}

public getType(): string {
const resource = this.stack.template.Resources?.[this.logicalResourceId ?? ''];
return resource?.Type ?? 'Unknown';
}

public equalTo(other: ResourceLocation): boolean {
return this.logicalResourceId === other.logicalResourceId && this.stack.stackName === other.stack.stackName;
}
}

/**
* A mapping between a source and a destination location.
*/
export class ResourceMapping {
constructor(public readonly source: ResourceLocation, public readonly destination: ResourceLocation) {
}

public toTypedMapping(): TypedMapping {
return {
// the type is the same in both source and destination,
// so we can use either one
type: this.source.getType(),
sourcePath: this.source.toPath(),
destinationPath: this.destination.toPath(),
};
}
}

function groupByKey<A>(entries: [string, A][]): Record<string, A[]> {
const result: Record<string, A[]> = {};

Expand Down Expand Up @@ -118,25 +72,27 @@ export function ambiguousMovements(movements: ResourceMovement[]) {
* Converts a list of unambiguous resource movements into a list of resource mappings.
*
*/
export function resourceMappings(movements: ResourceMovement[], stacks?: CloudFormationStack[]): ResourceMapping[] {
const predicate = stacks == null
? () => true
: (m: ResourceMapping) => {
// Any movement that involves one of the selected stacks (either moving from or to)
// is considered a candidate for refactoring.
const stackNames = [m.source.stack.stackName, m.destination.stack.stackName];
return stacks.some((stack) => stackNames.includes(stack.stackName));
};
export function resourceMappings(
movements: ResourceMovement[],
stacks?: CloudFormationStack[],
): 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));
};

return movements
.filter(([pre, post]) => pre.length === 1 && post.length === 1 && !pre[0].equalTo(post[0]))
.map(([pre, post]) => new ResourceMapping(pre[0], post[0]))
.filter(predicate);
.filter(stacksPredicate);
}

function removeUnmovedResources(
m: Record<string, ResourceMovement>,
): Record<string, ResourceMovement> {
function removeUnmovedResources(m: Record<string, ResourceMovement>): Record<string, ResourceMovement> {
const result: Record<string, ResourceMovement> = {};
for (const [hash, [before, after]] of Object.entries(m)) {
const common = before.filter((b) => after.some((a) => a.equalTo(b)));
Expand Down Expand Up @@ -196,6 +152,7 @@ function resourceDigests(stack: CloudFormationStack): [string, ResourceLocation]
export async function findResourceMovements(
stacks: CloudFormationStack[],
sdkProvider: SdkProvider,
skipList: ExcludeList = new NeverExclude(),
): Promise<ResourceMovement[]> {
const stackGroups: Map<string, [CloudFormationStack[], CloudFormationStack[]]> = new Map();

Expand All @@ -216,7 +173,11 @@ export async function findResourceMovements(
for (const [_, [before, after]] of stackGroups) {
result.push(...resourceMovements(before, after));
}
return result;

return result.filter(mov => {
const after = mov[1];
return after.every(l => !skipList.isExcluded(l));
});
}

async function getDeployedStacks(
Expand Down
112 changes: 112 additions & 0 deletions packages/@aws-cdk/toolkit-lib/lib/api/refactoring/skip.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file name should be exclude

Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import type { AssemblyManifest } from '@aws-cdk/cloud-assembly-schema';
import { ArtifactMetadataEntryType, ArtifactType } from '@aws-cdk/cloud-assembly-schema';
import type { ResourceLocation as CfnResourceLocation } from '@aws-sdk/client-cloudformation';
import type { ResourceLocation } from './cloudformation';

export interface ExcludeList {
isExcluded(location: ResourceLocation): boolean;
}

export class ManifestExcludeList implements ExcludeList {
private readonly skippedLocations: CfnResourceLocation[];

constructor(manifest: AssemblyManifest) {
this.skippedLocations = this.getSkippedLocations(manifest);
}

private getSkippedLocations(asmManifest: AssemblyManifest): CfnResourceLocation[] {
// First, we need to filter the artifacts to only include CloudFormation stacks
const stackManifests = Object.entries(asmManifest.artifacts ?? {}).filter(
([_, manifest]) => manifest.type === ArtifactType.AWS_CLOUDFORMATION_STACK,
);

const result: CfnResourceLocation[] = [];
for (let [stackName, manifest] of stackManifests) {
const locations = Object.values(manifest.metadata ?? {})
// Then pick only the resources in each stack marked with SKIP_REFACTOR
.filter((entries) =>
entries.some((entry) => entry.type === ArtifactMetadataEntryType.SKIP_REFACTOR && entry.data === true),
)
// Finally, get the logical ID of each resource
.map((entries) => {
const logicalIdEntry = entries.find((entry) => entry.type === ArtifactMetadataEntryType.LOGICAL_ID);
const location: CfnResourceLocation = {
StackName: stackName,
LogicalResourceId: logicalIdEntry!.data! as string,
};
return location;
});
result.push(...locations);
}
return result;
}

isExcluded(location: ResourceLocation): boolean {
return this.skippedLocations.some(
(loc) => loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId,
);
}
}

export class InMemoryExcludeList implements ExcludeList {
private readonly skippedLocations: CfnResourceLocation[];
private readonly skippedPaths: string[];

constructor(items: string[]) {
this.skippedLocations = [];
this.skippedPaths = [];

if (items.length === 0) {
return;
}

const locationRegex = /^[A-Za-z0-9]+\.[A-Za-z0-9]+$/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking: Can a valid path really never look like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An element of a path can look like this, but the full path to a resource has to include at least an element for the stack, so it will be at least <stack ID>/<construct ID> (in practice, at least <stack ID>/<construct ID>/Resource). But I realized that the other regex was wrong. So I removed it and now it's either the pattern <stack name>.<logical ID>, representing resource location, or anything else, that gets treated as a construct path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding that explanation as comment.


items.forEach((item: string) => {
if (locationRegex.test(item)) {
const [stackName, logicalId] = item.split('.');
this.skippedLocations.push({
StackName: stackName,
LogicalResourceId: logicalId,
});
} else {
this.skippedPaths.push(item);
}
});
}

isExcluded(location: ResourceLocation): boolean {
const containsLocation = this.skippedLocations.some((loc) => {
return loc.StackName === location.stack.stackName && loc.LogicalResourceId === location.logicalResourceId;
});

const containsPath = this.skippedPaths.some((path) => location.toPath() === path);
return containsLocation || containsPath;
}
}

export class UnionExcludeList implements ExcludeList {
constructor(private readonly skipLists: ExcludeList[]) {
}

isExcluded(location: ResourceLocation): boolean {
return this.skipLists.some((skipList) => skipList.isExcluded(location));
}
}

export class NeverExclude implements ExcludeList {
isExcluded(_location: ResourceLocation): boolean {
return false;
}
}

export class AlwaysExclude implements ExcludeList {
isExcluded(_location: ResourceLocation): boolean {
return true;
}
}

export function fromManifestAndExclusionList(manifest: AssemblyManifest, exclude?: string[]): ExcludeList {
return new UnionExcludeList([new ManifestExcludeList(manifest), new InMemoryExcludeList(exclude ?? [])]);
}

Loading