ci: overlay current-main images for packages a PR didn't rebuild - #3148
Conversation
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 improves the CI pipeline by ensuring that packages not explicitly rebuilt in a PR use the latest 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. Ignored Files
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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds touched-package tracking to the PR workflow, overlays current-main image refs onto unrebuilt packages during finalize, and expands shell tests for overlay selection, drift handling, and no-op cases. ChangesMain-image overlay for non-rebuilt packages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces 'hack/overlay-main-images.sh' and its corresponding unit tests to overlay current-main image references onto packages that were not rebuilt in the PR. The feedback highlights a potential issue where pretty-printed or multiline 'BUILT_JSON' inputs containing newlines could break the substring matching logic, leading to incorrect image overwrites. It is recommended to normalize the whitespace in the JSON input and add a unit test to ensure robustness against multiline inputs.
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.
| fi | ||
|
|
||
| # Surround with spaces so the `*" $u "*` membership test matches whole units. | ||
| built=" $(echo "$BUILT_JSON" | tr -d '[]"' | tr ',' ' ') " |
There was a problem hiding this comment.
If BUILT_JSON is formatted with newlines or extra whitespace (which is common for JSON inputs in CI/CD pipelines), the simple tr replacement will leave newlines in the built variable. This causes the substring matching *" $u "* to fail because the character preceding or succeeding the package name might be a newline instead of a space. As a result, packages that the PR actually rebuilt will not be skipped, and their fresh images will be overwritten by the main images, defeating the purpose of the PR build.
Using printf and normalizing all whitespace (including newlines) to spaces ensures robust matching.
| built=" $(echo "$BUILT_JSON" | tr -d '[]"' | tr ',' ' ') " | |
| built=" $(printf '%s\n' "$BUILT_JSON" | tr -d '[]"' | tr ',\n' ' ' | tr -s ' ') " |
| } | ||
|
|
There was a problem hiding this comment.
Adding a test case to verify that overlay-main-images.sh correctly handles pretty-printed/multiline BUILT_JSON inputs ensures that the whitespace normalization is robust and prevents regressions.
}
@test "skips a unit when BUILT_JSON is pretty-printed with newlines" {
root=$(pwd)
w=$(mktemp -d); trap 'rm -rf "$w"' EXIT
mkdir -p "$w/hack" "$w/packages/apps/foo" "$w/main/apps/foo"
cp "$root/hack/build-matrix.sh" "$w/hack/"; chmod +x "$w/hack/build-matrix.sh"
printf 'build:\\n\\tmake -C packages/apps/foo image\\n' > "$w/Makefile"
echo 'image: ghcr.io/cozystack/cozystack/foo:v1.5.0@sha256:aaaa' > "$w/packages/apps/foo/values.yaml"
echo 'image: iad.ocir.io/x/cozystack/foo:main@sha256:bbbb' > "$w/main/apps/foo/values.yaml"
cd "$w"
pretty_json='[
"packages/apps/foo"
]'
"$root/hack/overlay-main-images.sh" main "$pretty_json"
grep -q 'foo:v1.5.0@sha256:aaaa' packages/apps/foo/values.yaml
}
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/overlay-main-images.sh`:
- Around line 80-89: The drift validation in overlay-main-images.sh is too broad
because the img_line whitelist matches any repository/tag/digest key in
values.yaml, not just image fields. Tighten the check in the self-validation
block around img_line and the diff|grep gate so it only accepts known image
paths/blocks (or switch to a YAML-aware matcher) and does not treat unrelated
keys like backup.tag as image-related. Add a regression test case that changes a
non-image tag field to ensure the script still flags it as drift and does not
copy it in cp.
🪄 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: 319a4a6e-942e-42e2-bc70-4ca38e84e7ba
📒 Files selected for processing (3)
.github/workflows/pull-requests.yamlhack/overlay-main-images.shhack/overlay-main-images_test.bats
66a1c73 to
f4318d3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/overlay-main-images_test.bats (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Bats cleanup hooks instead of
EXITtraps.These new per-test
trap ... EXIThandlers violate the repo’s*.bats/*.shconvention. Please move temp-dir cleanup intoteardown()and drop the traps. As per coding guidelines,**/*.{bats,sh}: “noEXIT/RETURNtraps”.Proposed cleanup pattern
+teardown() { + [ -n "${w:-}" ] && rm -rf "$w" +} + `@test` "overlays an unbuilt unit (.tag) to current-main and reports it" { root=$(pwd) - w=$(mktemp -d); trap 'rm -rf "$w"' EXIT + w=$(mktemp -d) mkdir -p "$w/packages/apps/foo/images" "$w/main/apps/foo/images" ... }Also applies to: 28-28, 41-41, 52-52, 63-63, 77-77, 88-88, 100-100
🤖 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/overlay-main-images_test.bats` at line 16, The test setup in the Bats file is using per-test EXIT traps for temp-dir cleanup, which conflicts with the repo’s bats/sh convention. Update the affected test cases in overlay-main-images_test.bats to remove the trap-based cleanup and instead declare a teardown() function that removes the temp directory used by each test; keep the temp-dir variable scoped so teardown() can clean it up reliably. Use the test setup blocks and teardown() hook as the entry points to locate and replace the existing mktemp/trap pattern.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@hack/overlay-main-images_test.bats`:
- Line 16: The test setup in the Bats file is using per-test EXIT traps for
temp-dir cleanup, which conflicts with the repo’s bats/sh convention. Update the
affected test cases in overlay-main-images_test.bats to remove the trap-based
cleanup and instead declare a teardown() function that removes the temp
directory used by each test; keep the temp-dir variable scoped so teardown() can
clean it up reliably. Use the test setup blocks and teardown() hook as the entry
points to locate and replace the existing mktemp/trap pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 064bff9b-9547-48ee-94b3-22b8811f3cb8
📒 Files selected for processing (3)
.github/workflows/pull-requests.yamlhack/overlay-main-images.shhack/overlay-main-images_test.bats
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-requests.yaml
|
myasnikovdaniil this is now gating the entire PR e2e queue: since #2922 landed, every PR rebased onto current |
Conditional PR builds (the per-package matrix in pull-requests.yaml) only rebuild packages whose dir changed. Every other package falls back to the image refs committed in the tree — the last release's digests (currently v1.5.0), which go stale as main advances. So e2e and the installer's packages artifact ran current-main configs against last-release images for everything outside the PR's build matrix, and a PR installed via the cozy-installer chart was an incoherent mix. This is the systemic gap in issue #3143 (its concrete bite: the merged objectstorage-controller requeue fix #3034 never ran in e2e, flaking the harbor BucketClaim check). In finalize, pull the cozystack-packages:main OCI artifact that build-main already publishes (the whole current-main packages tree, refs digest-pinned to the images build-main pushed) and walk every ref-bearing file in it (values.yaml + images/*.tag), overlaying the repo's copy. This is driven by the artifact, not the build matrix, so it covers the ENTIRE first-party tree — apps/system/core AND extra/, library/ — and a merged fix to any first-party image (e.g. the objectstorage-sidecar ref under extra/seaweedfs) takes effect. For a file build-main did not rebuild the artifact copy is byte-identical and is skipped. Skipped explicitly: units the PR itself rebuilt (their pr-<N>-<sha> refs win), packages/core/talos and packages/core/installer (owned by their dedicated build-talos / finalize-installer jobs), and vendored charts/ subtrees. The copy is self-validating: a file is overlaid only when every differing line is image-reference-bearing (a full @sha256: ref, or a split image/repository/registry/tag/digest key, or a --…-image= arg); otherwise it keeps its committed ref. A missing artifact is a no-op (prior behaviour) and the step is best-effort so it never blocks a PR. No build-main change is needed: cozystack-packages:main is already published by `make build`. Runs before the installer build so the flux-pushed packages artifact and the pr.patch e2e applies both reflect the overlay. Result: a PR is tested and installed as current-main + its own changes, coherent across config and image, instead of current-main configs running on last-release images. Covered by hack/overlay-main-images_test.bats (run by `make unit-tests` in the checks job): single-line and split-form ref overlay, extra/* coverage, skip-rebuilt, talos/installer skip, vendored-charts prune, drift self-validation, and missing-artifact no-op. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
f4318d3 to
52f7d05
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hack/overlay-main-images.sh (1)
62-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNarrow the split-ref whitelist to actual image fields.
This still whitelists any
repository:,registry:,tag:ordigest:key invalues.yaml, so unrelated config drift can slip through Line 91 and get copied into the PR tree. Tighten the match to known image paths/blocks (or use a YAML-aware check) and add a regression case for a non-imagetag:change.Also applies to: 91-95
🤖 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/overlay-main-images.sh` around lines 62 - 64, The split-ref whitelist in the image-diff logic is too broad because the img_line pattern in hack/overlay-main-images.sh matches any repository, registry, tag, or digest key, allowing unrelated values.yaml drift to pass through. Tighten the match in the overlay/copy flow that uses img_line so it only recognizes known image paths or image-related blocks (or replace it with a YAML-aware check), and add a regression test covering a non-image tag change to ensure it is not treated as an image reference.
🤖 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.
Duplicate comments:
In `@hack/overlay-main-images.sh`:
- Around line 62-64: The split-ref whitelist in the image-diff logic is too
broad because the img_line pattern in hack/overlay-main-images.sh matches any
repository, registry, tag, or digest key, allowing unrelated values.yaml drift
to pass through. Tighten the match in the overlay/copy flow that uses img_line
so it only recognizes known image paths or image-related blocks (or replace it
with a YAML-aware check), and add a regression test covering a non-image tag
change to ensure it is not treated as an image reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e5a9408-ad7e-4ce5-98c7-00fd85b174c6
📒 Files selected for processing (3)
.github/workflows/pull-requests.yamlhack/overlay-main-images.shhack/overlay-main-images_test.bats
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pull-requests.yaml
- hack/overlay-main-images_test.bats
The current-main overlay skipped only rebuilt units (plus talos/installer), so for a package the PR edited but did not rebuild — an upstream image bump in a non-build-unit package such as keycloak or cert-manager — the committed ref differed from the main artifact on image-reference lines only, the drift check passed, and the overlay reverted the PR's edit back to current-main. e2e and the installer artifact then ran against the stale image. Pass the PR's edited package dirs (derived from the plan job's git diff) into the overlay skip set so a PR's own image-ref edits win over the artifact. Add tests pinning the preserve behaviour and covering the --image= arg form and the file-absent-in-PR-tree branch. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the overlay points unbuilt packages at current-main images, and the skip-set now preserves a PR's own image-ref edits to packages it did not rebuild.
Overlaying from the cozystack-packages:main artifact onto packages a PR didn't rebuild is the right fix for the stale-ref / unpublished-image problem, and the safe-degradation paths (missing artifact, drift, absent unit) are handled. A follow-up commit closes the one regression found in review: the skip-set now also covers packages the PR edited (derived from the plan job's git diff), so an upstream image bump in a non-build-unit package (keycloak, cert-manager, …) is no longer silently reverted to current-main. Covered by tests — preserve-edited-package, the --…-image= arg form, and file-absent-in-PR-tree; all unit tests pass, shellcheck/actionlint clean on the changed lines.
Non-blocking: skipping any edited package means a package edited for a non-image reason also keeps its committed image ref — a deliberate, conservative tradeoff that favors PR intent and matches pre-overlay behavior. Image effectiveness still depends on the cozystack-packages:main artifact being published for current main; the e2e run on this head will confirm the overlay actually takes effect.
flux pull artifact --output requires the target directory to already exist; it stats the path before any network call and aborts with "invalid output path: stat _out/mainpkgs: no such file or directory" when the dir is missing. The step deleted _out/mainpkgs with rm -rf but never recreated it, so flux failed locally on every run and the || fallback misreported the failure as "artifact not published" — leaving the current-main overlay a permanent no-op even though the artifact exists and is anonymously pullable. Add mkdir -p after the rm -rf so the directory exists before flux pull, and reword the fallback message so a genuine future pull failure is no longer mistaken for an absent artifact. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — re-approving on the current head after the overlay fixes.
The overlay correctly points unbuilt packages at current-main images (from the cozystack-packages:main artifact), with two corrections that make it actually work: packages the PR edited are preserved in the skip-set (an upstream bump in a non-build-unit package is no longer silently reverted), and the output dir is created before flux pull (the artifact is now actually pulled instead of the step failing locally and falling back to committed refs). Mechanism verified, unit tests pass, registry/name/tag match the publisher.
What this PR does
Conditional PR builds (the per-package matrix in
pull-requests.yaml) only rebuild packages whose dir changed. Every other package falls back to the image refs committed in the tree — the last release's digests (currentlyv1.5.0), which go stale asmainadvances. So e2e and the installer's packages artifact ran current-main configs against last-release images for everything outside a PR's build matrix, and a PR installed via thecozy-installerchart was an incoherent mix (your changes fresh, everything else at the last release).In
finalize, this overlays current-main image refs onto every package the PR did not rebuild:cozystack-packages:mainOCI artifact thatbuild-main.yamlalready publishes (make build→ installerimage-packages) — the whole current-main packages tree, refs digest-pinned to the imagesbuild-mainpushed. Nobuild-mainchange is needed.values.yaml+images/*.tag) from that tree. Units the PR rebuilt keep their freshpr-<N>-<sha>refs.@sha256:ref, or a splitimage/repository/registry/tag/digestkey, or a--…-image=arg); otherwise the unit is left on its committed ref.Runs after the registry login and before the installer build, so both the installer's
cozystack-packages:<pr>artifact and thepr.patche2e applies reflect the overlay.Net effect: a PR is tested and installed as current-main + its own changes, coherent across config and image.
Covered by
hack/overlay-main-images_test.bats(auto-discovered bymake unit-tests→bats-unit-tests, run in thechecksjob): single-line and split-form ref overlay, skip-rebuilt, drift self-validation, and missing-artifact no-op.Release note
Summary by CodeRabbit
CI / New Features
Tests