From 42358062e88bc4b82d37119ed57a5a6668c1b518 Mon Sep 17 00:00:00 2001 From: gitikavj Date: Fri, 16 Oct 2026 20:21:14 +0000 Subject: [PATCH] fix(migrate): make the plan and next steps easier to read - Blank line between the progress lines and the summary, and before apply's progress with --yes. - The preview shows the full plan link first; the AGENTCORE_MIGRATE_CDK_LIBRARY note is one line, and the install command appears only after migrating. - The install command runs from the project root (npm install --prefix agentcore/cdk), so `agentcore deploy` follows without changing directory. - The plan file's stack section is flatter: one row per change under a 'First v1 deploy' line, no 'warning' label on expected in-place updates, a note on where the new logical IDs come from, and no separate 'Replacements' verdict. --- src/core/project/migrate/plan.test.ts | 2 +- src/core/project/migrate/replacements.ts | 2 +- src/handlers/project/migrate/index.test.ts | 82 ++++++++++------ src/handlers/project/migrate/index.ts | 107 +++++++++++---------- 4 files changed, 109 insertions(+), 84 deletions(-) diff --git a/src/core/project/migrate/plan.test.ts b/src/core/project/migrate/plan.test.ts index d159c0c05..c93df6468 100644 --- a/src/core/project/migrate/plan.test.ts +++ b/src/core/project/migrate/plan.test.ts @@ -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", }, ]); }); diff --git a/src/core/project/migrate/replacements.ts b/src/core/project/migrate/replacements.ts index 7aa86e6a9..aae47bc35 100644 --- a/src/core/project/migrate/replacements.ts +++ b/src/core/project/migrate/replacements.ts @@ -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)), }; } diff --git a/src/handlers/project/migrate/index.test.ts b/src/handlers/project/migrate/index.test.ts index 96beec916..f8fb76d2d 100644 --- a/src/handlers/project/migrate/index.test.ts +++ b/src/handlers/project/migrate/index.test.ts @@ -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( @@ -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 { @@ -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", ); }); @@ -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", ); }); @@ -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, @@ -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"); }); }); diff --git a/src/handlers/project/migrate/index.ts b/src/handlers/project/migrate/index.ts index b641d9c92..45ffc535a 100644 --- a/src/handlers/project/migrate/index.ts +++ b/src/handlers/project/migrate/index.ts @@ -23,6 +23,7 @@ import { type ProjectMigrator, type StackMigrationPlan, type StackRefactorResult, + type StackResource, type StackResourceChange, type RetainedStackResource, } from "./types"; @@ -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) { @@ -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({ @@ -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[] { @@ -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); @@ -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`; } @@ -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", ]; } @@ -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( @@ -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( @@ -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)[], + 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 {