Repository navigation
fix(ci): mirror and retag images whose digest is embedded in the values tag - #3375
Conversation
The nightly's full-suite e2e failed at install: the e2e-image-prepuller
DaemonSet never became ready, because
ghcr.io/cozystack/cozystack/linstor-csi@sha256:2f314cb9... and
.../piraeus-server@sha256:39fdacec... both 404 on GHCR. They are not there
because nightly-mirror.sh never copied them.
collect_refs handled three values shapes, and the one most package Makefiles
actually write is not among them: a split map whose digest is embedded in the
tag, {repository, tag: <tag>@sha256:<digest>}, produced by a single yq call
setting .image.tag to "$(IMAGE_TAG)@$(digest)". It matches neither the
{repository, tag, digest} rule (no `digest` key) nor, usefully, the
single-string rule -- that one matches the bare tag value, which carries no
repository, so ref_repo() reduces "main@sha256:..." to the repository "main"
and the SRC_REGISTRY case drops it.
Being dropped there is silent, and the host rewrite at the end of the script
runs over every depth-2 values.yaml regardless of what was mirrored. So the
image was never copied AND its repository was still repointed at
DST_REGISTRY, leaving a reference to an image that was never pushed there --
exactly the dangling-ref failure the comment above that rewrite warns about,
reached by a path it did not anticipate. The 404 then surfaces at pull time,
one job later, as a DaemonSet rollout timeout that names no image.
This is not limited to linstor. Checking the :nightly tag on GHCR, the two
packages using a recognised shape (cilium, cozystack-operator) are present
and eight images across six packages using this shape are absent: kamaji,
kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller,
metallb-speaker, redis-operator. The prepuller only templates
kubeovn/linstor/cert-manager, so it surfaced two of the eight; the rest would
have failed later at HelmRelease install.
Latent since the script was added in 4777273 (2026-06-26) -- the nightly
never completed a full run before today, so the mirror step had never been
exercised end to end.
The new rule type-guards `.tag` before test(). Without it a non-string tag
aborts the whole expression ("cannot match with !!int"), and since the
invocation swallows stderr and status that abort would silently discard every
shape-3 ref in the SAME file -- reintroducing this very bug in any file that
happens to carry an unquoted `tag: 1.24` next to a cozystack image. Rule 1 is
immune because its select(tag == "!!str") already precedes its test().
The test fixture is why this passed green: _make_tree() covered the
single-string, {repository,tag,digest} and platformSource shapes, a layout
only cilium actually uses, and omitted the dominant one. It now builds that
shape too, co-locates a numeric third-party tag with a cozystack shape-3 ref
to pin the type guard, and asserts the planned copy carries the REPOSITORY
rather than the bare tag -- a rule matching the tag alone would plan a copy
from "main@sha256:..." and be dropped as non-SRC_REGISTRY, so asserting only
the destination would not catch the regression.
Verified each assertion fails against the unfixed script and passes with it,
and that a realistic baked linstor tree now plans both copies.
Signed-off-by: Myasnikov Daniil <[email protected]>
promote-retag.sh carries the same collect_refs blind spot the previous commit fixes in nightly-mirror.sh, and nightly-mirror.sh's own comment asserted the two were "identical to" each other -- which is how the gap stayed invisible once one of them was fixed. Measured against the committed tree, the dry-run selects 19 repositories before this change and 27 after. The eight it was missing are exactly the set that broke the nightly: kamaji, kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller, metallb-speaker, redis-operator. Nothing is lost from the previous selection. The consequence differs from the nightly case and is milder. This retag is within a single registry, so the digests already exist and every digest-pinned install still resolves -- there is no dangling reference and no 404. What is missing is the :<version> tag (and :latest under MOVE_LATEST=1) for those eight images, so a promoted release does not carry the tags it claims to, and anything resolving a cozystack component by version tag rather than by digest does not find it. Nothing in the promotion detects this: the script reports success having simply never seen those refs. The existing test could not catch it. It asserts "at least one cozystack-owned image is selected" and that nothing third-party leaks in -- both satisfied by the shapes that already worked, which is why eight omissions sat under a green suite. It now names concrete shape-3 packages, since a count or a generic pattern would drift back to proving nothing. Verified the assertion fails against the unfixed script. Same type guard on `.tag` as the previous commit, for the same reason: without it an unquoted numeric tag anywhere in a file silently discards every shape-3 ref in that file, and the swallowed stderr leaves no trace. Signed-off-by: Myasnikov Daniil <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the CI pipeline where specific image reference shapes—specifically those with digests embedded in the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
area/ci
📝 WalkthroughWalkthroughThe image reference collectors now support additional digest-pinned YAML shapes, including digests embedded in ChangesDigest-pinned image reference collection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ValuesYAML
participant collect_refs
participant yq
participant DryRunPlan
ValuesYAML->>collect_refs: provide image reference shapes
collect_refs->>yq: evaluate guarded extraction expressions
yq-->>collect_refs: return canonical repository@digest refs
collect_refs->>DryRunPlan: add selected mirror or retag entries
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for a fourth image-ref shape where the digest is embedded directly in the tag field of a split map (e.g., {repository, tag: Shamim Hasnath (@sha256):}). This shape is commonly written by package Makefiles but was previously ignored, leading to images not being mirrored or promoted. The changes update hack/nightly-mirror.sh and hack/promote-retag.sh to extract these references using yq, with type guards to prevent silent failures on non-string tags. Corresponding test cases are added to the BATS test suites. The reviewer noted that the PR description is missing the required release-note block as specified in the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| [ "$DRY_RUN" -eq 1 ] || command -v skopeo >/dev/null || { echo "skopeo is required" >&2; exit 1; } | ||
|
|
||
| # Collect "repo@sha256:..." refs from every package values.yaml, across the three | ||
| # Collect "repo@sha256:..." refs from every package values.yaml, across the four |
There was a problem hiding this comment.
The pull request description is missing the required release-note block. According to the repository style guide, every PR must contain a release note block in its body. Please add one to the PR description.
References
- PR body must contain a release note block of the format: release-note type(scope): human-readable changelog entry (link)
There was a problem hiding this comment.
Added. The PR body now carries the release-note block and the Downstream Repositories checklist from the template.
github-actions
Bot
added
the
size/L
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the fix itself is correct and I verified it end to end (my own dry-run over the committed tree gives 19 → 27 repositories, exactly the eight named images gained and nothing lost; both suites green; removing the new rule and removing the type guard each turn the suites red), but two more cozystack-owned images of the exact class named in the title remain silently skipped, and the comments this PR adds and retains now claim a completeness the tree contradicts.
Business context: the nightly mirror and the release promotion silently skipped every image whose digest is embedded in the values tag field, so the nightly published dangling references that 404 at install and promoted releases did not carry the tags they claim.
Blockers
B1: two cozystack-owned digest-in-tag refs are still silently dropped, while the comments declare the enumeration complete
File: hack/promote-retag.sh:84
Issue: the new rule assumes the registry host lives inside repository. Two cozystack-owned refs of this same class do not satisfy that and are emitted host-less, so the ownership filter silently drops them — the same emit-then-drop failure mode this PR fixes for shape 1: (1) packages/system/keycloak-operator/values.yaml carries {registry: ghcr.io, repository: cozystack/cozystack/keycloak-operator, tag: "latest@sha256:…"}; the map matches the has("repository") and has("tag") select, the rule emits cozystack/cozystack/keycloak-operator@sha256:…, and the ${REGISTRY}/* filter drops it — and the package's own Makefile writes exactly this shape (yq -i '.keycloak-operator.image.tag = strenv(TAG)'); (2) packages/system/kubeovn/values.yaml carries global.registry.address: ghcr.io/cozystack/cozystack plus {repository: kubeovn, tag: v1.15.10@sha256:…}; the rule emits bare kubeovn@sha256:…, dropped, while the chart renders {{ .Values.global.registry.address }}/kubeovn:… — an image under the cozystack registry.
Evidence: exhaustive audit of the committed tree — 35 unique @sha256: digests across packages/*/*/values.yaml vs 28 in the new dry-run plan; of the 7 misses, only these two render to repositories under $REGISTRY (the rest are third-party or outside it, correctly excluded). The registry confirms the consequence: ghcr.io/cozystack/cozystack/keycloak-operator carries exactly one tag, latest, although the package has shipped since 2024-11 — no promotion has ever tagged it; the kubeovn repository carries only upstream-scheme tags (v1.13.x–v1.15.x and chart-suffixed variants), no cozystack release tags. Both digests referenced by the tree resolve on GHCR (HTTP 200), so the retained explanation "not pushed to $REGISTRY" does not hold. Compounding it, the comments now misstate coverage in both directions: the new header says "the four shapes the build writes" while the build also writes the keycloak-operator shape, and the retained filter comment (hack/promote-retag.sh:124-126) still says the filter drops "bare upstream tags (kube-ovn/keycloak/kilo) … all vendored by digest but not pushed to $REGISTRY" — kilo is precisely one of the eight this PR now selects, and kube-ovn and keycloak-operator ARE on $REGISTRY. A future reader auditing collect_refs against these comments would conclude coverage is complete — the exact "why this was invisible" trap this PR's description warns about.
Impact: promotion keeps reporting success while keycloak-operator and kubeovn never receive their :<version> tags (nor :latest under MOVE_LATEST=1). The nightly is not affected by these two: neither package appears in the root Makefile's build target, so the baked main tree never carries an OCIR ref for them.
Fix: either extend coverage here — a rule joining .registry + "/" + .repository for the keycloak-operator shape, plus a decision on the kubeovn global.registry.address case, each with a fixture that goes red without the rule — or, if deliberately out of scope, correct both comments to name these two refs as known-unhandled and file a follow-up issue. Stronger than either: an invariant-style completeness test that collects every @sha256: digest in the tree whose rendered repository falls under $REGISTRY and asserts each one appears in the copy plan — that test would have caught the original eight, both of these, and any future shape drift; a named-package list only catches the already-known past.
Non-blocking follow-ups
-
.repository(rule 3) and.digest(rule 2) carry no string type guard: a non-stringrepository:aborts the per-file expression exactly the way a non-stringtag:did, silently dropping co-located owned refs from the same file. The guard style is established now;select(.repository | tag == "!!str")is cheap insurance. hack/promote-retag.sh:84, hack/nightly-mirror.sh:81. -
Nightly impact framing: kilo and redis-operator are not in the root Makefile's
buildtarget, so their baked refs keep the committed GHCR host and theSRC_REGISTRYfilter still drops them — their:nightlytags remain absent after this PR (installs are unaffected; those digests already resolve on GHCR). The nightly recovers the six OCIR-built images; promotion recovers all eight. Worth one line in the PR description so nobody expectskilo:nightlyto appear. -
The PR description does not use the repository template: the release-note block (already flagged by a bot) and the downstream-repositories checklist are missing.
| # shape 2 | ||
| yq -r '.. | select(tag == "!!map") | select(has("repository") and has("digest")) | .repository + "@" + .digest' "$f" 2>/dev/null || true | ||
| # shape 3 | ||
| yq -r '.. | select(tag == "!!map") | select(has("repository") and has("tag")) | select(.tag | tag == "!!str") | select(.tag | test("@sha256:[0-9a-f]{64}")) | .repository + "@" + (.tag | sub(".*@"; ""))' "$f" 2>/dev/null || true |
There was a problem hiding this comment.
This rule still loses two cozystack-owned refs of the same digest-in-tag class, because it assumes the registry host lives inside repository: keycloak-operator keeps the host in a sibling registry key (the map matches the has("repository") and has("tag") select, is emitted host-less as cozystack/cozystack/keycloak-operator@sha256:…, then dropped by the ${REGISTRY}/* filter), and kubeovn keeps it in global.registry.address with a bare repository: kubeovn. Evidence and fix options in the review body (B1).
There was a problem hiding this comment.
Confirmed and fixed in acd88bdd0. Shapes 2 and 3 now rejoin an optional registry sibling, and a new shape-4 rule handles global.registry.address + global.images.<n>, so both refs reach the ownership filter with a host. Dry-run over the committed tree goes from 27 to 29 unique repositories — exactly keycloak-operator and kubeovn, nothing lost.
On kubeovn specifically: it is cozystack-built, not an upstream mirror. cozystack/kubeovn-chart builds it from its own docker/Dockerfile and pushes to $(REGISTRY)/kubeovn, and its make image writes all three keys — so the shape is ours, not an upstream convention that make update can shift. The registry backs the consequence you described: its newest cozystack-versioned tag is v1.14.11-v0.38.8, from before the 1.0 switch, while cilium carries v1.5.0 through v1.6.0-rc.2.
Both comments are corrected. The header no longer presents its list as exhaustive, and the ownership-filter comment no longer names kilo, kube-ovn and keycloak-operator as third parties not pushed to $REGISTRY. I did not add the invariant completeness test — it needs a rendered-repository oracle the scraper cannot produce for refs it fails to see, so it degrades into a hand-maintained allow-list. An audit of all 35 digests in the tree is recorded in the PR body instead.
| [ -f "$f" ] || continue | ||
| yq -r '.. | select(tag == "!!str") | select(test("@sha256:[0-9a-f]{64}"))' "$f" 2>/dev/null || true | ||
| yq -r '.. | select(tag == "!!map") | select(has("repository") and has("digest")) | .repository + "@" + .digest' "$f" 2>/dev/null || true | ||
| yq -r '.. | select(tag == "!!map") | select(has("repository") and has("tag")) | select(.tag | tag == "!!str") | select(.tag | test("@sha256:[0-9a-f]{64}")) | .repository + "@" + (.tag | sub(".*@"; ""))' "$f" 2>/dev/null || true |
There was a problem hiding this comment.
Non-blocking: .repository here (and .digest in rule 2) have no string type guard, so a non-string repository: aborts this file's whole expression exactly the way a non-string tag: did, silently dropping co-located owned refs. The guard style is established now — select(.repository | tag == "!!str") is cheap insurance.
There was a problem hiding this comment.
Applied in 292f8ec65, with one correction to the mechanism worth recording in the comment. These keys are never fed to test(), so the cannot match with !!int path does not reach them: yq coerces int, bool, null and seq on +, and each merely yields a ref the ownership filter discards. I checked each type — only a !!map actually aborts, raising !!str () cannot be added to a !!map, and that does take the whole file down.
So the guard is narrower insurance than the tag one: a nested-map typo in a vendored chart, not an unquoted version number. It is a select() rather than a test(), so an offending map drops only itself and its neighbours in the same file still resolve. registry is guarded on the same grounds, since the new join concatenates it. The nightly fixture gains a file pairing a map-typed repository with an owned ref; removing the guard turns it red.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The fix is correct and well-tested for the three shapes it adds, but it is not exhaustive: a real, currently-shipped cozystack-owned image already in the tree uses a fourth split-map shape that neither script's new rule recognizes, so it stays silently dropped from both the nightly mirror and the stable-promotion retag after this change lands.
Findings
[MAJOR] hack/nightly-mirror.sh:81, hack/promote-retag.sh:84 — new shape-3 rule still silently drops the {registry, repository, tag: <t>@sha256:<d>} split shape
Both new rules build the collected ref as .repository + "@" + (.tag | sub(".*@"; "")), which assumes the full host+path lives in .repository. That holds for the shapes fixed here (kamaji, kilo, etc., which embed the host in .repository directly), but packages/system/keycloak-operator/values.yaml:1-5 splits the host into a separate .registry key:
keycloak-operator:
image:
registry: ghcr.io
repository: cozystack/cozystack/keycloak-operator
tag: "latest@sha256:27279247dfbd2d1e86fc4e827d34c891880ca65eefd75da1701bc0a1adf865a9"The chart renders this as ghcr.io/cozystack/cozystack/keycloak-operator:latest@sha256:... (packages/system/keycloak-operator/charts/keycloak-operator/templates/deployment.yaml:40: {{ if .Values.image.registry }}{{ .Values.image.registry }}/{{ end }}{{ .Values.image.repository }}:...), so it is unambiguously a cozystack-owned image in the same ghcr.io/cozystack/cozystack/* namespace as every other retagged image. But the new rule concatenates only .repository + "@" + digest = cozystack/cozystack/keycloak-operator@sha256:... — no host — so ref_repo() reduces it to cozystack/cozystack/keycloak-operator, which fails the "${REGISTRY}/"* / "${SRC_REGISTRY}/"* ownership-prefix match in both scripts and is discarded, exactly as silently as the bug this PR fixes (script exits 0, prints nothing about the drop).
Verified concretely:
env -u REGISTRY hack/promote-retag.sh v9.9.9 --dry-runagainst the committed tree (this PR applied) selects 27 repositories;keycloak-operatoris not one of them (grep -c keycloak /tmp/out= 0).- A synthetic reproduction of the same shape (
registry: iad.ocir.io/repository: idyksih5sir9/cozystack/keycloaktest/tag: main@sha256:<64hex>) placed in a baked tree is likewise absent fromhack/nightly-mirror.sh --dry-run's copy plan, while a sibling package using the now-fixed shape in the same tree is copied correctly. git log --oneline -- packages/system/keycloak-operator/values.yamlshows this file is untouched by this PR — the gap is pre-existing, not a regression, but it directly contradicts the PR's own framing of "the four shapes the build writes" as an exhaustive enumeration, and it is the identical failure class (silent drop, script reports success) the PR's motivating incident is about.
Consequence: on the next stable cut, promote-retag.sh (even with this fix) will not create keycloak-operator's :vX.Y.Z tag (or :latest under MOVE_LATEST=1) — same "release doesn't carry the tag it claims to" gap the PR describes for the shapes it does fix. For nightly-mirror.sh, if keycloak-operator (or any future package) is ever baked through the OCIR pipeline with this split shape, the failure is worse than the one this PR fixes: the trailing sed -i "s|${SRC_ESC}/|${DST_REGISTRY}/|g" host-rewrite only matches the literal ${SRC_REGISTRY}/ string, which is not present when the host lives in a separate .registry key, so the tree is not rewritten to GHCR either — the published install closure would embed a reference to the private CI-only OCIR registry instead of a dangling-but-reachable GHCR 404.
Fix shape: extend the shape-3 rule to also read an optional .registry key, e.g. (if has("registry") then .registry + "/" else "" end) + .repository + "@" + (.tag | sub(".*@"; "")), and add a keycloak-operator-shaped case to both bats fixtures (mutation-tested, per the module already applied to shape 3).
[MINOR] PR body bypasses the required template — no release-note block, no Downstream Repositories checklist
docs/agents/contributing.md:139-149 requires filling in .github/PULL_REQUEST_TEMPLATE.md, which "includes the required release-note block and the Downstream Repositories checklist," and explicitly warns that gh pr create --body/--body-file with a from-scratch body "replaces the body wholesale... the template is not applied and its checklists are silently dropped." That is what happened here: the PR body has no ```release-note fenced block and no Downstream Repositories checklist section (confirmed via gh pr view 3375 --json body). Walking the trigger map in docs/agents/contributing.md:151-206 against this diff (hack/*.sh, hack/*.bats, no packages/, no values.schema.json, no ApplicationDefinition) turns up no actual downstream-repo impact, so this is process rather than substance, but the release-note block itself is still owed.
Claim mismatches
[PARTIAL] "the three shapes the build writes" / "the four shapes the build writes" (both scripts' updated header comments) — not exhaustive: a fifth shape, {registry, repository, tag: <t>@sha256:<digest>}, is present today in packages/system/keycloak-operator/values.yaml:1-5 and is not matched by any of the four rules. See Findings.
[UNVERIFIABLE] "Confirmed against the :nightly tag on GHCR" (the 8-images-absent table) — this static, hermetic review has no registry credentials and does not query GHCR directly. Independently corroborated instead via gh run view on the actual failing nightly run (29743625345, 2026-07-20): the log shows daemonset.apps/e2e-image-prepuller stuck at "0 of 3 updated pods are available" for exactly 10 minutes ([00:01] to [10:01]) while templating ghcr.io/cozystack/cozystack/linstor-csi:main@sha256:2f314cb9... and ghcr.io/cozystack/cozystack/piraeus-server:main@sha256:39fdacec... — both digests match the commit message verbatim.
Caveats
- Phase 5b (upgrade/fresh-install impact) and Phase 5c (config-toggle matrix) are not applicable: this PR touches only
hack/nightly-mirror.sh,hack/promote-retag.sh, and their.batsfixtures — nopackages/, chart, migration, CRD, RBAC, orvalues.yamlcontent changes. Classified as pure CI/release tooling. - Non-vacuity of the new tests was independently verified by mutation, not just asserted: reverting the shape-3 rule in either script turns its respective new bats assertion red; separately stripping only the
select(.tag | tag == "!!str")type guard also turns the nightly-mirror assertion red. Both full suites (2/2 and 4/4) pass green as shipped. shellcheck(default severity, and-S style) on both changed scripts: zero findings.promote-retag.sh --dry-runagainst the current committed tree reproduces the PR's own numbers exactly: 19 repositories selected before the shape-3 rule, 27 after.- The new
promote-retag_test.batsloop names 4 of the 8 previously-missing packages; the other 4 share the identical shape-3 code path. - Commits: two, each
fix(ci): ..., signed off, Conventional-Commits-formatted — no convention issues found.
Recommended follow-ups
- Extend the shape-3 rule in both scripts to handle an optional separate
.registrykey (see Findings), and add akeycloak-operator-shaped fixture case to both.batssuites. - Audit
packages/*/*/values.yamlonce for any other{registry, repository, ...}-shaped map before the next release cut.
Address review feedback from lexfrei and IvanHunters on
hack/promote-retag.sh:84 and hack/nightly-mirror.sh:81.
The shape-3 rule assumed the registry host lives inside `repository`.
Two cozystack-built images do not satisfy that and were emitted
host-less, so the ownership filter dropped them — the same silent
emit-then-drop this PR fixes for the other shapes:
- keycloak-operator splits the host into a sibling `registry` key
({registry: ghcr.io, repository: cozystack/cozystack/keycloak-
operator, tag: latest@sha256:...}), and its own Makefile writes
exactly that layout.
- kubeovn keeps the host in the document-level
global.registry.address with a bare `repository: kubeovn`. It is
built and pushed to $REGISTRY by the cozystack/kubeovn-chart
wrapper, whose `make image` writes all three keys.
Neither has received a 1.x release tag: on GHCR keycloak-operator
carries only `latest`, and kubeovn's newest cozystack-versioned tag is
v1.14.11-v0.38.8, from before the 1.0 versioning switch — while cilium,
which uses an already-covered shape, has v1.5.0 through v1.6.0-rc.2.
Shapes 2 and 3 now rejoin an optional `registry` sibling, and a new
shape-4 rule handles the chart-global layout. An audit of every
@sha256: digest under packages/*/*/values.yaml against the selector
output confirms the remaining five misses are third-party or sit
outside $REGISTRY. Dry-run over the committed tree: 28 -> 30
repositories, exactly these two gained, nothing lost.
Also correct two comments that overstated coverage. The header no
longer presents its list as the shapes the build is limited to — the
list is empirical and charts are vendored — and the ownership-filter
comment no longer names kilo, kube-ovn and keycloak-operator as
third parties not pushed to $REGISTRY. All three are pushed there, and
that wrong comment is part of why the missing tags went unnoticed.
Signed-off-by: Myasnikov Daniil <[email protected]>
Address non-blocking review feedback from lexfrei on hack/nightly-mirror.sh:81 and hack/promote-retag.sh:84. `registry`, `repository` and `digest` are joined into a ref by string concatenation with no type guard, so a non-string value could abort the whole per-file expression. Because collect_refs swallows both stderr and status, that abort silently discards every co-located owned ref — the same per-file blast radius the `tag` guard already prevents. The mechanism differs from `tag`'s, though, and the narrower reading is worth recording. These keys are never fed to test(), so the "cannot match with !!int" abort does not apply to them: yq coerces int, bool, null and seq on `+`, and each of those merely yields a ref the ownership filter discards. Only a !!map actually aborts, raising "!!str () cannot be added to a !!map". The guard is therefore narrow insurance against a nested-map typo in a vendored chart, not protection against unquoted scalars. Implemented as select() rather than a test(), so an offending map drops only itself and its neighbours in the same file still resolve. The nightly fixture gains a file pairing a map-typed `repository` with an owned ref, asserting the latter is still mirrored; removing the guard turns it red. Selection over the committed tree is unchanged at 30 repositories. Signed-off-by: Myasnikov Daniil <[email protected]>
|
Aleksei Sviridkin (@lexfrei) Thanks — B1 was correct on both refs and is fixed in Follow-up 2 is right and I extended it: four of the ten recovered images are not in the root Follow-up 3 is done: release-note block and Downstream Repositories checklist added. On the completeness test — I agree it is the stronger shape, but I could not make it honest here. Asserting on collection does not catch this class, since both missed refs were collected and then dropped by the ownership filter; asserting on the rendered repository needs an oracle the scraper cannot produce for exactly the refs it fails to see. Every version I tried collapsed into a hand-maintained allow-list, which is the weaker thing you were arguing against. I ran the audit instead and recorded it in the PR body: all 35 digests accounted for, 30 selected, five third-party or outside |
|
IvanHunters Thanks — the Your point about the host rewrite was the sharper half and is now called out in the comment: the closing On the The release-note block and Downstream Repositories checklist are added. Walking the trigger map against the diff ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/nightly-mirror_test.bats`:
- Around line 167-168: Add an assertion in the nightly mirror test alongside the
existing survivor check to verify that the ref tagged `numeric` appears in
`$tmp/out`. Keep the fixture and existing assertions unchanged, and assert the
expected mirrored output for that ref.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bde5289d-6575-4ecb-9efd-211697abf75d
📒 Files selected for processing (4)
hack/nightly-mirror.shhack/nightly-mirror_test.batshack/promote-retag.shhack/promote-retag_test.bats
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/promote-retag_test.bats
- hack/promote-retag.sh
| # An owned ref sharing a file with a map-typed `repository` survives. | ||
| grep -q 'docker://ghcr.io/cozystack/cozystack/survivor:0.0.0-nightly.test' "$tmp/out" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the numeric-tagged ref is mirrored.
The fixture intentionally co-locates numeric with an invalid tag: 123 to verify that the type guard on .tag prevents the file from aborting (as explained in the excellent comment at lines 50-55). However, the test script doesn't actually assert the presence of numeric in the output plan, meaning it doesn't currently enforce the behavior it aims to test.
💚 Proposed fix to assert the presence of `numeric`
# An owned ref sharing a file with a map-typed `repository` survives.
grep -q 'docker://ghcr.io/cozystack/cozystack/survivor:0.0.0-nightly.test' "$tmp/out"
+
+ # An owned ref sharing a file with a numeric `tag` survives.
+ grep -q 'docker://ghcr.io/cozystack/cozystack/numeric:0.0.0-nightly.test' "$tmp/out"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # An owned ref sharing a file with a map-typed `repository` survives. | |
| grep -q 'docker://ghcr.io/cozystack/cozystack/survivor:0.0.0-nightly.test' "$tmp/out" | |
| # An owned ref sharing a file with a map-typed `repository` survives. | |
| grep -q 'docker://ghcr.io/cozystack/cozystack/survivor:0.0.0-nightly.test' "$tmp/out" | |
| # An owned ref sharing a file with a numeric `tag` survives. | |
| grep -q 'docker://ghcr.io/cozystack/cozystack/numeric:0.0.0-nightly.test' "$tmp/out" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/nightly-mirror_test.bats` around lines 167 - 168, Add an assertion in
the nightly mirror test alongside the existing survivor check to verify that the
ref tagged `numeric` appears in `$tmp/out`. Keep the fixture and existing
assertions unchanged, and assert the expected mirrored output for that ref.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Correct, well-scoped fix to two CI-only shell scripts; the selector gap and the host-outside-repository gap are real, the new rules are covered by non-vacuous tests, and nothing on the publish path regresses. Only minor follow-up notes below.
Claim mismatches
None. Every load-bearing self-claim I re-checked held up:
- "shape 3 matches neither shape 1 nor 2, silently dropped" — reproduced:
ref_repo("main@sha256:...")reduces tomain, dropped by the ownership filter. - "the
tag/repositorytype guards are load-bearing" — reproduced directly: an unguarded shape-3 expression on a file with a map-typedrepositoryerrors!!str () cannot be added to a !!map(exit 1), taking every co-located ref down; the guarded expression emitssurvivorcleanly. - "both suites pass; every new assertion mutation-tested" — verified: 2/2 + 4/4 green; independently mutated out (a) the shape-3 rule and (b) the
.repositoryguard, both turn the nightly suite RED. - "shellcheck clean at default and
-S style" — verified, exit 0 both.
Caveats
- Scope: touches only
hack/nightly-mirror.sh,hack/promote-retag.shand their two.batssuites. Nopackages/, no chart, novalues.schema.json, no CRD/RBAC/migration. Phase 5b (customer-cluster upgrade / fresh-install) and Phase 5c (chart config-combination) are therefore not applicable — the change alters only what CI publishes, not any in-cluster resource. - No regression to the existing publish set: for a host-in-
repositoryref with noregistrysibling (e.g. cilium) the new shape-2 join collapses back to the prior.repository, and the new type guards areselect()s that no real tree ref trips. Thepromote-retagreal-tree test asserts every emitteddocker://ref is underghcr.io/cozystack/cozystack/(no third-party/malformed leak). - I did not independently re-count the body's "19 -> 29 repositories" nor the "35 digests, 30 selected" audit (each full
collect_refspass ~7 min locally). The substantive property those numbers stand for is directly asserted by the passing real-tree test. - Nightly shape-4 (
global.registry.address) and split-host paths are exercised only by the synthetic fixture: per the PR body, kubeovn/keycloak-operator keep the committed GHCR host in the baked main tree and so are filtered bySRC_REGISTRYat nightly time (installs unaffected). Those branches are dead against real nightly data today but harmless and unit-covered.
Recommended follow-ups
- Non-blocking performance:
collect_refsnow spawns 5yqprocesses pervalues.yaml(was 3) inside an O(files) loop. Could collapse into a singleyqalternation to cut process-spawn overhead. - Theoretical, not present today: a ref carrying BOTH a
registrysibling AND a host already inrepositorywould double-prefix and then be silently dropped — same emit-then-drop class this PR fixes. No such ref exists in the tree. - Server-side behaviour (actual
skopeo copy/inspect) is out of scope for this static review; validated only via--dry-run. Could exercise via thecozystack-pr-testflow before the next rc cut.
3 participants
What's broken
collect_refsin bothhack/nightly-mirror.shandhack/promote-retag.shrecognises three values shapes. The one most package Makefiles actually write is not among them:Package Makefiles set
.image.tagto"$(IMAGE_TAG)@$(digest)"in a singleyqcall rather than maintaining a separatedigestkey. That matches neither the{repository, tag, digest}rule (nodigestkey) nor, usefully, the single-string rule — that one matches the bare tag value, which carries no repository, soref_repo()reducesmain@sha256:...to the repositorymainand the ownership filter drops it.Being dropped is silent. Both scripts then report success having simply never seen those refs.
The same emit-then-drop hits a second class, found in review: refs whose registry host does not live inside
repositoryat all.keycloak-operatorsplits it into a siblingregistrykey, andkubeovnkeeps it in the document-levelglobal.registry.addresswith a barerepository: kubeovn. Both are built and pushed to$REGISTRYby cozystack —kubeovnby thecozystack/kubeovn-chartwrapper repo, whose ownmake imagewrites all three keys — and both were emitted host-less and discarded as third-party.Impact
Nightly (
nightly-mirror.sh) — this is the one that broke. The image is never copied to GHCR, and the unconditional host rewrite at the end of the script still repoints its repository at the destination registry. The result is a published reference to an image that was never pushed there.Confirmed against the
:nightlytag on GHCR — the two packages using a recognised shape are present, and eight images across six packages using this shape are absent::nightly{repository,tag,digest}{repository, tag:<t>@sha256}This surfaced as the first full nightly run failing at install: the
e2e-image-prepullerDaemonSet never became ready (0/3, 10-minute timeout) becauselinstor-csiandpiraeus-server404 on GHCR. The prepuller only templates kubeovn/linstor/cert-manager, so it exposed two of the eight; the other six would have failed later at HelmRelease install.Promotion (
promote-retag.sh) — same blind spot, milder consequence. This retags within a single registry, so the digests already exist and digest-pinned installs still resolve — there is no dangling reference and no 404. What is missing is the:<version>tag (and:latestunderMOVE_LATEST=1) for those images, so a promoted release does not carry the tags it claims to.The registry shows how long that has been true for the two host-split images.
keycloak-operatorcarries exactly one tag,latest, though the package has shipped since 2024-11.kubeovn's newest cozystack-versioned tag isv1.14.11-v0.38.8, from before the 1.0 versioning switch — whilecilium, which uses an already-recognised shape, carriesv1.5.0throughv1.6.0-rc.2. Neither has received a tag from any 1.x release.Measured against the committed tree, the dry-run selects 19 unique repositories before this change and 29 after — the eight digest-in-tag images plus
keycloak-operatorandkubeovn, nothing lost.What the nightly does not recover
Four of the ten are not in the root
Makefile'sbuildtarget:kilo,redis-operator,keycloak-operatorandkubeovn. Their baked refs keep the committed GHCR host, so theSRC_REGISTRYfilter still drops them and their:nightlytags remain absent after this PR. Installs are unaffected — those digests already resolve on GHCR. The nightly recovers the six OCIR-built images; promotion recovers all ten.Why this was invisible
Latent since
nightly-mirror.shwas added — the nightly had never completed a full run, so its mirror step was never exercised end to end.Both test suites were green throughout.
nightly-mirror_test.bats's fixture covered the single-string,{repository,tag,digest}and platformSource shapes — a layout only cilium actually uses — and omitted the dominant one.promote-retag_test.batsasserted "at least one cozystack-owned image is selected", which is satisfied by the shapes that already worked and cannot detect an omission.Two comments actively pointed the wrong way, and are corrected here. The header presented its list as the shapes the build writes, which reads as exhaustive; it now says the list is empirical, names what an audit found outside it, and says to re-run that audit when a package is added. The ownership-filter comment named
kilo,kube-ovnandkeycloak-operatoras third parties "not pushed to$REGISTRY" — all three are pushed there, and that wrong comment is part of why the missing tags went unnoticed.Both fixtures are extended here. The promotion test names concrete packages rather than asserting a count or a generic pattern, since either would drift back to proving nothing.
Type guards
Both digest-in-tag rules check
.tagis a string beforetest(). Without it a non-string tag aborts the whole expression (cannot match with !!int, can only match strings), and since these invocations swallow stderr and status, that abort would silently discard every ref of this shape in the same file — reintroducing this very bug in any file that happens to carry an unquotedtag: 1.24next to a cozystack image. Rule 1 is immune because itsselect(tag == "!!str")already precedes itstest().registry,repositoryanddigestare guarded too, against a narrower failure. Those keys are concatenated rather thantest()ed, and yq coerces int, bool, null and seq on+— each merely yields a ref the ownership filter discards. Only a!!mapaborts, raising!!str () cannot be added to a !!map, which takes the file's whole expression with it. So that guard is insurance against a nested-map typo in a vendored chart, not against unquoted scalars. It is aselect(), so an offending map drops only itself and its neighbours still resolve.No non-string value of either kind exists in the tree today, so both are prevention rather than an active fix.
Testing
nightly-mirror_test.bats2/2,promote-retag_test.bats4/4.global.registry.addressrule, and therepositorytype guard. Also verified that a rule matching the bare tag instead of the repository (the subtle wrong fix) fails, which is why the assertions check the full source ref rather than only the destination.promote-retag.sh --dry-runagainst the real committed tree: 19 → 29 unique repositories, no losses.@sha256:digest underpackages/*/*/values.yamlagainst the selector output accounts for all 35: 30 are selected, and the remaining five are third-party (ghcr.io/lexfrei/{kuberture,ouroboros},docker.io/clastix/kubectl,ghcr.io/kvaps/...) or sit outside$REGISTRY(ghcr.io/cozystack/ingress-nginx-with-protobuf-exporter/*).shellcheckclean on both scripts at default severity and-S style.Ordering note for reviewers
Worth landing before the next release candidate is cut, rather than treating this as routine CI cleanup.
promote-rc.yamlbuilds the release branch from the rc's staging branch, andpull-requests-release.yamlruns./hack/promote-retag.shfrom that merged tree — so the copy of the script that executes at promotion is the one frozen into the rc tag's tree, not whatever is on the default branch at promotion time. An rc cut before this merges will carry the old script and silently skip those retags when promoted.The nightly half is also what unblocks the full-suite e2e lane, which cannot currently publish a working install closure to GHCR.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff: it touches onlyhack/nightly-mirror.sh,hack/promote-retag.shand their two.batssuites. Nopackages/, no chart, novalues.schema.json, noApplicationDefinition, no CRD or RBAC. Thecozystack/ccptrigger covers moving or renaming files underhack/and changing what a make target does; neither applies — the anchor files it gates on (hack/package.mk,hack/common-envs.mk) are untouched and no make target changes behaviour.Release note
Summary by CodeRabbit
tagfield and optional registry wrappers.tag-embedded digests, numeric-tag variants, and more “owned” image assertions.