feat(release): publish manifest revision with image bundles - #297
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly extends the v1 release bundle with an additive manifests.git field and picks a fixed Snapshot manifest revision instead of a moving main head, which is the right call for reproducible promotions. The publisher, pipeline embedding, and test coverage are solid; my one substantive concern is that the revision-selection loop is order-dependent on non-linear history, and there is a cross-PR merge-order coordination item on the shared Tekton trigger.
Findings
[Major] manifest_source selection is order-dependent on non-linear history - scripts/release_bundle.py:124-144 (and the embedded copy in pipelines/release-bundle/pipeline.yaml).
The loop iterates revisions in lexical SHA order and only compares each revision against the running selected. When the component revisions are totally ordered (a strictly linear main), this correctly returns the newest and is deterministic. But when the set contains a divergent pair that nonetheless has a common descendant in the set (e.g. {X, A, C} where X and A are siblings both merged before C), the outcome depends purely on the lexical order of the SHAs: some orderings promote selected to C and succeed, while others compare X against A first and raise "divergent histories." Same inputs, different result, decided by an essentially random SHA sort - for a publisher whose stated contract is reproducibility, that determinism gap is worth closing.
Practical impact is low today because the repo squashes to main (linear history, every commit totally ordered, so the divergent-pair case cannot arise). But the code and test_divergent_component_revisions_stop_selection deliberately handle merge/divergence, so non-linear history is treated as in-scope. Suggested deterministic fix: after computing selected, assert that every revision is an ancestor of selected (single well-defined rule), and fail otherwise - this makes success/failure independent of iteration order. Confidence: Medium.
[Minor] Non-linear-history behavior is asserted but its intent is not documented. The README and the # Divergent histories cannot form one manifest/image release unit. comment imply divergence should always fail, yet a divergent set with a unifying descendant can currently succeed (see above). A one-line note on the intended rule (newest commit that is a descendant of all component revisions; otherwise fail) would make the contract explicit. Confidence: Medium.
Notes (not blocking)
- The manifest existence check uses
git cat-file -e <rev>:<path>/kustomization.yamlagainst a--filter=blob:noneclone. I verified this is fine: the check resolves the tree entry (trees are fetched), so a present path passes and a missing path fails as intended; the tests exercise this over a full clone, but the production partial-clone path behaves equivalently for path existence. - The CEL trigger now rebuilds the API-server image on any
deploy/***change to mint a Snapshot for manifest-only edits. That is intentional per the description; just noting it trades some build churn for manifest-only release coverage.
Cross-PR coordination
Another open pull request rewrites this same component's build pipeline and the same pipelinesascode.tekton.dev/on-cel-expression trigger in .tekton/hypershell-api-server-main-push.yaml: the "managed source releases" PR (#237). That PR is based on a pre-release-bundle revision of the file (it still sets dockerfile: Dockerfile / path-context: components/api-server and knows nothing of the release-bundle pipelineSpec), and it adds a different path token (VERSION) to the same CEL list that this PR extends with deploy/***, pipelines/release-bundle/***, and the two publisher scripts. Maintainers need to decide the merge order and ensure #237 rebases onto the merged release-bundle pipeline so that this PR's deploy/** and release-bundle triggers (and the release-bundle-aware pipeline body) are preserved rather than reverted by the stale rewrite. This is a plan/ordering decision, not a plain text merge conflict.
Previous concerns
There are no prior Amber findings in the review history to reconcile for this pull request.
Findings Summary (ordered by severity, highest first)
- [Major] Revision selection in
manifest_sourceis order-dependent for non-linear history, giving non-deterministic success/failure - Correctness / Reproducibility (release_bundle.py L124-144) - [Minor] Intended divergence rule is asserted in tests but not documented, and diverges from actual behavior - Docs / Spec Completeness (release_bundle.py L137, README)
Convention Checklist
| Convention | Result |
|---|---|
No panic() / explicit errors (Python raises on failed subprocess) |
Pass |
| No secrets in logs or responses (auth.json 0o600, not logged) | Pass |
| Input validated (digest/revision regex, namespace/name checks) | Pass |
| Reproducible/deterministic publisher output | Fail |
| Test diff scrutiny (no flipped/removed guarantees; changes additive) | Pass |
Embedded pipeline matches source (render/bootstrap test) |
Pass |
| Conventional commit message | Pass |
| run("git", "merge-base", "--is-ancestor", revision, | ||
| "refs/remotes/origin/main", cwd=source_directory) | ||
| selected = revisions[0] | ||
| for revision in revisions[1:]: |
There was a problem hiding this comment.
[Major] Order-dependent revision selection.
This loop walks revisions in lexical SHA order and only compares each revision against the running selected. For a strictly linear history this correctly returns the newest commit deterministically. But for a divergent set that still has a common descendant in the set (e.g. {X, A, C} with X/A siblings both merged before C), the result depends on SHA order: some orderings promote selected to C and succeed, others compare X vs A first and raise "divergent histories." Same inputs, different outcome. Squash-merges keep main linear today so it is latent, but since divergence is explicitly handled/tested, consider a deterministic rule: after choosing selected, assert every revision is an ancestor of selected and fail otherwise.
2 participants
Add
manifests.git.urlandmanifests.git.revisionto each release bundle so Kargo can promote shared manifests with the image set. Select the newest component source commit in the accepted Snapshot, verify that all component revisions are its ancestors, and check the three required manifest paths. Do not select the head ofmain, which can advance while a release waits.Changes under
deploy/and changes to the publisher now trigger the API-server push build. This lets a manifest-only change produce a Snapshot and release bundle while retaining unchanged component images. The new manifest field is additive to the version 1 bundle format. The existing registry, credentials, and tenant final pipeline configuration are reused.Merge and sync https://github.com/openshift-online/hypershell-gitops/pull/153 first. Then merge this PR. Its Tekton change starts the build that can produce the first bundle with a manifest revision. The shared refs affect all consumers; the companion promotion checks deployment health only for hyp1.
Validation: all 20 publisher tests passed, including the embedded pipeline, fixed revision selection after
mainadvances, manifest-only changes, invalid or divergent source histories, missing manifests, and publication checks before registry access. Git fixture tests clear inherited Git environment variables so they also run safely from a commit hook. Repository policy checks and Tekton YAML parsing passed.