Skip to content

fix(migrate): make the plan and next steps easier to read #2612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
notgitika wants to merge 1 commit into
base: refactor
Choose a base branch
from
Open

fix(migrate): make the plan and next steps easier to read #2612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/core/project/migrate/plan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ describe("deployImpact", () => {
logicalId: "Mem",
type: "AWS::BedrockAgentCore::Memory",
severity: "warning",
reason: "the v1 deploy updates it in place: Description changes",
reason: "Description changes",
},
]);
});
Expand Down
2 changes: 1 addition & 1 deletion src/core/project/migrate/replacements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ function update(logicalId: string, type: string, updates: PropertyUpdate[]): Sta
logicalId,
type,
severity: "warning",
reason: `the v1 deploy updates it in place: ${changes(updates.map((property) => property.name))}`,
reason: changes(updates.map((property) => property.name)),
};
}

Expand Down
82 changes: 53 additions & 29 deletions src/handlers/project/migrate/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,9 @@ describe("agentcore migrate, planning", () => {
await subject.run();

const out = subject.io.stdout();
// A blank line separates the summary from the progress lines on stderr.
expect(out).toStartWith(
"Migration plan: CompatA → v1 (target default, 998846730471/us-east-1)\n",
"\nMigration plan: CompatA → v1 (target default, 998846730471/us-east-1)\n",
);
expect(out).toMatch(/\n {2}Keeps the names of \d+ deployed resources \(runtime CompatAAgent, /);
expect(out).toContain(
Expand Down Expand Up @@ -261,8 +262,11 @@ describe("agentcore migrate, planning", () => {
const subject = cli();
await subject.run();
const out = subject.io.stdout();
expect(out).toContain(` ! ${CDK_LIBRARY_OVERRIDE_ENV} is set (testing only)`);
expect(out).toContain(` ${overrideInstallCommand(`file:${resolve("/tmp/lib.tgz")}`)}\n`);
// The preview notes the override after the plan; installing it is an after-migrating step.
expect(out).toMatch(
new RegExp(`\nFull plan: \\S+\n\n! ${CDK_LIBRARY_OVERRIDE_ENV} is set \\(testing only\\)`),
);
expect(out).not.toContain("npm install");
// The override's own long warnings stay in the report.
expect(out).not.toContain("Synthesized with @aws/agentcore-cdk from");
} finally {
Expand Down Expand Up @@ -707,11 +711,11 @@ describe("agentcore migrate, reports", () => {

const out = subject.io.stdout();
expect(out).toContain(
"Migrated. Review the changes with `git diff`, then:\n" +
` ! ${CDK_LIBRARY_OVERRIDE_ENV} is set (testing only): the migrated app uses the released\n` +
" library, so before `agentcore deploy` install the one this plan was made with:\n" +
" cd agentcore/cdk && npm install /tmp/lib.tgz --no-save\n" +
" agentcore deploy\n",
`Migrated. Review the changes with \`git diff\`. ${CDK_LIBRARY_OVERRIDE_ENV} is set ` +
"(testing only), and the migrated app uses the released CDK library, so install the one " +
"this plan was made with, then deploy, from the project root:\n" +
" npm install --prefix agentcore/cdk /tmp/lib.tgz --no-save\n" +
" agentcore deploy\n",
);
});

Expand Down Expand Up @@ -877,13 +881,13 @@ describe("report helpers", () => {

test("overrideInstallCommand uses a file override's path, quoting when needed", () => {
expect(overrideInstallCommand("file:/tmp/lib.tgz")).toBe(
"cd agentcore/cdk && npm install /tmp/lib.tgz --no-save",
"npm install --prefix agentcore/cdk /tmp/lib.tgz --no-save",
);
expect(overrideInstallCommand("@aws/agentcore-cdk@1.0.0-rc.4")).toBe(
"cd agentcore/cdk && npm install @aws/agentcore-cdk@1.0.0-rc.4 --no-save",
"npm install --prefix agentcore/cdk @aws/agentcore-cdk@1.0.0-rc.4 --no-save",
);
expect(overrideInstallCommand("file:/tmp/my lib.tgz")).toBe(
"cd agentcore/cdk && npm install '/tmp/my lib.tgz' --no-save",
"npm install --prefix agentcore/cdk '/tmp/my lib.tgz' --no-save",
);
});

Expand Down Expand Up @@ -1109,28 +1113,49 @@ describe("formatStackPlan", () => {
ready: false,
};

test("lists moves, the first v1 deploy's changes, and the verdict", () => {
test("lists moves, then a row per change the first v1 deploy makes", () => {
expect(formatStackPlan(stack)).toEqual([
"Stack AgentCore-P-default (target 'default', 123456789012/us-east-1)",
" Refactor: move 1 resource to new logical IDs",
" Stack refactor: moves 1 resource to new logical IDs; 1 resource keeps its logical ID",
" TYPE FROM TO",
" AWS::BedrockAgentCore::Memory OldMem NewMem",
" 1 resource keep their logical IDs",
" Then the first v1 deploy:",
" creates",
" AWS::CloudFormation::CustomResource Trigger2",
" deletes",
" AWS::CloudFormation::CustomResource Trigger1: recreated",
" replaces",
" warning AWS::ECR::Repository Repo: rebuilt",
" BLOCKER AWS::BedrockAgentCore::ConfigurationBundle Bundle: lost",
" updates in place",
" BLOCKER AWS::BedrockAgentCore::Gateway Gw: ProtocolType changes",
" Replacements: 1 resource that must be kept would be replaced or deleted",
" The new IDs are the ones the v1 CDK app gives these resources (CDK derives them from the",
" construct path). The refactor only renames them in the stack: each resource, its name,",
" and its data stay as they are.",
"",
" First v1 deploy: replaces 2 resources, deletes 1 resource (1 resource that must be kept); " +
"updates 1 resource in place; creates 1 resource",
" creates AWS::CloudFormation::CustomResource Trigger2",
" deletes AWS::CloudFormation::CustomResource Trigger1: recreated",
" replaces warning AWS::ECR::Repository Repo: rebuilt",
" replaces BLOCKER AWS::BedrockAgentCore::ConfigurationBundle Bundle: lost",
" updates BLOCKER AWS::BedrockAgentCore::Gateway Gw: ProtocolType changes",
" BLOCKER: the stack name changes",
]);
});

test("an expected in-place update has no warning label", () => {
const lines = formatStackPlan({
...stack,
added: [],
removed: [],
replaced: [],
blockers: [],
updated: [
{
type: "AWS::BedrockAgentCore::Runtime",
logicalId: "Rt",
severity: "warning",
reason: "EnvironmentVariables changes",
},
],
});
expect(lines).toContain(" First v1 deploy: replaces nothing; updates 1 resource in place");
expect(lines).toContain(
" updates AWS::BedrockAgentCore::Runtime Rt: EnvironmentVariables changes",
);
});

test("says when the deploy changes nothing", () => {
const quiet = formatStackPlan({
...stack,
Expand All @@ -1141,10 +1166,9 @@ describe("formatStackPlan", () => {
updated: [],
blockers: [],
});
expect(quiet).toContain(" Refactor: move 0 resources to new logical IDs");
expect(quiet).toContain(" creates, deletes, and replaces nothing");
expect(quiet.at(-1)).toBe(
" Replacements: 0 replacements; no resource that must be kept is replaced or deleted",
expect(quiet).toContain(
" Stack refactor: moves 0 resources to new logical IDs; 1 resource keeps its logical ID",
);
expect(quiet.at(-1)).toBe(" First v1 deploy: replaces nothing");
});
});
107 changes: 54 additions & 53 deletions src/handlers/project/migrate/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
type ProjectMigrator,
type StackMigrationPlan,
type StackRefactorResult,
type StackResource,
type StackResourceChange,
type RetainedStackResource,
} from "./types";
Expand Down Expand Up @@ -139,7 +140,8 @@ export const createMigrateProjectHandler = (config: MigrateProjectHandlerConfig)
? formatMigrationSummary(plan, planFile.path)
: // Without the report the summary would point nowhere: show everything.
`${formatMigrationPlan(plan)}\nWarning: couldn't save the full plan (${planFile.error})\n`;
io.stdout.write(`${summary}${footer ? `\n${footer}\n` : ""}`);
// A blank line sets the summary apart from the progress lines above it.
io.stdout.write(`\n${summary}${footer ? `\n${footer}\n` : ""}`);
};

if (!plan.ready) {
Expand All @@ -162,6 +164,8 @@ export const createMigrateProjectHandler = (config: MigrateProjectHandlerConfig)
return;
}

// Without the prompt's answer line, a blank line sets apply's progress apart from the plan.
if (flags.yes && !json) io.stderr.write("\n");
const result = await runWithProgress(
progressFrom((emit) =>
prepared.apply({
Expand Down Expand Up @@ -279,24 +283,22 @@ function listed(items: string[], limit = 5): string {

/**
* The command that installs the CDK library a plan was made with, run from the
* project root: the migrated app vends the released library, which a plan made
* with AGENTCORE_MIGRATE_CDK_LIBRARY doesn't match.
* project root (so `agentcore deploy` follows without changing directory): the
* migrated app vends the released library, which a plan made with
* AGENTCORE_MIGRATE_CDK_LIBRARY doesn't match.
*/
export function overrideInstallCommand(cdkLibraryOverride: string): string {
const spec = cdkLibraryOverride.startsWith("file:")
? cdkLibraryOverride.slice("file:".length)
: cdkLibraryOverride;
const quoted = /^[A-Za-z0-9@%+=:,./_-]+$/.test(spec) ? spec : `'${spec.replace(/'/g, "'\\''")}'`;
return `cd agentcore/cdk && npm install ${quoted} --no-save`;
return `npm install --prefix agentcore/cdk ${quoted} --no-save`;
}

function overrideWarningLines(cdkLibraryOverride: string, indent: string): string[] {
return [
`${indent}! ${CDK_LIBRARY_OVERRIDE_ENV} is set (testing only): the migrated app uses the released`,
`${indent} library, so before \`agentcore deploy\` install the one this plan was made with:`,
`${indent} ${overrideInstallCommand(cdkLibraryOverride)}`,
];
}
/** The preview's note on the override: installing the library comes after migrating. */
const OVERRIDE_PREVIEW_NOTE =
`! ${CDK_LIBRARY_OVERRIDE_ENV} is set (testing only): this plan uses that CDK library. ` +
"After migrating, you'll be shown how to install it before `agentcore deploy`.";

/** One line per thing that keeps the plan from being ready. */
export function planBlockers(plan: MigrationPlan): string[] {
Expand Down Expand Up @@ -355,8 +357,13 @@ function deployImpact(stack: StackMigrationPlan): string {
stack.replaced.length > 0 ? `replaces ${count(stack.replaced.length, "resource")}` : "",
deleted.length > 0 ? `deletes ${count(deleted.length, "resource")}` : "",
].filter(Boolean);
const mustKeep = [...stack.removed, ...stack.replaced].filter(
(change) => change.severity === "blocker",
).length;
const parts = [
lost.length === 0 ? "replaces nothing" : `${lost.join(", ")} (none that must be kept)`,
lost.length === 0
? "replaces nothing"
: `${lost.join(", ")} (${mustKeep === 0 ? "none" : count(mustKeep, "resource")} that must be kept)`,
stack.updated.length > 0 ? `updates ${count(stack.updated.length, "resource")} in place` : "",
stack.added.length > 0 ? `creates ${count(stack.added.length, "resource")}` : "",
].filter(Boolean);
Expand Down Expand Up @@ -449,8 +456,8 @@ export function formatMigrationSummary(plan: MigrationPlan, planPath: string): s
if (override !== undefined && warning.includes(CDK_LIBRARY_OVERRIDE_ENV)) continue;
lines.push(` ! ${brief(warning)}`);
}
if (override !== undefined) lines.push(...overrideWarningLines(override, " "));
lines.push("", `Full plan: ${planPath}`);
if (override !== undefined) lines.push("", OVERRIDE_PREVIEW_NOTE);
return `${lines.join("\n")}\n`;
}

Expand All @@ -460,9 +467,11 @@ function nextSteps(outcome: string, cdkLibraryOverride: string | undefined): str
return [`${outcome} Review the changes with \`git diff\`, then run \`agentcore deploy\`.`];
}
return [
`${outcome} Review the changes with \`git diff\`, then:`,
...overrideWarningLines(cdkLibraryOverride, "").map((line) => ` ${line}`),
" agentcore deploy",
`${outcome} Review the changes with \`git diff\`. ${CDK_LIBRARY_OVERRIDE_ENV} is set ` +
"(testing only), and the migrated app uses the released CDK library, so install the one " +
"this plan was made with, then deploy, from the project root:",
` ${overrideInstallCommand(cdkLibraryOverride)}`,
" agentcore deploy",
];
}

Expand Down Expand Up @@ -698,19 +707,22 @@ export function formatMigrationPlan(plan: MigrationPlan): string {
return `${lines.join("\n")}\n`;
}

/** One stack's refactor: what moves, then what the first v1 deploy changes, then the verdict. */
/** A retained resource for the summary: its kind and the ID to find it by. */
function retainedLabel(resource: RetainedStackResource): string {
return `${resource.type} ${resource.physicalId ?? resource.logicalId}`;
}

/** One stack's refactor: what moves, what stays outside the stack, then what the first v1 deploy does. */
export function formatStackPlan(stack: StackMigrationPlan): string[] {
const lines = [
`Stack ${stack.stackName} (target '${stack.target}', ${stack.account}/${stack.region})`,
];
const indent = (rows: string[]) => rows.map((row) => ` ${row}`);

lines.push(` Refactor: move ${count(stack.moves.length, "resource")} to new logical IDs`);
lines.push(
` Stack refactor: moves ${count(stack.moves.length, "resource")} to new logical IDs; ` +
`${count(stack.unchanged, "resource")} keep${stack.unchanged === 1 ? "s its" : " their"} logical ID${stack.unchanged === 1 ? "" : "s"}`,
);
if (stack.moves.length > 0) {
lines.push(
...indent(
Expand All @@ -719,12 +731,15 @@ export function formatStackPlan(stack: StackMigrationPlan): string[] {
stack.moves.map((move) => [move.type, move.from, move.to]),
),
),
" The new IDs are the ones the v1 CDK app gives these resources (CDK derives them from the",
" construct path). The refactor only renames them in the stack: each resource, its name,",
" and its data stay as they are.",
);
}
lines.push(` ${count(stack.unchanged, "resource")} keep their logical IDs`);
const retained = stack.retained ?? [];
if (retained.length > 0) {
lines.push(
"",
" Kept in AWS outside the stack: set to retain before the refactor, so the first v1 " +
"deploy leaves them, and their data, in place. v1 doesn't manage them afterwards.",
...indent(
Expand All @@ -747,47 +762,33 @@ export function formatStackPlan(stack: StackMigrationPlan): string[] {
);
}

lines.push(" Then the first v1 deploy:");
const changes = (label: string, entries: StackResourceChange[]) => {
if (entries.length === 0) return;
lines.push(` ${label}`);
// One row per change, the action first. An in-place update is expected, so it
// carries a label only when it blocks the migration.
lines.push("", ` First v1 deploy: ${deployImpact(stack)}`);
const rows = (
action: string,
entries: (StackResource & Partial<StackResourceChange>)[],
labelWarnings = true,
) => {
for (const entry of entries) {
lines.push(` ${severityLabel(entry)}${entry.type} ${entry.logicalId}: ${entry.reason}`);
const label =
entry.severity === "blocker" || (labelWarnings && entry.severity === "warning")
? severityLabel(entry.severity)
: "";
const reason = entry.reason ? `: ${entry.reason}` : "";
lines.push(` ${action.padEnd(8)} ${label}${entry.type} ${entry.logicalId}${reason}`);
}
};
if (stack.added.length > 0) {
lines.push(" creates");
for (const entry of stack.added) lines.push(` ${entry.type} ${entry.logicalId}`);
}
changes("deletes", stack.removed);
changes("replaces", stack.replaced);
changes("updates in place", stack.updated);
if (stack.added.length + stack.removed.length + stack.replaced.length === 0) {
lines.push(
stack.updated.length === 0
? " creates, deletes, and replaces nothing"
: " creates, deletes, and replaces nothing else",
);
}

const lost = [...stack.removed, ...stack.replaced].filter(
(change) => change.severity === "blocker",
).length;
lines.push(
lost === 0
? ` Replacements: ${count(stack.replaced.length, "replacement")}; no resource that must be kept is replaced or deleted`
: ` Replacements: ${count(lost, "resource")} that must be kept would be replaced or deleted`,
);
rows("creates", stack.added);
rows("deletes", stack.removed);
rows("replaces", stack.replaced);
rows("updates", stack.updated, false);
for (const blocker of stack.blockers) lines.push(` BLOCKER: ${blocker}`);
return lines;
}

function severityLabel(change: StackResourceChange): string {
return change.severity === "blocker"
? "BLOCKER "
: change.severity === "warning"
? "warning "
: "";
function severityLabel(severity: StackResourceChange["severity"]): string {
return severity === "blocker" ? "BLOCKER " : severity === "warning" ? "warning " : "";
}

function count(n: number, noun: string): string {
Expand Down
Loading