Skip to content

feat(lib)!: add asset bundling extension interface - #442

Open
eduardomourar wants to merge 1 commit into
open-constructs:mainfrom
eduardomourar:feat/lib-asset-bundler
Open

eduardomourar wants to merge 1 commit into
open-constructs:mainfrom
eduardomourar:feat/lib-asset-bundler

Conversation

@eduardomourar

@eduardomourar eduardomourar commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

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 a bundler option.

  • IAssetBundler — bundle(options): BundleResult, plus optional bundlerKey (folded into the asset hash so a changed build changes identity) and outputFileName (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-filtered source and caller-owned outputDir.
  • ChainBundler — tries bundlers in order until one runs (try-local-else-docker); folds every leg's bundlerKey.
  • BundlerKey — builds a bundlerKey from ordered parts (of/add/withEnv), escaping the separator.
  • IAssetPackaging.acceptsDirectorySource.
  • bundler? on AssetStaging and TerraformAsset; AssetType.FILE is allowed with a file-producing bundler.

Staging: the bundler reads exactly the tree the hash covers (filtered input is materialized when exclude/ignoreStrategy is set); output shape is validated against the packaging; staging is single-use; scratch dirs are swept on exit and on SIGINT/SIGTERM.

Checklist

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

BREAKING CHANGE: IAssetPackaging gains acceptsDirectorySource as a required member. Custom implementations of the interface must add it.

@eduardomourar
eduardomourar requested a review from a team as a code owner September 16, 2026 14:46

@jsteinich jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/cdktn/src/asset-staging.ts
Comment thread packages/cdktn/src/asset-staging.ts Outdated
Comment thread packages/cdktn/src/assets.ts
@so0k so0k added the assets label Sep 17, 2026
@eduardomourar

Copy link
Copy Markdown
Contributor Author

@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 jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/cdktn/src/asset-staging.ts
Comment thread packages/cdktn/src/asset-staging.ts Outdated

@jsteinich jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eduardomourar eduardomourar changed the title feat(lib): add asset bundling extension interface feat(lib)!: add asset bundling extension interface Sep 18, 2026
@eduardomourar
eduardomourar force-pushed the feat/lib-asset-bundler branch 2 times, most recently from 122ee60 to 6da0277 Compare September 22, 2026 14:06

@jsteinich jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. BundleOptions.source is documented absolute, but terraform-asset.ts:134 stores path.relative(process.cwd(), absolutePath) for a relative path, and AssetStaging passes 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 in AssetStaging or drop the guarantee.
  2. exclude is documented as filtering what the bundler reads; the bundler is handed the raw source. It does reach identity via hashSource, 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:

  1. A second stage() rebuilds: eagerBuild is consumed, then the call falls through to the deferred path. Under OUTPUT the 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.
  2. 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:

  1. bundle()'s return is not validated, so a file-returning bundler silently bypasses the directory contract.
  2. The FILE-packaging guard throws with AssetStaging's id, so the message reads "TerraformAsset Staging" — the internal child rather than the user's construct.
  3. bundlerKey is 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.

@jsteinich jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/cdktn/src/assets.ts Outdated
Comment thread packages/cdktn/src/asset-staging.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants