Skip to content

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

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

feat(lib): add asset pipeline foundation types and hash utilities#371
eduardomourar wants to merge 1 commit into
open-constructs:mainfrom
eduardomourar:feat/asset-foundation

Conversation

@eduardomourar

Copy link
Copy Markdown

Related issue

Spin-off from #339

Description

Introduce the foundational layer for the asset pipeline:

  • Asset interfaces and enums: IAsset, AssetOptions, AssetHashType, FileAssetSource, DockerImageAssetSource, FileAssetPackaging, FileAssetLocation, DockerImageAssetLocation, DockerCacheOption
  • Hash utility enhancements in private/fs.ts: ExcludePredicate type, CopySyncOptions with shouldExclude support, excludeMatcher() for building predicates from pattern lists, shouldExclude threading through hashPath/copySync for both legacy and canonical schemes
  • Export assets module from package index
  • Unit tests for type contracts and test fixtures for hashing

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.

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

Introduce the foundational layer for the asset pipeline:

- Asset interfaces and enums: IAsset, AssetOptions, AssetHashType,
  FileAssetSource, DockerImageAssetSource, FileAssetPackaging,
  FileAssetLocation, DockerImageAssetLocation, DockerCacheOption
- Hash utility enhancements in private/fs.ts: ExcludePredicate type,
  CopySyncOptions with shouldExclude support, excludeMatcher() for
  building predicates from pattern lists, shouldExclude threading
  through hashPath/copySync for both legacy and canonical schemes
- Export assets module from package index
- Unit tests for type contracts and test fixtures for hashing

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

chore: fix
@eduardomourar
eduardomourar force-pushed the feat/asset-foundation branch from b33ffc7 to 9374683 Compare August 10, 2026 21:14
@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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants