feat(lib): add asset pipeline foundation types and hash utilities - #371
feat(lib): add asset pipeline foundation types and hash utilities#371eduardomourar wants to merge 2 commits into
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.
bd975c5 to
0e106fd
Compare
0e106fd to
7fa394c
Compare
Related issue
Spin-off from #339
Description
Foundational layer for the asset pipeline, per the #380 scope discussion:
IAsset,AssetOptions,AssetHashType— identity typesIAssetPackaging+AssetPackaging.FILE/DIRECTORY/ZIP, withpack()taking aPackOptionsstruct (source,target,ignoreStrategy?) so it stays extensible without a breaking changeStagedAsset(assetHash,path,isDirectory) — publisher input, replaces the earlierFileAssetSourceIIgnoreStrategy(with optionalcacheKeyfor future result caching) +ExcludeIgnoreStrategy, andAssetHash.of()for content identity without stagingarchiveSyncgained an exclude predicate to matchcopySync, so packaging and hashing can agree on what's excludedAssetHash.of()pins to the canonical hash scheme and throws ifexcludeandignoreStrategyare both passedTerraformAssetnow packages throughAssetPackaginginstead of its own switch statementHeld 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.Checklist