Skip to content

feat(lib)!: add asset pipeline foundation types and hash utilities - #371

Merged
jsteinich merged 1 commit into
open-constructs:mainfrom
eduardomourar:feat/asset-foundation
Sep 13, 2026
Merged

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

Conversation

@eduardomourar

@eduardomourar eduardomourar commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Related issue

Spin-off from #339

Description

Foundational layer for the asset pipeline, per the #380 scope discussion:

  • IAsset, AssetOptions, AssetHashType — identity types. TerraformAsset now implements IAsset and consumes AssetHashType: SOURCE (default) hashes the source, CUSTOM uses an explicit assetHash, OUTPUT throws until bundling exists.
  • IAssetPackaging + AssetPackaging.FILE/DIRECTORY/ZIP, with pack() taking a PackOptions struct (source, target, ignoreStrategy?) so it stays extensible without a breaking change. TerraformAsset packages through AssetPackaging and derives its on-disk layout and artifact name from producesDirectory/extension rather than a local switch.
  • StagedAsset (assetHash, path, isDirectory) — publisher input, replaces the earlier FileAssetSource.
  • IIgnoreStrategy + ExcludeIgnoreStrategy, and AssetHash.of() for content identity without staging. ignores() takes an IgnoreQuery (relativePath, isDirectory) so .gitignore/.dockerignore-style directory-only patterns are expressible; pruneExcludedDirectories lets a strategy opt into descending excluded directories for negation patterns; optional cacheKey supports future result caching.
  • archiveSync gained an exclude predicate to match copySync, so packaging and hashing agree on what's excluded.
  • AssetHash.of() pins to the canonical hash scheme, resolves relative paths against cdktf.json (matching TerraformAsset), and throws if both exclude and ignoreStrategy are passed.

Held back per #380: Docker/container asset types (DockerImageAssetSource, DockerImageAssetLocation, DockerCacheOption) and FileAssetLocation — these model synth-resolved locations that don't fit the publish-at-apply model this pipeline is built on.

This is the foundation PR (1 of 3) for the asset staging pipeline. Later PRs will add AssetStaging, TerraformAsset integration, and Docker bundling on top of these primitives.

Breaking changes

  • TerraformAsset.assetHash is now readonly (required by implements IAsset). The generated setter is removed in Java, C#, Python, and Go. It was only ever assigned at construction, so this removes an assignment that would have desynced assetHash from path.

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

@so0k

so0k commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@sakul-learning — picking up your review threads from #339 (comment) here, since the shapes have moved into this slice.

On temporarily removing extraHash / bundling / assetHashType and their index.ts exports

I follow the reasoning, but I'm not sure I agree with pulling them.

The split into stacked PRs is a review-ergonomics device — it exists to keep each diff small enough to review properly, not to create independent release boundaries. The slices are intended to land together, so the enums and option shapes won't reach consumers ahead of the implementation that gives them meaning. The cross-language commitment you describe is real, but it's incurred at release, and there's no release in between.

Worth noting too that your recommendation was explicitly conditional on finishing the unification being too large for #339's scope — and the unified fingerprinting has since landed in d212d56, standardising on the existing MD5-derived identity rather than introducing the second SHA-256 walker. So the dual-path compatibility migration you were guarding against shouldn't materialise.

Stating the assumption explicitly so it's on the record rather than implied: if this foundation slice would ever ship on its own, ahead of the slices that implement it, your concern applies in full and we should revisit.

On symlinks

Your catch on #339 was a good one, and it's worth noting that this slice is on the right side of it. private/fs.ts keeps the lstatSync()-based walkers, and the behaviour is now documented rather than incidental:

Symlinks are recreated as symlinks rather than dereferenced, which keeps the copy consistent with hashPath (it hashes links by their target) and makes dangling links and link cycles harmless.

The new shouldExclude predicate threads through both the legacy and canonical schemes without touching symlink handling, so the #321 baseline is preserved here.

One gap in the same area: the new fixtures under test/fs/fixtures/test1/ (a local symlink and a dangling absolute one) aren't referenced by anything — test/fs/ contains no test file at all. That looks like exactly where the symlink coverage should land, and having the fixtures pre-staged makes it cheap.

On test quality

You already made this point on #339 — that the SOURCE vs OUTPUT tests asserted only toBeDefined() or hash length rather than the advertised contract — so treat this as an ack rather than a new finding. It carries directly into this slice: assets-types.test.ts is 280 lines asserting enum string values and that struct literals compile, which would pass against any implementation. Meanwhile excludeMatcher() and the shouldExclude threading are the genuinely new logic in this PR and currently have no tests or callers.

That's a very fixable shape of problem, and the groundwork is already there. The matcher's contract is well documented — I checked its stated limitations against 22 edge cases locally (bare *, **, ! negation, leading /, ./ prefixes, dir vs dirty/file, case sensitivity) and it behaves exactly as the docstring says. A behavioural suite over those cases plus the staged fixtures would lock it in, and it's a better use of the slice than the type-shape assertions currently there.

@eduardomourar

Copy link
Copy Markdown
Contributor Author

Agreed. No release boundary between these slices, so keeping extraHash / bundling / assetHashType in place makes sense.

To back that up with real implementation, I'll fold in AssetStaging Core with no bundling: the AssetStaging class doing SOURCE/OUTPUT/CUSTOM hash resolution through the shared fingerprinting path (gated on canonicalAssetHashes, per the unification in d212d56), just without Docker bundling. That gives the types actual behavior instead of shipping ahead of it.

To keep that from bloating the PR, I'll move the Docker-specific bits — the docker* fields on DockerImageAssetSource and DockerCacheOption — into the Docker Bundling PR, since they've got no caller until bundling lands anyway.

So this PR ends up as: foundation types (minus Docker interfaces) + a working AssetStaging core (SOURCE/OUTPUT/CUSTOM, no bundling). Docker bundling PR gets the Docker interfaces plus the actual DockerImage/bind-mount/volume-copy execution.

@so0k and @jsteinich, could you both confirm if that is the approach we want to take? I know that is not the small API surface we had in mind, but, at least, we can release those slices separately if needed be.

@eduardomourar

Copy link
Copy Markdown
Contributor Author

@jsteinich and @so0k, this is ready for review now fully based on the discussed from #380.

@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 trimmed assets.ts and the behavioural asset-hash.test.ts are big improvements. Wiring TerraformAsset through PACKAGING_BY_TYPE also gives IAssetPackaging a real consumer rather than shipping it ahead of one, which is better than what the delta asked for, and removing the unreferenced test/fs/fixtures/ closes an earlier thread.

Four notes below. The pack() signature is the one I'd want settled before merge, since it is public API. The canonical pinning is a decision worth making explicitly. The last two are cheap now and expensive later.

Comment thread packages/cdktn/src/assets.ts Outdated
Comment thread packages/cdktn/src/asset-hash.ts
Comment thread packages/cdktn/src/asset-hash.ts
Comment thread packages/cdktn/src/ignore-strategy.ts

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

Thanks — all four items from the previous pass look good. PackOptions threads the strategy through to both copySync and archiveSync, and putting the shouldExclude check ahead of the lstatSync in the archive walk means excluding a directory correctly skips its whole subtree. The canonical pin, the conflicting-options throw, and cacheKey all read the way I'd hoped.

Two follow-ups on the new test, below. Both are fixed by the same rewrite.

Comment thread packages/cdktn/test/asset-hash.test.ts
@eduardomourar
eduardomourar force-pushed the feat/asset-foundation branch 3 times, most recently from 0e106fd to 7fa394c Compare September 6, 2026 00: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.

Looks good to me.
What do you think @so0k ?

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

nitpicks, didn't get through it all (left draft comments where I haven't personally reviewed it again) - don't want to block this

Comment thread packages/cdktn/src/ignore-strategy.ts Outdated
Comment thread packages/cdktn/src/asset-hash.ts
Comment thread packages/cdktn/src/asset-hash.ts Outdated
Comment thread packages/cdktn/src/asset-hash.ts Outdated
Comment thread packages/cdktn/src/assets.ts
Comment thread packages/cdktn/src/assets.ts
Comment thread packages/cdktn/src/private/fs.ts Outdated
Comment thread packages/cdktn/test/assets-types.test.ts

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

Went through the latest round. The IgnoreQuery struct and the pruneExcludedDirectories opt-out both look right — carrying isDirectory from the walker's lstat is the only way a .gitignore-style strategy can honour directory-only patterns, and negation genuinely does require descending into an excluded parent.

Also confirmed the TerraformAsset refactor is behaviour-preserving for all three existing types: ARCHIVE resolves to "archive" + ".zip" exactly as the old ARCHIVE_NAME did, FILE and DIRECTORY keep basename, and producesDirectory maps onto the old type === DIRECTORY checks in both the path getter and _onSynthesize. canonical-asset-hash.test.ts is purely additive — nothing existing was rewritten.

Two notes below.

Comment thread packages/cdktn/src/terraform-asset.ts
Comment thread packages/cdktn/src/ignore-strategy.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.

One more, on a change against main rather than against an earlier state of this PR.

Comment thread packages/cdktn/src/terraform-asset.ts
BREAKING CHANGE: TerraformAsset.assetHash is now readonly (required by
`implements IAsset`). The generated setter is removed in Java, C#, Python
and Go. It was assigned once at construction, so this only removes an
assignment that would have desynced `assetHash` from `path`.
@eduardomourar eduardomourar changed the title feat(lib): add asset pipeline foundation types and hash utilities feat(lib)!: add asset pipeline foundation types and hash utilities Sep 12, 2026
@jsteinich
jsteinich enabled auto-merge (squash) September 12, 2026 19:31
auto-merge was automatically disabled September 12, 2026 21:47

Head branch was pushed to by a user without write access

@jsteinich
jsteinich force-pushed the feat/asset-foundation branch from b93d17d to ea8e3ab Compare September 13, 2026 01:59
@jsteinich
jsteinich enabled auto-merge (squash) September 13, 2026 02:00
@jsteinich
jsteinich merged commit a008d12 into open-constructs:main Sep 13, 2026
518 of 520 checks passed
@eduardomourar
eduardomourar deleted the feat/asset-foundation branch September 13, 2026 14:32
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
eduardomourar added a commit to eduardomourar/cdk-terrain that referenced this pull request Sep 15, 2026
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").
jsteinich pushed a commit that referenced this pull request Sep 16, 2026
### 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 #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 #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

- [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

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