feat(lib)!: add asset pipeline foundation types and hash utilities - #371
Conversation
b33ffc7 to
9374683
Compare
|
@sakul-learning — picking up your review threads from #339 (comment) here, since the shapes have moved into this slice. On temporarily removing
|
|
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 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. |
2576788 to
86bb7ae
Compare
|
@jsteinich and @so0k, this is ready for review now fully based on the discussed from #380. |
86bb7ae to
d91e168
Compare
jsteinich
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
0e106fd to
7fa394c
Compare
so0k
left a comment
There was a problem hiding this comment.
nitpicks, didn't get through it all (left draft comments where I haven't personally reviewed it again) - don't want to block this
7fa394c to
3a42064
Compare
jsteinich
left a comment
There was a problem hiding this comment.
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.
jsteinich
left a comment
There was a problem hiding this comment.
One more, on a change against main rather than against an earlier state of this PR.
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`.
3a42064 to
ea8e3ab
Compare
Head branch was pushed to by a user without write access
b93d17d to
ea8e3ab
Compare
### 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
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").
### 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").
Related issue
Spin-off from #339
Description
Foundational layer for the asset pipeline, per the #380 scope discussion:
IAsset,AssetOptions,AssetHashType— identity types.TerraformAssetnowimplements IAssetand consumesAssetHashType:SOURCE(default) hashes the source,CUSTOMuses an explicitassetHash,OUTPUTthrows until bundling exists.IAssetPackaging+AssetPackaging.FILE/DIRECTORY/ZIP, withpack()taking aPackOptionsstruct (source,target,ignoreStrategy?) so it stays extensible without a breaking change.TerraformAssetpackages throughAssetPackagingand derives its on-disk layout and artifact name fromproducesDirectory/extensionrather than a local switch.StagedAsset(assetHash,path,isDirectory) — publisher input, replaces the earlierFileAssetSource.IIgnoreStrategy+ExcludeIgnoreStrategy, andAssetHash.of()for content identity without staging.ignores()takes anIgnoreQuery(relativePath,isDirectory) so.gitignore/.dockerignore-style directory-only patterns are expressible;pruneExcludedDirectorieslets a strategy opt into descending excluded directories for negation patterns; optionalcacheKeysupports future result caching.archiveSyncgained an exclude predicate to matchcopySync, so packaging and hashing agree on what's excluded.AssetHash.of()pins to the canonical hash scheme, resolves relative paths againstcdktf.json(matchingTerraformAsset), and throws if bothexcludeandignoreStrategyare passed.Held back per #380: Docker/container asset types (
DockerImageAssetSource,DockerImageAssetLocation,DockerCacheOption) andFileAssetLocation— 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,TerraformAssetintegration, and Docker bundling on top of these primitives.Breaking changes
TerraformAsset.assetHashis nowreadonly(required byimplements 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 desyncedassetHashfrompath.Checklist