feat(project): add CDK Toolkit adapter - #2057
Conversation
GitHub Actions
Failure
github-actions
Bot
added
agentcore-harness-reviewing
a3da8e2 to
000e3b8
Compare
GitHub Actions
Failure
000e3b8 to
ef1f731
Compare
GitHub Actions
Failure
|
|
||
| export type CdkOutputs = Record<string, string>; | ||
|
|
||
| export type CdkRunner = (operation: CdkOperation, options: CdkRunOptions) => Promise<CdkOutputs>; |
There was a problem hiding this comment.
Just curious, did you consider making this a class-based adapter rather than exposing CdkRunner as a function? The function works fine here, but I'd like to understand what drove this choice.
ef1f731 to
c4723b3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/project-deploy-contract #2057 +/- ##
=============================================================
Coverage 97.15% 97.15%
=============================================================
Files 387 388 +1
Lines 23141 23202 +61
=============================================================
+ Hits 22482 22543 +61
Misses 659 659 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c4723b3 to
6c52205
Compare
6c52205 to
8b9d408
Compare
| outdir: DIST, | ||
| target: "node", | ||
| minify: MINIFY, | ||
| external: EXTERNAL, |
There was a problem hiding this comment.
This keeps @aws-cdk/toolkit-lib external for the npm bundle, but compile() below (the six platform binaries) doesn't pass external. So the moment #2058 makes this reachable from src/index.ts, Bun will statically bundle the toolkit into every standalone binary — and that breaks the exact thing the comment above warns about: the toolkit reads data files relative to its own package dir at runtime (e.g. lib/api/bootstrap/bootstrap-template.yaml), and that path doesn't exist inside a compiled binary. Marking it external in compile() won't rescue it either, since the binaries ship no node_modules to resolve against.
Since this PR is where both the dependency and the asymmetry come in, I'd rather we settle the compiled-binary deploy story here than discover it in #2058. A few options: embed the toolkit's data files through the existing asset pipeline, make deploy fail with a clear message on compiled binaries, or at the very least narrow the comment to say it only covers the npm bundle.
| patterns: [operation.stackName], | ||
| }, | ||
| }); | ||
| return result.stacks[0]?.outputs ?? {}; |
There was a problem hiding this comment.
PATTERN_MUST_MATCH_SINGLE throws on a non-match, so the only way stacks comes back empty is the toolkit's empty-template path — and in 1.38.2 that path will delete an existing stack and still return normally. When that happens, ?? {} quietly turns it into a success with no outputs, and a caller expecting RuntimeArn just gets undefined with nothing pointing at where it went. This branch also isn't covered by a test.
Suggest asserting result.stacks.length === 1 and throwing otherwise, then keeping ?? {} only for the genuine case where a stack is present but has no outputs.
8b9d408 to
33e58a0
Compare
The test assembled its own ValueContext with ProjectKey, JsonKey and a stub JsonRenderer, which re-implemented what createRootHandler installs — so it could pass while the real wiring was wrong. It now goes through createRootHandler with the fake manager injected via TestCoreClient, the same pattern project.test.ts and add/harness/index.test.ts use, exercising the real --json group flag, the real JSON renderer and the real withProject wrap. Verified by unwrapping deploy from withProject: both tests now fail, where the hand-built context passed. No fixture setup needed since the fake's resolve() returns a project. Also drops the CloudFormation wording from DeployResult.outputs. The shape is already backend-neutral; only the doc comment implied CDK.
Resolve the named aws-targets.json entry in FsProjectManager and pass the resolved account and region to the backend, mirroring build(). deploy() previously threw NotImplementedError before dispatching, which left CdkBackend.deploy unreachable and duplicated the same message in two places; the backend is now the single place that reports deployment as unimplemented. Narrow the TestCoreClient seam from a whole-manager override to a single backend override. Handler tests keep the real FsProjectManager, so target resolution and withProject run for real and only the deploy boundary is faked.
Match the name pattern the schema on main already enforces. The CDK normalizes underscores to hyphens, so accepting them here would let a target be written one way and deployed under another.
The root handler prints only `error.message`, so the zod detail that `FsReadWriteJson.read` put in `cause` never reached the user: a typo'd region or a duplicated target name in a hand-edited file produced only `Failed to deserialize file at "<path>"`, naming the file but not the field. DeserializationError now requires a `details` string and appends it to the message, and json.ts passes the prettified zod error (or the parse error) through. Fixing it in the IO layer rather than catching in `ProjectManager.deploy` covers every hand-edited file at once. Making `details` required rather than optional keeps the opaque message from being reintroduced. Tested at the handler level, since the schema tests pass in isolation without proving the detail reaches stderr.
33e58a0 to
d5e69ff
Compare
3 participants
Summary
@aws-cdk/toolkit-libStack
This is PR 3 of 4 decomposing #2001. The adapter is backend-local rather than part of shared
io. Review and merge bottom-up.typecheck, lint, format, test all pass