feat(lib)!: add asset bundling extension interface - #442
eduardomourar wants to merge 1 commit into
Conversation
jsteinich
left a comment
There was a problem hiding this comment.
The interface shape matches what #380 proposed — bundle(options): string returning the output directory, build deferred into the synth window, bundlers outside core. Worth noting one divergence: #380 said "OUTPUT hashing stays supported, documented as forcing an eager build"; this rejects it outright when a bundler is set. Same direction, but a stricter resolution that deserves explicit sign-off rather than reading as already-settled.
Three notes below. The first I verified end to end rather than reading off the code — two bundlers writing different output, consumed by a real resource referencing asset.path:
hashA / hashB 9FD43BDFF8DE0DE43D08F9C369E89522 identical
pathA / pathB assets/asset/9FD43BDF.../archive.zip identical
emitted cdk.tf.json byte-identical
staged artifact bytes differ
_onSynthesize removes the previous version folder before writing, so the old artifact is actively deleted and the new one written at the same path — it is not a stale file lingering, it is the artifact being replaced underneath a plan that shows nothing.
Test gaps while in here: nothing covers FILE packaging with a bundler (see below), nothing proves the scratch dir is actually cleaned, nothing covers a bundler that throws, and nothing exercises the documented "or a subdirectory of it" return.
4c28a75 to
35063d4
Compare
|
@jsteinich, you were right to want sign-off here, and after thinking about it I went the other way from my first pass: rather than reject OUTPUT, I built the eager-build machinery so it actually works. OUTPUT now builds once in the constructor, hashes the real output, and stage() reuses that build. So it matches #380 (OUTPUT supported, eager build) and lands the internals the local/Docker bundler PRs need — they just implement IAssetBundler and pick a hash type. SOURCE stays the deferred, skippable default; OUTPUT trades skippability for hashing real bytes, which is the point of it. Test gaps closed too: FILE+bundler, scratch cleanup, a throwing bundler, subdirectory return, and the OUTPUT build-once/reuse path. |
jsteinich
left a comment
There was a problem hiding this comment.
All three addressed, and implementing OUTPUT rather than rejecting it puts this back in line with what #380 landed on. The bundlerKey docs and the packBundlerOutput rationale both capture the reasoning well, and the new tests cover the gaps — including the throwing-bundler cleanup and the subdirectory return.
Two things the eager-build path introduces.
35063d4 to
c7dcf5c
Compare
jsteinich
left a comment
There was a problem hiding this comment.
Both addressed well. The exit sweep drops entries in cleanupScratch as well as removing the directory, so the set does not grow across repeated synths, and only the eager build registers — the SOURCE-path scratch stays inside its own try/finally. acceptsDirectorySource is the right property and retires the proxy. Moving the type validation ahead of AssetStaging is a good catch on top; the collapsed stat.isFile() !== (this.type === AssetType.FILE) is equivalent across all four cases.
One thing left: IAssetPackaging gains acceptsDirectorySource as a required member. #433 merged on the 16th, so the interface is already published with omitsDirectoryEntries — this is a second breaking change to a live behavioural interface, and neither commit here is marked.
No version cost, same as last time: the manifest is at 0.24.0 and #371 and #433 are both already in the cycle as feat(lib)!:, so the pending release is 0.25.0 either way.
Unrelated to this PR, for the record — the failing unit job is the new OpenTofu matrix cell from #439, which landed on main at 20:43 today and arrived here via the rebase:
tofu1.12.6 init exited with code 1
│ Error: Failed to install provider
│ Error while installing kreuzwerker/docker v2.16.0: the provider is not ...
- "registry.terraform.io/hashicorp/null"
+ "registry.opentofu.org/hashicorp/null"
The failures are in @cdktn/provider-schema and @cdktn/provider-generator; packages/cdktn passed 621/621 in the same job. #433 and #434 merged before that job existed and main does not run the workflow, so this PR looks like the first to exercise it — worth chasing on #439 rather than here.
122ee60 to
6da0277
Compare
jsteinich
left a comment
There was a problem hiding this comment.
Nine real bundlers (TerraConstructs local/esbuild/bind-mount/volume-copy/chain, buildkit, Rolldown) were ported onto this interface and run against real toolchains in sakul-learning/cdktn-bundler-harness — 108/108 scenarios pass, so this is not a "does not work" list. The DX-REPORT covers where an implementation still needed a workaround. It targets head 98b39e49, which is content-identical to the current head.
I checked the following against the code here rather than taking them from the report.
Contract mismatches — docs currently state something the code does not do. Cheapest to close, and the most likely to burn an implementer:
BundleOptions.sourceis documented absolute, butterraform-asset.ts:134storespath.relative(process.cwd(), absolutePath)for a relativepath, andAssetStagingpasses it through unresolved. That is the common case, and a bundler spawning a tool with its own cwd (docker-w, esbuild) resolves elsewhere. Either resolve inAssetStagingor drop the guarantee.excludeis documented as filtering what the bundler reads; the bundler is handed the raw source. It does reach identity viahashSource, so the two disagree in both directions — an excluded file that changes moves the artifact and not the hash, and copy-everything bundlers ship bytes the identity does not account for. Either materialise the filtered input or delete the claim.
Correctness:
- A second
stage()rebuilds:eagerBuildis consumed, then the call falls through to the deferred path. UnderOUTPUTthe hash came from the first build, so a non-deterministic bundler stages bytes that do not match its own hash. Asserting single-use or retaining the artifact both close it. - The exit sweep only registers
process.once("exit"), which Node does not run on SIGINT/SIGTERM — so Ctrl-C during a synth strands the scratch, which is exactly when the largest trees are left behind.
Smoothing:
bundle()'s return is not validated, so a file-returning bundler silently bypasses the directory contract.- The FILE-packaging guard throws with
AssetStaging's id, so the message reads "TerraformAsset Staging" — the internal child rather than the user's construct. bundlerKeyis the only config channel and it is optional and unvalidated; forgetting it silently reuses stale artifacts. Worth stating in the docs that it must serialise everything that can move the output, and shipping a helper so the convention has one implementation rather than one per bundler.
Probably follow-ups rather than this slice — both are design additions and both have documented workarounds: an archive-shaped artifact (AssetType.FILE + bundler is a hard reject, so archive-producing bundlers wrap in a directory and get a zip containing archive.zip), and a decline protocol so a third-party local bundler and a third-party Docker bundler can be composed instead of reimplemented together.
6da0277 to
6859b5d
Compare
jsteinich
left a comment
There was a problem hiding this comment.
All seven addressed. Materialising the filtered input rather than dropping the doc claim is the right call — the bundler now reads exactly the tree the hash was taken over. Worth knowing it costs a full tree copy per build when exclusions are set, which seems a fair trade for the two agreeing.
Two new ones from the additions.
6859b5d to
6488b53
Compare
Related issue
Spin-off from #339, per the core split in #380.
Description
Adds
IAssetBundler, the extension point for asset bundling. No implementation ships; bundlers live in their own packages and pass via abundleroption.IAssetBundler—bundle(options): BundleResult, plus optionalbundlerKey(folded into the asset hash so a changed build changes identity) andoutputFileName(names a single-file artifact).BundleResult/BundleOutputType— a bundler returns a directory or a single-file artifact, or declines (BundleResult.declined()) so bundlers can be composed.BundleOptions— absolute, already-exclude-filteredsourceand caller-ownedoutputDir.ChainBundler— tries bundlers in order until one runs (try-local-else-docker); folds every leg'sbundlerKey.BundlerKey— builds abundlerKeyfrom ordered parts (of/add/withEnv), escaping the separator.IAssetPackaging.acceptsDirectorySource.bundler?onAssetStagingandTerraformAsset;AssetType.FILEis allowed with a file-producing bundler.Staging: the bundler reads exactly the tree the hash covers (filtered input is materialized when
exclude/ignoreStrategyis set); output shape is validated against the packaging; staging is single-use; scratch dirs are swept on exit and on SIGINT/SIGTERM.Checklist
BREAKING CHANGE:
IAssetPackaginggainsacceptsDirectorySourceas a required member. Custom implementations of the interface must add it.