feat(carvel): add BOSH link support via job-spec-overlay and values-overlay sidecars - #663
Conversation
Add BOSHLinkConsumer and BOSHLinks types and a BoshLinks field to the Kilnfile struct so that tile authors can declare additional BOSH link consumptions in their Kilnfile for carvel tiles. Co-authored-by: Cursor <cursoragent@cursor.com>
- Extract buildRegistryDataSpec() helper to build the registry-data job.MF; it appends any bosh_links.consumes entries from the Kilnfile after the hardcoded cluster-info link - Read Kilnfile in generateBoshReleaseDir() to discover additional BOSH link declarations (missing Kilnfile or missing field is a no-op) - Update generateManifestTemplate() to accept optional overlayContent injected before YAML.dump(values), allowing tile authors to mutate the values hash using BOSH link objects at deploy time - Read <entry>.values-overlay.erb alongside each packageinstall YAML and embed its content in the generated ERB template Co-authored-by: Cursor <cursoragent@cursor.com>
|
I have a concern on bosh_links placement: should this live in Kilnfile? Kilnfile is a dependency manifest. It tracks which BOSH releases to fetch and where from (think Gemfile). bosh_links is deploy-time BOSH job config. Those are different concerns, and mixing them here will confuse anyone reading the Kilnfile expecting only release sourcing info. Three alternatives worth discussing: Option A — Sidecar file (matches this PR's own pattern) This PR already introduces *.values-overlay.erb as a co-located sidecar kiln auto-detects. Same idea works here: a registry-data-consumes.yml kiln reads and merges into the generated job spec. Keeps things co-located and Kilnfile clean. Option B — base.yml with a kiln extension stanza Option C — Dedicated carvel.yml Option A feels most natural given what's already in this PR. Open to other takes. |
|
@rizwanreza I like option A - any strong idea of how you'd describe the "overlay" section for the job spec vs the link definition section? I'll update the design/pr later today with this in mind |
…ec-overlay sidecar - Remove BOSHLinkConsumer/BOSHLinks types and BoshLinks field from cargo.Kilnfile; Kilnfile stays focused on dependency management - Add local boshLinkConsumer and jobSpecOverlay types to carvel package - Detect <entry>.job-spec-overlay.yml alongside each packageinstall in packageinstalls/; parse and accumulate consumes entries across all entries; pass to buildRegistryDataSpec() - Update buildRegistryDataSpec signature to use local boshLinkConsumer - Tests: add jobSpecOverlay parse tests; update buildRegistryDataSpec tests to use local type; remove TestKilnfile_ParsesBoshLinks Co-authored-by: Cursor <cursoragent@cursor.com>
|
@rizwanreza Updated — went with a variant of your Option A that mirrors the Instead of
|
…dateVariables Both branches added code after the progress() function: the PR branch added boshLinkConsumer/jobSpecOverlay types; main added validateVariables (from #664). Include both blocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| }) | ||
| }) | ||
|
|
||
| Context("Bake", func() { |
There was a problem hiding this comment.
🤖 AI Review
🟡 Should Fix
The integration test (Bake context) exercises the full pipeline via testdata/sample-tile but that test data doesn't include any .job-spec-overlay.yml or .values-overlay.erb sidecars. The unit tests for the helper functions are good, but there's no end-to-end coverage of the new sidecar loading path through generateBoshReleaseDir. Consider adding a variant of testdata/sample-tile (or a fixture file) that includes at least one job-spec-overlay.yml and verifying it appears in the generated spec file.
- Surface OS errors other than not-found for both sidecar reads (values-overlay.erb and job-spec-overlay.yml) instead of silently swallowing them - Return an error when a job-spec-overlay.yml file exists but contains malformed YAML - Deduplicate BOSH link names before generating registry-data job spec; BOSH rejects duplicate consumes entries - Use yaml.Marshal to emit the additional consumes block in buildRegistryDataSpec, preventing YAML injection from link name/type values - Add test-install.job-spec-overlay.yml to sample-tile testdata; extend e2e spec assertion to verify binding_cache link appears in generated job spec - Add unit tests for injection safety, deduplication contract, and malformed YAML Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rizwanreza
left a comment
There was a problem hiding this comment.
Just a minor thing but otherwise look great!
| seen[c.Name] = struct{}{} | ||
| deduped = append(deduped, c) | ||
| } | ||
| } |
There was a problem hiding this comment.
The deduplication keeps the first occurrence and discards the rest. If packageinstall A declares binding_cache with type: binding_cache and packageinstall B declares binding_cache with type: binding-cache-v2, B's type is silently dropped. The generated BOSH spec will use A's type with no indication that B's declaration was ignored. This would be a confusing bug to track down at deploy time.
There was a problem hiding this comment.
good call - we log this now in the output since I think its a reasonable decision but puts the information in the tile author's view during build. WDYT?
When two packageinstalls declare a BOSH link with the same name but different type or optional, the second was previously silently dropped. Now emits a WARNING via progress output so the user knows which definition won and which was ignored, making deploy-time confusion much easier to debug. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add explicit assertion on which entry is kept in the optional-conflict case, a multi-conflict test verifying one warning per extra occurrence, and a nil-input edge case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Introduce two per-packageinstall sidecar conventions that kiln auto-detects in
packageinstalls/:<entry>.job-spec-overlay.ymlregistry-datajob.MFkiln carvel baketime<entry>.values-overlay.erbYAML.dump(values); mutates the values hash via BOSH link objectsMissing sidecar files are silently skipped (no-op). Both sidecars are co-located with the packageinstall YAML they extend.
Motivation
Carvel tiles need to consume BOSH links from co-deployed products (e.g. the
binding_cachelink provided by CF'sloggr-syslog-binding-cachejob) to inject environment-specific runtime values at BOSH deploy time. Previously only the hardcodedcluster-infolink was available in the generated job spec. The binding-cache has no BOSH DNS alias, so its address can only be discovered via a BOSH link.Approach — co-located sidecars (mirrors the values-overlay.erb pattern)
Tile authors declare BOSH link consumptions in a
<name>.job-spec-overlay.ymlsidecar alongside their packageinstall YAML. kiln reads all such files and aggregates theirconsumes:entries into the generatedregistry-datajob spec. The schema mirrors the BOSH job specconsumes:block directly.Example
job-spec-overlay.yml:Example
values-overlay.erb:Kilnfile is not modified. It stays focused on dependency management (release sources, releases, stemcell). The
BOSHLinkConsumer/BOSHLinkstypes andBoshLinksfield from the initial commit have been removed.Backward Compatibility
*.job-spec-overlay.ymlfiles produce identical output to before (no-op)*.values-overlay.erbfiles produce identical output to before (no-op)cluster-infolink behaviour is preservedTest Plan
jobSpecOverlay— parses aconsumes:list, handles empty list, handles missing keybuildRegistryDataSpec— includes user-declared links after cluster-infobuildRegistryDataSpec— no additional links when no sidecars presentbuildRegistryDataSpec— optional flag rendered correctlygenerateManifestTemplatewith overlay — overlay content present and beforeYAML.dumpgenerateManifestTemplatewith empty overlay — still produces valid templatego test ./...— 0 failures (Docker integration test skipped due to no daemon)Made with Cursor