Repository navigation
feat(migrate): retain unsupported v0 resources automatically - #2598
Conversation
…, and simplify unsupported-resource output
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
The retain-before-refactor machinery is thoughtful and well-tested. Highlights:
retainResourcesis defensive at every step: it verifies the change set contains only the two retain-policy attribute changes on exactly the expected resources (retainChangesMismatch), deletes a mismatched set unexecuted, distinguishes CloudFormation refusal (execute-failed, stack unchanged) from a lost response (unknown,needs-repair), and re-reads the deployed template afterUPDATE_COMPLETEto confirm the policies really landed before letting the refactor proceed.- Journal/rerun semantics are correct.
retainingisn't a resumable state — on rerun,planre-reads the deployed template andresourcesToRetainseesalreadyRetained=truefor anything the earlier attempt actually persisted, soretaincollapses toundefinedand no duplicate update is attempted. New change set names are timestamp-unique, so leftovers from an unknown-outcome run don't collide. repairStepscorrectly usesretainChangeSet && !refactorIdto pick the retain-specific recovery message, and that message correctly notes the update only touched DeletionPolicy/UpdateReplacePolicy and nothing was deleted or replaced.- Tests mock only at the CFN SDK boundary (
fakeCloudFormation) and use temp dirs for FS — in line with the project's testing patterns.
A couple of very minor things noticed but not worth blocking on:
- In
retain.ts, thecreate-failedbranch setsdetails.refactorId: changeSetNameonStackRefactorErroreven thoughonCreatedwasn't called and no change set exists in AWS. Harmless (apply.ts only useserror.message), but slightly misleading if that detail is ever surfaced elsewhere. - An unknown-outcome retain that in reality succeeded leaves the executed change set on the stack after a successful rerun (the rerun creates a fresh one). Minor clutter, not a correctness problem.
Nothing requiring changes before merge.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #2598 +/- ##
============================================
- Coverage 96.55% 96.54% -0.01%
============================================
Files 697 698 +1
Lines 56670 57246 +576
============================================
+ Hits 54715 55271 +556
- Misses 1955 1975 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude Security Review: no high-confidence findings. (run) |
jariy17
left a comment
There was a problem hiding this comment.
It looks good. Just a small non blocking fix.
|
|
||
| /** v0 online eval configs that run insights, which migrate leaves outside the v1 project. */ | ||
| export function onlineInsightConfigs(v0: V0ProjectSpec): V0ProjectSpec["onlineEvalConfigs"] { | ||
| return v0.onlineEvalConfigs.filter((config) => config.insights !== undefined); |
There was a problem hiding this comment.
Can we check (config.insights?.length ?? 0) > 0 here?
For example, an evaluator config can have:
{
"evaluators": ["Builtin.Helpfulness"],
"insights": []
}This config uses an evaluator and has no insights. The current check treats it as insights-only because the field exists, so migration removes it from onlineEvalConfigs. It should stay in the migrated project.
Description
Follow-up to #2584 (
agentcore migrate). It preserves v0 resources that v1 does not manage, while keeping capacity-provider migrations blocked until v1 supports them.Retained outside the v1 stack
Migrate now handles these resources the same way:
They are removed from the migrated
agentcore.json. Before the stack refactor, migrate setsDeletionPolicyandUpdateReplacePolicytoRetainon each resource and its dropped supporting resources. The first v1 deploy removes them from the stack while leaving them in AWS.The retained-resource traversal follows CloudFormation references in both directions. This keeps resources that the retained resource depends on and resources that point back to it, such as an
AWS::IAM::Policyattached to a knowledge base service role.Capacity providers and runtimes that use them remain migration blockers.
References from resources that remain in v1
Knowledge-base gateway connector references cannot keep using a removed project knowledge-base name. Migrate now:
RetrieveandAgenticRetrieveStreamreferences to the unique deployed knowledge-base ID and shows the rewrite in the preview; orThis prevents the migrated project from retaining a dangling reference.
Retain change-set recovery
Other output behavior
--target.Related Issue
No public issue yet; follows up #2584.
Documentation PR
The user-facing migration guide is in progress separately.
How to test
agentcore migratecurrently needs the@aws/agentcore-cdksupport from aws/agentcore-l3-cdk-constructs#423. PointAGENTCORE_MIGRATE_CDK_LIBRARYat a build of that PR, then use a clean, committed v0 project containing the resources above.agentcore migratewithout applying and inspect both the terminal preview andagentcore/.cli/migrate/plan-<timestamp>.md.agentcore migrate --yes, then deploy v1. The retained resources should leave the stack withDELETE_SKIPPEDand remain in AWS.Type of Change
Testing
bun test— 4489 pass, 0 failbun run typecheckbun run lint:checkbun run format:checkbun run buildChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.