[Draft] Implement remote fetch, carvel publish, and hook synthesis for carvel tiles

[ichandrabhatta]

Summary

Extends kiln carvel bake so Carvel tiles can declare third-party BOSH releases (additional_releases) fetched from real remote sources via Kilnfile/Kilnfile.lock — instead of requiring pre-staged local tarballs — and auto-synthesizes pre/post-install hook adapter jobs directly into the tile's own release. Also brings carvel upload/publish/rebake up to the same standard as classic kiln bake: a cached, checksummed release in Artifactory, a reproducible .pivotal built from it, and a bake record for rebake to verify against later.

Key Features

1. Remote fetch for additional_releases

   • additional_releases: entries are now resolved via Kilnfile/Kilnfile.lock's release_sources: (fetchAdditionalReleases), reusing the same component.NewReleaseSourceRepo/DownloadRelease primitive classic kiln bake already uses — not a local tarball_path: anymore.
   • New carvel bake flags: --releases-directory (default releases), --skip-fetch (use whatever's already staged there), --from-lockfile (explicit opt-in to bake the tile's own release from a cached copy instead of regenerating it — see bug fix below for why this is now explicit).
   • Local SHA1 cache-hit check avoids re-downloading a release that's already present and matches the lock.

2. Auto-synthesis of pre/post-install hooks

   • pre_install_hooks:/post_install_hooks: in base.yml get a BOSH job auto-generated into the tile's own release (auto-prefixed with the tile's name to avoid collisions), depositing a single script at bin/hooks/{pre,post}-install that execs the declared command.
   • Eliminates the need for tile authors to maintain separate "hook adapter" BOSH release repositories.

3. carvel upload / publish / rebake + bake records

   • carvel upload builds the tile's own release, uploads it to Artifactory, and records it in Kilnfile.lock.
   • carvel publish downloads that cached release (by name, not by list position) and bakes the final .pivotal; --final writes a bake_records/<version>.json (git SHA + tile SHA256 + version).
   • carvel rebake reproduces a past release from its bake record and verifies the checksum matches.
   • Kilnfile.lock's upsert logic (writeStandardKilnfileLock/UpdateBOSHReleaseTarballLockWithName) now merges by release name instead of overwriting the whole file — the tile's own release and its additional_releases entries coexist in one lockfile.

Bug Fixes / Robustness (found and fixed during review)

• Critical: carvel bake's mode dispatch used to switch to "bake from cached copy" purely because Kilnfile.lock existed on disk — silently skipping hook/registry-data synthesis. Now gated behind explicit --from-lockfile.
• Critical: downloadCarvelRelease/generateRuntimeConfigs's cache/lock lookups used to assume "the tile's own release" was always Releases[0] — broke the moment additional_releases shared the same lockfile. Now resolved by name everywhere.
• A malformed Kilnfile.lock used to be silently swallowed (falls back to an empty lock, surfaces a confusing unrelated error later); now hard-fails immediately, while still tolerating a genuinely absent lockfile.
• additional_releases cached-file lookup assumed every release source names its downloaded tarball <name>-<version>.tgz; s3/artifactory sources actually write filepath.Base(RemotePath) (e.g. smoke-tests-4.12.14-ubuntu-jammy-1.1298.tgz), which broke --skip-fetch and the cache-hit check for those sources. Now resolves the real source type before deciding the filename convention.
• Hook validation (missing name/command) was only enforced on the bake path; publish/rebake (via BakeFromLockfile) skipped it. Now enforced on both.
• Restored a dropped diagnostic in update-release (the "-release/-boshrelease suffix" hint) and fixed a stale test that had silently stopped asserting on it.
• De-duplicated the hook job-name auto-prefix logic (previously implemented twice, at risk of drifting out of sync between job generation and runtime-config reference).
• Fixed several stale/self-contradicting code comments.
• Fixed 4 test call sites still using an old function signature, which had been failing go vet/go test outright.

Known residual gap (documented, not fixed here)

publish/rebake rebuild the runtime-config's hook/job references from whatever base.yml currently says, independent of what's actually inside the cached release tarball built by upload. If hooks change between upload and publish without re-running upload, the two can drift. Currently an operator contract ("re-run upload if you change hooks"), not enforced in code.

Technical Details

• internal/carvel/models/metadata.go: AdditionalReleases, PreInstallHooks, PostInstallHooks fields on Metadata; AdditionalRelease/AdditionalJob/HookDeclaration structs.
• internal/carvel/baker.go: fetchAdditionalReleases, hookJobName (shared prefix helper), additionalReleaseLocalFilename (source-type-aware cache filename resolution), hook synthesis in generateBoshReleaseDir, hook/additional-release wiring in generateRuntimeConfigs, BakeOptions (SkipFetch, ReleasesDirectory) threaded through Bake/BakeFromLockfile.
• internal/commands/carvel_bake.go: --releases-directory/--skip-fetch/--from-lockfile flags; Kilnfile/Kilnfile.lock load-error handling that distinguishes "missing" from "malformed".
• internal/commands/carvel_helpers.go: downloadCarvelRelease/writeStandardKilnfileLock resolve by release name, not list position; upsert instead of overwrite.
• internal/commands/carvel_upload.go / carvel_publish.go / carvel_rebake.go: updated to the new BakeOptions/name-based lookup plumbing.
• internal/commands/update_release.go, pkg/cargo/kilnfile.go: minor fixes described above.
• Tests: internal/carvel/baker_test.go, internal/commands/carvel_bake_test.go, internal/commands/carvel_helpers_test.go, pkg/cargo/kilnfile_test.go — coverage for hook synthesis, remote-fetch/cache-hit behavior (including the source-type filename mismatch), lockfile-preservation, and the restored diagnostics.

[Copilot]

Pull request overview

Extends the Carvel tile workflow to match classic kiln bake behavior by enabling remote fetching of additional_releases via Kilnfile/Kilnfile.lock, synthesizing pre/post-install hook adapter jobs into the tile’s generated BOSH release, and updating carvel upload/publish/rebake to rely on name-based lockfile resolution and reproducible cached release artifacts.

Changes:

• Add additional_releases, pre_install_hooks, and post_install_hooks to Carvel base.yml metadata and thread these into runtime-config/release generation.
• Implement additional release fetching (with cache-hit + --skip-fetch behavior) and hook job synthesis into the tile’s own release.
• Update Carvel commands and lockfile handling to support name-based lookups and lockfile upserts/preservation.

Reviewed changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
pr_description.md	Adds a repo-stored PR/feature description for the Carvel enhancements.
pkg/cargo/kilnfile.go	Changes lockfile update helper to pointer receiver and “upsert” semantics.
pkg/cargo/kilnfile_test.go	Updates unit tests to reflect new lockfile upsert behavior.
internal/commands/update_release.go	Adjusts update-release to tolerate missing lock entries and improves error hinting.
internal/commands/update_release_test.go	Updates expectations for the revised update-release error behavior.
internal/commands/carvel_upload.go	Threads new bake options and improves Kilnfile/Kilnfile.lock loading behavior.
internal/commands/carvel_rebake.go	Switches to name-based lockfile lookup and updated baker signatures/options.
internal/commands/carvel_rebake_test.go	Updates tests for new baker method signatures.
internal/commands/carvel_publish.go	Uses name-based lockfile lookup and passes bake options through publish path.
internal/commands/carvel_publish_test.go	Updates tests for new baker method signatures.
internal/commands/carvel_helpers.go	Adds name-based release download and lockfile upsert/preservation logic.
internal/commands/carvel_helpers_test.go	Adds coverage to ensure lockfile writes preserve existing entries.
internal/commands/carvel_bake.go	Adds --releases-directory, --skip-fetch, and --from-lockfile flow + lockfile parse handling.
internal/commands/carvel_bake_test.go	Adds regression coverage for “lockfile present but not opted-in” and malformed lockfile failures.
internal/carvel/models/metadata.go	Extends metadata schema with additional_releases and hook declarations.
internal/carvel/models/job.go	Broadens job properties type to support non-package-install job payloads.
internal/carvel/baker.go	Implements hook synthesis, additional release fetch/caching logic, and new bake option plumbing.
internal/carvel/baker_test.go	Adds/updates tests for additional releases, hook synthesis, and properties passthrough.
internal/acceptance/carvel/carvel_workflow_test.go	Adjusts acceptance workflow assertions related to lockfile/CI bake behavior.

Suppressed comments (1)

internal/commands/carvel_helpers.go:85

• writeStandardKilnfileLock silently ignores all read errors from os.ReadFile (not just “file does not exist”). This can cause data loss by overwriting a lockfile that was unreadable due to permissions/IO errors. Only ignore IsNotExist; return an error for other failures.

	if data, err := os.ReadFile(lockfilePath); err == nil {
		if err := yaml.Unmarshal(data, &lock); err != nil {
			return fmt.Errorf("failed to parse existing Kilnfile.lock: %w", err)
		}
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (5)

internal/carvel/baker.go:866

• Same as generateBoshReleaseDir: runtime-config hook validation allows whitespace-only name/command, which will later produce invalid job references or broken hook scripts. Use TrimSpace() in the validation guard.

	for _, group := range b.hookModeGroups() {
		for _, hook := range group.hooks {
			if hook.Name == "" || hook.Command == "" {
				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
			}
			addonJobs = append(addonJobs, models.Job{Name: b.hookJobName(hook.Name), Release: b.metadata.Name})

internal/carvel/baker.go:541

• Hook validation treats whitespace-only name/command as valid (e.g. name: " "), which will generate an invalid BOSH job or an exec line with no command. Trim whitespace before validating so these fail fast with the intended error.

This issue also appears on line 861 of the same file.

	for _, group := range b.hookModeGroups() {
		for _, hook := range group.hooks {
			if hook.Name == "" || hook.Command == "" {
				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
			}

internal/carvel/baker.go:976

• This progress message claims the local tarball has a “correct SHA1”, but the code also treats lockEntry.SHA1 == "" as a cache hit (no verification). Adjust the message so it stays accurate when the lockfile doesn’t include a SHA1.

					actualSHA1 := hex.EncodeToString(h.Sum(nil))
					if lockEntry.SHA1 == "" || actualSHA1 == lockEntry.SHA1 {
						b.progress(fmt.Sprintf("  Release %s %s already exists locally with correct SHA1 — skipping fetch", lockEntry.Name, lockEntry.Version))
						f.Close()

internal/commands/carvel_rebake.go:138

• carvel rebake hard-codes ReleasesDirectory: "releases", which is interpreted relative to the current working directory. That can unexpectedly download additional_releases into whatever directory the user ran the command from and leave artifacts behind. Prefer making it relative to the tile source directory (or a temp dir) so rebake is self-contained.

		err = b.BakeFromLockfile(sourcePath, kilnfile, kilnfileLock, releaseLock, localTarball, carvel.BakeOptions{
			SkipFetch:         false,
			ReleasesDirectory: "releases",
		})
	} else {
		c.outLogger.Printf("Re-baking Carvel tile from %s", sourcePath)
		err = b.Bake(sourcePath, kilnfile, kilnfileLock, carvel.BakeOptions{
			SkipFetch:         false,
			ReleasesDirectory: "releases",
		})

internal/commands/carvel_bake.go:83

• When LoadKilnfiles fails and Kilnfile.lock is absent, the code assumes the lockfile is the problem and proceeds. If a Kilnfile exists but is malformed/unparseable, this path will silently ignore the Kilnfile error and bake anyway (until additional_releases are needed). Consider explicitly validating Kilnfile when it exists so genuine Kilnfile parse errors aren’t swallowed.

	kf, kl, loadErr := c.Options.LoadKilnfiles(nil, nil)
	if loadErr == nil {
		kilnfile = kf
		kilnfileLock = kl
	} else if lockfilePresent {
		return fmt.Errorf("failed to load Kilnfiles: %w", loadErr)
	} else if c.Options.FromLockfile {
		return fmt.Errorf("failed to load Kilnfiles (required for --from-lockfile): %w", loadErr)
	} else {
		// lockfilePresent is false here, so the lockfile is confirmed absent
		// — that's what actually blocks additional_releases/--from-lockfile,
		// regardless of whether a Kilnfile exists on its own (LoadKilnfiles
		// always requires both, so we can't tell from loadErr alone).
		c.outLogger.Printf("No Kilnfile.lock found — proceeding without additional_releases support")
	}

[Copilot]

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

internal/carvel/baker.go:934

• Same as above: runtime-config hook validation should treat whitespace-only hook.Name/hook.Command as missing to avoid emitting an addon job referencing a synthesized hook with an empty exec command.

	for _, group := range b.hookModeGroups() {
		for _, hook := range group.hooks {
			if hook.Name == "" || hook.Command == "" {
				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
			}

internal/commands/carvel_helpers_test.go:68

• This test relies on chmod(0000) making the lockfile unreadable. On platforms/filesystems where chmod does not remove read permissions (e.g. Windows), the test will fail because writeStandardKilnfileLock can still read the file. Add a post-chmod read probe and skip the test if the file remains readable.

	require.NoError(t, os.WriteFile(lockfilePath, data, 0644))
	require.NoError(t, os.Chmod(lockfilePath, 0000))
	defer func() { _ = os.Chmod(lockfilePath, 0644) }()

internal/commands/carvel_bake.go:82

• This log message says the bake is proceeding "without additional_releases support", but the bake will still attempt to process additional_releases from base.yml and then fail later with a lockfile-not-found error. Consider making the message explicit that additional_releases/--from-lockfile require a lockfile, and that the command is baking from source.

		// lockfilePresent is false here, so the lockfile is confirmed absent
		// — that's what actually blocks additional_releases/--from-lockfile,
		// regardless of whether a Kilnfile exists on its own (LoadKilnfiles
		// always requires both, so we can't tell from loadErr alone).
		c.outLogger.Printf("No Kilnfile.lock found — proceeding without additional_releases support")
	}

internal/carvel/baker.go:589

• Hook validation currently treats whitespace-only values as non-empty, which can generate a hook script with an empty exec command. Trim whitespace before validating hook.Name/hook.Command.

This issue also appears on line 930 of the same file.

	for _, group := range b.hookModeGroups() {
		for _, hook := range group.hooks {
			if hook.Name == "" || hook.Command == "" {
				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
			}

[Copilot]

Pull request overview

Copilot reviewed 19 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/commands/carvel_rebake.go:132

• In the lockfile rebake path, BakeFromLockfile is configured to download additional releases into a hard-coded relative "releases" directory. That can create/modify a ./releases directory in the caller’s working directory (and potentially dirty the repo) even though rebake already has a temp dir available. Use the existing tmpDir as the releases download/cache directory for this run.

		err = b.BakeFromLockfile(sourcePath, kilnfile, kilnfileLock, releaseLock, localTarball, carvel.BakeOptions{
			SkipFetch:         false,
			ReleasesDirectory: "releases",
		})

internal/commands/update_release.go:124

• Don’t ignore the return value from UpdateBOSHReleaseTarballLockWithName. The method can fail (e.g., empty name), and ignoring it would silently write an unchanged/incorrect Kilnfile.lock. Handle the error and surface it to the user.

	_ = (&kilnfileLock).UpdateBOSHReleaseTarballLockWithName(u.Options.Name, releaseLock)

	err = u.Options.SaveKilnfileLock(u.filesystem, kilnfileLock)