Skip to content

feat(lib)!: add asset staging core with no bundling - #433

Merged
jsteinich merged 1 commit into
open-constructs:mainfrom
eduardomourar:feat/asset-staging-core
Sep 16, 2026
Merged

jsteinich merged 1 commit into
open-constructs:mainfrom
eduardomourar:feat/asset-staging-core

Conversation

@eduardomourar

@eduardomourar eduardomourar commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Related issue

Spin-off from #339

Description

Second slice of the asset pipeline (foundation types landed in #371). Gives AssetHashType.OUTPUT, exclude, and extraHash real behavior in TerraformAsset, backed by a new AssetStaging construct — no Docker/bundling code, which stays scoped to #339.

  • AssetStaging — resolves SOURCE/CUSTOM/OUTPUT hashing (OUTPUT no longer throws: without a bundler, an asset's "output" is its source verbatim) with exclude/ignoreStrategy/extraHash, and a stage() method that packs content honoring the same exclusions used for hashing. Hashing happens eagerly in the constructor; the filesystem side effect is deferred to stage(), called from the owning construct's onSynthesize.
  • TerraformAsset gains exclude/extraHash config options. Routes through AssetStaging only when either is set, so existing simple usage is unchanged — no new construct child, no behavior change. Also fixes a latent gap: packing now receives the same ignore strategy used for hashing, so excluded files are actually absent from the staged/zipped output, not just the hash.
  • AssetStaging never changes packaging based on exclude/extraHash — a directory asset stays a directory unless the caller explicitly asks for ARCHIVE. This is a deliberate departure from the original feat(lib): Initial Asset Pipeline implementation #339 spike, which silently zipped directories once any advanced option was set.
  • Adds a per-App content-keyed cache for the SOURCE/OUTPUT hash walk (same asset referenced from multiple resources/stacks hashes once), and ASSET_HASH_SALT_CONTEXT_KEY for app-wide cache-busting alongside the existing per-asset extraHash. Both ported from feat(lib): Initial Asset Pipeline implementation #339, minus its Docker-bundling coupling.

Held back for #339: IAssetBundler, Docker bundling, and the ILocalBundling/bind-mount/volume-copy execution.

Not in scope here: the publisher layer (IAssetPublisher/AssetReference) and fixing TerraformAsset's staging directory to live inside the stack directory (#380's concern #1) — both pre-existing, separate concerns.

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 a required member omitsDirectoryEntries; external implementations must add it. Custom TerraformAsset assetHash values are now validated against /^[A-Za-z0-9_.-]+$/ and throw if they contain other characters (e.g. "feature/my-branch").

@eduardomourar
eduardomourar requested a review from a team as a code owner September 13, 2026 16:15
@eduardomourar eduardomourar changed the title Feat/asset staging core feat(lib): add AssetStaging core with no bundling Sep 13, 2026
@eduardomourar eduardomourar changed the title feat(lib): add AssetStaging core with no bundling feat(lib): add asset-staging core with no bundling Sep 13, 2026

@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.

AssetStaging itself looks good — hashing eager, stage(targetPath) taking the path from the caller, and the cache scoped per construct-tree root rather than module-global. Two notes on how TerraformAsset consumes it.

Comment thread packages/cdktn/src/terraform-asset.ts Outdated
Comment thread packages/cdktn/src/asset-staging.ts Outdated
@eduardomourar
eduardomourar force-pushed the feat/asset-staging-core branch from e148ca9 to f5673a2 Compare September 14, 2026 14:39
jsteinich pushed a commit that referenced this pull request Sep 15, 2026
### Related issue

 Follow-up from #380.

### Description

#380 claimed asset staging lands as a sibling of `stacks/`, breaking
once uploaded to a hosted runner. Traced it: `TerraformAsset` already
stages inside `stacks/<stack-id>/`, and has since before #371. The bug
only ever existed in the old, never-merged spike's `AssetStaging`, which
#433 didn't carry forward.

No code fix needed. Adds a regression test pinning this contract so it
can't regress silently later (e.g. in #339).

### Checklist

- [x] I have updated the PR title to match [CDKTN's style
guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1)
- [x] I have run the linter on my code locally
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the
[documentation](https://github.com/open-constructs/cdk-terrain-docs/tree/main/content)
if applicable
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works if applicable
- [x] New and existing unit tests pass locally with my changes

@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 points addressed — and the custom-packaging test pins exactly the right thing.

Two resulting behaviour changes need the breaking marker, same as #371 got:

  1. IAssetPackaging.omitsDirectoryEntries is a new required member on an interface that has been published since #371 (v0.25.0-pre.21 onward). jsii behavioural interfaces require implementers to supply every member, so an external implementation stops compiling.

  2. Custom assetHash values are now validated for every TerraformAsset. They were returned verbatim before; now SAFE_ASSET_HASH rejects anything outside [A-Za-z0-9_.-], so assetHash: "feature/my-branch" throws where it used to work. Right change — it just needs to reach the changelog.

No version cost to marking it: the manifest is at 0.24.0 with bump-minor-pre-major, and #371 already landed as feat(lib)!:, so this cycle targets 0.25.0 either way. Release-please applies the highest bump once rather than compounding per commit.

Second slice of the asset pipeline (foundation types landed in open-constructs#371).
Gives AssetHashType.OUTPUT, exclude, and extraHash real behavior in
TerraformAsset, backed by a new AssetStaging construct. No Docker/bundling.

BREAKING CHANGE: IAssetPackaging gains a required member
omitsDirectoryEntries; external implementations must add it. Custom
TerraformAsset assetHash values are now validated against /^[A-Za-z0-9_.-]+$/
and throw if they contain other characters (e.g. "feature/my-branch").
@eduardomourar
eduardomourar force-pushed the feat/asset-staging-core branch from f5673a2 to 65be865 Compare September 15, 2026 21:15
@eduardomourar eduardomourar changed the title feat(lib): add asset-staging core with no bundling feat(lib)!: add asset staging core with no bundling Sep 15, 2026
@jsteinich
jsteinich enabled auto-merge (squash) September 16, 2026 01:46
@jsteinich
jsteinich merged commit 44d8bd1 into open-constructs:main Sep 16, 2026
518 of 520 checks passed
@eduardomourar
eduardomourar deleted the feat/asset-staging-core branch September 16, 2026 08:27
@so0k so0k added the assets label Sep 17, 2026
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