refactor(build): parallel matrix image builds on ephemeral runners - #2983
Conversation
hack/build-matrix.sh emits the CI build matrix (the list of package dirs to build) from the root Makefile `build:` target — the single source of truth — so the matrix can't drift from `make build`. It supports diff-driven scoping: only units whose package dir changed are emitted, unless a shared-dependency path (cozy-lib, the build macros, go.mod, the build workflows) is touched, in which case the full matrix is emitted. talos and installer are excluded from the parallel matrix: talos shares heavy _out/assets between its image and the nocloud disk (built once in a dedicated leg), and installer's `flux push artifact --path=packages` bundles the whole digest-patched tree, so it must run in finalize after all other fragments merge. hack/build-matrix_test.bats covers full/scoped/empty selection, the talos+installer exclusion, shared-dep escalation, and JSON/Makefile parity. Refs #2937 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>
Replaces the single serial Build job (one ephemeral VM running `make build`, ~26 images one after another, wedging on the shared buildkit under concurrent load — #2937) with a fan-out: plan changed files + hack/build-matrix.sh -> scoped matrix checks unit + controller Go tests (decoupled from e2e) build one package per job, own docker-container buildx, uploads a per-package digest patch fragment build-talos talos image + nocloud disk (its own leg; e2e needs it) finalize apply all fragments, then build the installer LAST (its flux-push bundles the whole patched tree) and emit the unified pr.patch e2e consumes unchanged build-matrix.sh scopes the matrix to the packages a PR touches (full set on shared-dependency changes) and fans seaweedfs out to objectstorage-controller. common-envs.mk gains an SBOM=1 flag wiring buildx --sbom, kept OFF until the OCIR referrers probe confirms support. .github/actionlint.yaml registers the custom ephemeral runner labels. Refs #2937 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>
build-parity.yaml (workflow_dispatch) builds both ways on the same commit and asserts the matrix decomposition patches the same tracked files as the legacy serial `make build` — the cutover gate for #2937. It compares the patched file SET, not the patch bytes, because image digests are not reproducible across two independent builds. Also probes whether the registry accepts SBOM attestations, gating SBOM=1. TEMPORARY — to be removed once parity is confirmed (header notes this). Refs #2937 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>
build-main.yaml now warms the mode=max cache on the ephemeral pool (oracle-vm-24cpu) instead of the dedicated self-hosted runner, so no build path depends on that host (#2937 decommission). Adds an idempotent flux install for the installer's image-packages step and bumps the timeout for the serial build on the ephemeral shape. The self-hosted runner remains referenced only by the `debug`-label breakpoint path in e2e; fully de-registering it is a separate infra step. Refs #2937 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>
The "What TIA does and does not do" section described the old single Build job and the `detect-changes` paths-filter. Update it for the new `plan` job (docs gate + build matrix), note that image builds are now diff-scoped by hack/build-matrix.sh (distinct from TIA's app-loop trim), and point the "widen the skip" advice at the plan docs-gate. Refs #2937 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>
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 refactors the CI build pipeline to improve efficiency and reliability. By transitioning from a serial build process to a parallel, diff-scoped matrix, the system now only builds images affected by specific PR changes, significantly reducing wall-time. The changes include new infrastructure for ephemeral runners, improved documentation on E2E testing scopes, and the addition of optional SBOM generation support. 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:
📝 WalkthroughWalkthroughUpdates PR CI to plan package-specific builds, split image generation into parallel jobs, assemble a unified patch in finalize, and run E2E from the new workflow path. Also adds Oracle runner support, SBOM build args, and refreshed E2E docs. ChangesParallel Matrix CI Build Refactor
Sequence Diagram(s)sequenceDiagram
participant plan
participant checks
participant build as build jobs
participant build_talos as build-talos
participant finalize
participant e2e
plan->>plan: diff and run build-matrix.sh
plan-->>checks: code gate
plan-->>build: matrix output
build->>build: make build package
build-->>finalize: patch fragment artifact
build_talos->>build_talos: build Talos + nocloud disk
build_talos-->>finalize: Talos patch fragment
finalize->>finalize: git apply fragments
finalize->>finalize: build installer/operator
finalize-->>e2e: pr.patch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 a new build-matrix generator script (hack/build-matrix.sh) and its corresponding unit tests (hack/build-matrix_test.bats) to dynamically compute the CI build matrix from the root Makefile. It also updates actionlint configurations, documentation, and adds optional SBOM support in hack/common-envs.mk. The review feedback highlights a potential POSIX shell syntax error in hack/build-matrix.sh when emitting an empty list, and suggests replacing fragile tr and wc -l JSON array length assertions in the test suite with robust jq commands.
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.
| emit_json() { | ||
| printf '[' | ||
| _first=1 | ||
| for _u in $1; do | ||
| if [ "$_first" -eq 1 ]; then _first=0; else printf ','; fi | ||
| printf '"%s"' "$_u" | ||
| done | ||
| printf ']\n' | ||
| } |
There was a problem hiding this comment.
In strict POSIX shells like dash (the default /bin/sh on Ubuntu GitHub Actions runners), a for loop with an empty word list (e.g., for _u in ; do) results in a syntax error. If $1 is empty (which happens when no packages are selected, such as in docs-only PRs), for _u in $1 will trigger this syntax error and fail the CI pipeline.
To prevent this, wrap the loop in a non-empty check.
| emit_json() { | |
| printf '[' | |
| _first=1 | |
| for _u in $1; do | |
| if [ "$_first" -eq 1 ]; then _first=0; else printf ','; fi | |
| printf '"%s"' "$_u" | |
| done | |
| printf ']\n' | |
| } | |
| emit_json() { | |
| printf '[' | |
| _first=1 | |
| if [ -n "${1:-}" ]; then | |
| for _u in $1; do | |
| if [ "$_first" -eq 1 ]; then _first=0; else printf ','; fi | |
| printf '"%s"' "$_u" | |
| done | |
| fi | |
| printf ']\n' | |
| } |
| # The parallel units; assert known members are present. | ||
| echo "$out" | grep -q '"packages/core/platform"' | ||
| echo "$out" | grep -q '"packages/apps/mariadb"' | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -gt 20 ] |
There was a problem hiding this comment.
Using tr and wc -l to count elements in a JSON array is fragile and can produce incorrect counts depending on formatting or empty arrays (e.g., an empty array [] has 0 elements but would count as 1 line). Since jq is already a dependency and used elsewhere in the tests, use it here for a robust and precise count.
[ "$(echo "$out" | jq '. | length')" -gt 20 ]
| out=$(hack/build-matrix.sh "$tmp") | ||
| echo "$out" | grep -q '"packages/apps/mariadb"' | ||
| echo "$out" | grep -q '"packages/system/dashboard"' | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -eq 2 ] |
There was a problem hiding this comment.
Use jq to robustly assert the length of the JSON array instead of relying on tr and wc -l.
[ "$(echo "$out" | jq '. | length')" -eq 2 ]
| echo "packages/library/cozy-lib/templates/_helpers.tpl" > "$tmp" | ||
| out=$(hack/build-matrix.sh "$tmp") | ||
| echo "$out" | grep -q '"packages/core/platform"' | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -gt 20 ] |
There was a problem hiding this comment.
Use jq to robustly assert the length of the JSON array instead of relying on tr and wc -l.
[ "$(echo "$out" | jq '. | length')" -gt 20 ]
| tmp=$(mktemp); trap 'rm -f "$tmp"' EXIT | ||
| echo "hack/common-envs.mk" > "$tmp" | ||
| out=$(hack/build-matrix.sh "$tmp") | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -gt 20 ] |
There was a problem hiding this comment.
Use jq to robustly assert the length of the JSON array instead of relying on tr and wc -l.
[ "$(echo "$out" | jq '. | length')" -gt 20 ]
| tmp=$(mktemp); trap 'rm -f "$tmp"' EXIT | ||
| echo "go.mod" > "$tmp" | ||
| out=$(hack/build-matrix.sh "$tmp") | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -gt 20 ] |
There was a problem hiding this comment.
Use jq to robustly assert the length of the JSON array instead of relying on tr and wc -l.
[ "$(echo "$out" | jq '. | length')" -gt 20 ]
| tmp=$(mktemp); trap 'rm -f "$tmp"' EXIT | ||
| echo ".github/workflows/pull-requests.yaml" > "$tmp" | ||
| out=$(hack/build-matrix.sh "$tmp") | ||
| [ "$(echo "$out" | tr ',' '\n' | wc -l)" -gt 20 ] |
There was a problem hiding this comment.
Use jq to robustly assert the length of the JSON array instead of relying on tr and wc -l.
[ "$(echo "$out" | jq '. | length')" -gt 20 ]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/pull-requests.yaml (3)
21-51: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit permissions to the
planjob.The
planjob only needs read access to repository contents. Adding an explicit permissions block follows the principle of least privilege and silences the static analysis warning.🔒 Suggested fix
plan: name: Plan build runs-on: ubuntu-latest + permissions: + contents: read outputs: code: ${{ steps.p.outputs.code }}🤖 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 @.github/workflows/pull-requests.yaml around lines 21 - 51, The plan job in the GitHub Actions workflow is missing explicit permissions declaration. Add a permissions block to the plan job (the job with name "Plan build") immediately after the outputs section and before the steps section. This permissions block should specify read-only access to the repository contents using the contents permission set to read. This will follow the principle of least privilege and resolve static analysis warnings about missing permissions.Source: Linters/SAST tools
56-78: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit permissions to the
checksjob.Similar to the
planjob, this job only needs read access.🔒 Suggested fix
checks: name: Unit & controller tests runs-on: ${{ contains(github.event.pull_request.labels.*.name, 'debug') && 'self-hosted' || 'oracle-vm-4cpu-16gb-x86-64' }} timeout-minutes: 30 + permissions: + contents: read needs: ["plan"]🤖 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 @.github/workflows/pull-requests.yaml around lines 56 - 78, The `checks` job is missing explicit permissions declaration. Add a `permissions` field to the `checks` job (the job that runs the "Unit & controller tests") at the same level as the `runs-on`, `timeout-minutes`, and `needs` fields. Set the permissions to read-only access (read-all) since this job only needs to checkout code and run tests without requiring write access to the repository. This aligns the security posture with the `plan` job which already has explicit permissions defined.Source: Linters/SAST tools
109-112: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider pinning the Flux install script or using a versioned release.
Piping
curldirectly tobashis a supply chain risk. If the script atfluxcd.iois compromised, it could execute malicious code. This is a common pattern but worth hardening for CI security.Alternative: download a specific release binary with checksum verification, or pin to a known-good version.
🤖 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 @.github/workflows/pull-requests.yaml around lines 109 - 112, The "Set up build toolchain" step downloads and executes an install script via curl piping to bash, which poses a supply chain security risk. Replace this pattern by downloading a specific versioned release of the Flux binary directly (rather than executing an untrusted script), preferably with checksum verification to ensure integrity. Alternatively, if using the install script, pin it to a specific known-good version rather than always fetching the latest..github/workflows/build-parity.yaml (1)
36-36: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider pinning actions to commit SHAs.
Static analysis flags unpinned action references. While using version tags (e.g.,
@v4,@v3) is common and provides stability, pinning to commit SHAs offers stronger supply-chain security by preventing tag-rewriting attacks.Example: pinning checkout action
- uses: actions/checkout@v4 + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1Note: This may be a project-wide convention decision. If the project prefers version tags for maintainability, feel free to defer this suggestion.
Also applies to: 53-53, 63-63, 124-124
🤖 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 @.github/workflows/build-parity.yaml at line 36, Replace version tag references with full commit SHAs for improved supply-chain security. Update all instances of actions/checkout@v4 (and any other action references at lines 53, 63, and 124) to use their specific commit SHA instead of the version tag. For example, change actions/checkout@v4 to actions/checkout@<full-commit-sha> where the SHA corresponds to the specific release version you want to use. This prevents potential tag-rewriting attacks while maintaining the stability of your workflow.Source: Linters/SAST tools
🤖 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 @.github/workflows/build-parity.yaml:
- Around line 35-39: The checkout action in the workflow is using the default
persist-credentials behavior which stores the GitHub token in .git/config,
creating a potential security risk since artifacts containing git diffs are
uploaded. Add persist-credentials: false to the with: parameters of the
actions/checkout@v4 step to prevent the credentials from being stored in the git
configuration.
---
Nitpick comments:
In @.github/workflows/build-parity.yaml:
- Line 36: Replace version tag references with full commit SHAs for improved
supply-chain security. Update all instances of actions/checkout@v4 (and any
other action references at lines 53, 63, and 124) to use their specific commit
SHA instead of the version tag. For example, change actions/checkout@v4 to
actions/checkout@<full-commit-sha> where the SHA corresponds to the specific
release version you want to use. This prevents potential tag-rewriting attacks
while maintaining the stability of your workflow.
In @.github/workflows/pull-requests.yaml:
- Around line 21-51: The plan job in the GitHub Actions workflow is missing
explicit permissions declaration. Add a permissions block to the plan job (the
job with name "Plan build") immediately after the outputs section and before the
steps section. This permissions block should specify read-only access to the
repository contents using the contents permission set to read. This will follow
the principle of least privilege and resolve static analysis warnings about
missing permissions.
- Around line 56-78: The `checks` job is missing explicit permissions
declaration. Add a `permissions` field to the `checks` job (the job that runs
the "Unit & controller tests") at the same level as the `runs-on`,
`timeout-minutes`, and `needs` fields. Set the permissions to read-only access
(read-all) since this job only needs to checkout code and run tests without
requiring write access to the repository. This aligns the security posture with
the `plan` job which already has explicit permissions defined.
- Around line 109-112: The "Set up build toolchain" step downloads and executes
an install script via curl piping to bash, which poses a supply chain security
risk. Replace this pattern by downloading a specific versioned release of the
Flux binary directly (rather than executing an untrusted script), preferably
with checksum verification to ensure integrity. Alternatively, if using the
install script, pin it to a specific known-good version rather than always
fetching the latest.
🪄 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: a8e30bd5-9ab5-4107-a48d-2fd536d43920
📒 Files selected for processing (8)
.github/actionlint.yaml.github/workflows/build-main.yaml.github/workflows/build-parity.yaml.github/workflows/pull-requests.yamldocs/agents/e2e-testing.mdhack/build-matrix.shhack/build-matrix_test.batshack/common-envs.mk
finalize (and therefore e2e, which needs it) now depends on the checks job succeeding, so a red unit/controller test skips the expensive install+e2e path instead of running it for ~1h. Restores the gating the old in-build-job tests gave, without re-coupling the cheap parallel image builds. Refs #2937 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>
The full-matrix PR run validated the decomposition functionally (green install + 21/22 app tests on the matrix-built tree), stronger evidence than the file-set parity diff would give — and it avoids running the serial make build the parity job exists to compare against. SBOM stays capability-only (common-envs.mk) until an OCIR referrers probe is run when SBOM is actually pursued. Refs #2937 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>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM (REQUEST_CHANGES)
Business context: this replaces the serial single-VM make build with a diff-scoped parallel matrix on ephemeral runners, so most PRs build only the images they touch instead of all ~26 serially.
Blocker
1. Root Go source changes (api/, cmd/, internal/, pkg/) rebuild nothing, so e2e tests stale binaries.
hack/build-matrix.sh's full_rebuild_pattern lists packages/library/, the build macros, Makefile, go.mod/go.sum, the build workflows, and itself — but not api/, cmd/, internal/, or pkg/. A PR that touches only those root directories matches neither the full-rebuild pattern nor any ^packages/<dir>/ scope, so the selector emits [], plan sets any=false, and the build matrix job is skipped. Eight matrix-built images embed the root Go module via COPY api pkg cmd internal: cozystack-api, cozystack-controller, lineage-controller-webhook, backup-controller, backupstrategy-controller, flux-plunger, flux-shard-operator, kubeovn-plunger. None is rebuilt, no digest fragment is produced, finalize carries the old digests into pr.patch, and e2e installs the previously-built images — so a change to operator/API/controller Go code is never exercised by e2e, silently. This is a regression from the old make build, which rebuilt every image whenever code==true, and it contradicts this selector's own stated principle that "a missed dependent ships a stale image." It also diverges from the sibling hack/select-e2e.sh, whose full_suite_pattern does include api/|cmd/|internal/ — so the test selector escalates to the full app suite for these changes and then runs that suite against stale images.
Evidence: hack/build-matrix.sh:25 (pattern omits the root Go dirs); .github/workflows/pull-requests.yaml:88-90 (build job gated on any == 'true'); packages/system/cozystack-api/images/cozystack-api/Dockerfile:11-14 plus the seven sibling controller/webhook Dockerfiles all COPY api/pkg/cmd/internal; hack/select-e2e.sh:22 (full_suite_pattern already includes api/|cmd/|internal/).
Fix: add api/, cmd/, internal/, pkg/ to full_rebuild_pattern, mirroring select-e2e.sh and adding pkg/ since the Dockerfiles copy it as well.
Non-blocking
-
The new selector's bats suite is not run in CI, and it has no case covering the root-Go escalation above.
build-matrix.shis the single gate deciding what gets built; a future parse regression (aMakefilereformat breaking thesed/grepchain, or the gap above) would ship undetected. Consider runninghack/build-matrix_test.bats(andhack/select-e2e_test.bats) in thechecksjob and adding a Go-source-escalation case. Evidence: nobats/build-matrixinvocation anywhere under.github/workflows/. -
command -v flux >/dev/null || curl -s https://fluxcd.io/install.sh | sudo bashis unpinned remote root code execution with neither--failnor a version pin, repeated across the build, build-talos, finalize, and warm-cache jobs. It floats the flux version between runs (non-reproducible) and is a supply-chain exposure. Pin a version and verify a checksum until flux is baked into the runner image. Evidence:.github/workflows/pull-requests.yaml:110-112,264-266;.github/workflows/build-main.yaml:59-61. -
finalize's fragment-apply loop runsgit apply "$f"with no--3wayor conflict handling, on the assumption each fragment touches only its own package. That holds today, but image targets can write outside their own dir (e.g.packages/apps/kuberneteswritespackages/system/kubevirt-csi-node/values.yaml); if two fragments ever edit the same file, the secondgit applyaborts the wholefinalize. Worth a guard or an explicit comment recording the invariant. Evidence:.github/workflows/pull-requests.yaml:276-281;packages/apps/kubernetes/Makefile:56. -
The PR is currently in a merge-conflict state against the base branch and needs a rebase before merge. Evidence: GitHub reports the PR as CONFLICTING.
|
|
||
| # Paths that force a full rebuild when touched. Kept deliberately broad — a | ||
| # false full-rebuild only costs time, a missed dependent ships a stale image. | ||
| full_rebuild_pattern='^(packages/library/|hack/common-envs\.mk|hack/package\.mk|Makefile$|go\.mod$|go\.sum$|\.github/workflows/(pull-requests|build-main)\.yaml$|hack/build-matrix\.sh$)' |
There was a problem hiding this comment.
full_rebuild_pattern omits the root Go module dirs api/, cmd/, internal/, pkg/. Eight matrix images COPY those dirs (cozystack-api, cozystack-controller, lineage-controller-webhook, backup-controller, backupstrategy-controller, flux-plunger, flux-shard-operator, kubeovn-plunger), so a PR changing only root Go source emits [], skips the build job, and e2e installs stale digests — the Go change is never exercised by e2e. hack/select-e2e.sh:22 already escalates on api/|cmd/|internal/; mirror it here and add pkg/.
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes.
A clean, well-commented CI refactor with no packages/ impact. The matrix selector, fragment-merge model, and finalize-ordered installer artifact build are all internally consistent, locally verifiable, and faithfully documented in build-matrix.sh and the workflow comments. Two operational nits below; neither blocks merge.
Findings
[MINOR] .github/workflows/pull-requests.yaml:158-162,222-226,228-231,321-325 — actions/upload-artifact@v4 has no overwrite: true on any of the four upload steps (pr-patch-fragment-<safe>, pr-patch-fragment-talos, talos-image, pr-patch).
v4's default is overwrite: false, which errors on a name collision. The happy path is unaffected — each matrix shard has a unique safe name — but a "re-run failed jobs" cycle that re-executes an already-succeeded finalize (e.g., one that produced pr-patch and was then marked failed by a downstream e2e flake on the same run) would hard-fail at the upload step before producing useful diagnostics. Add overwrite: true to at least the pr-patch and pr-patch-fragment-* uploads to make re-runs idempotent.
[MINOR] .github/workflows/pull-requests.yaml:119,191,290 (login if: !head.repo.fork) vs hack/common-envs.mk:37 (PUSH := 1) — fork PRs skip OCIR login but the matrix build still runs make image with the default PUSH=1, so the docker buildx --push=1 step will fail hard on a fork.
This is pre-existing behavior (the previous serial Build job had the same shape — fork PRs never produced PR images), not a regression. Note only because the refactor was an opportunity to either guard the build step on head.repo.fork (skip push, build-only) or document the fork-PR limitation in the workflow. Out of scope for this PR; tracking in #2937 is fine.
Claim mismatches
[PARTIAL] "the runner remains referenced only by the e2e debug breakpoint path" — grep -nE 'runs-on:' .github/workflows/*.yaml shows [self-hosted] is still pinned by auto-release.yaml:17, backport.yaml:26,96, pull-requests-release.yaml:17, release-e2e.yaml:26,71, tags.yaml:18,248,453, and update-releasenotes.yaml:15. The intended (and true) narrower statement is "no build path depends on it" — which the preceding sentence already says correctly, and the Follow-ups section reiterates ("de-register the self-hosted build runner"). The middle sentence is just imprecise; nothing in the PR is actually broken by this.
Red-team concerns
- Future cross-package sidecar deps must remember to edit
hack/build-matrix.sh. Today onlyobjectstorage-controllerreads another package'svalues.yamlat build time (packages/system/objectstorage-controller/Makefile:17,18,28reads../seaweedfs/values.yaml). Verified bygrep -rln '\.\./[a-z][a-z-]*/values\.yaml' packages/*/*/Makefilereturning only that file. The fanout is documented inhack/build-matrix.sh:76-85; the test exists (hack/build-matrix_test.bats:75-87). A new cross-package tag dep added without an accompanyingbuild-matrix.shedit will silently ship a stale sidecar tag. Hard to enforce in-tree; themake buildparity bats test (hack/build-matrix_test.bats:118-134) only catches count drift, not fanout drift. Worth a note inAGENTS.mdor a follow-up to declare such deps closer to the Makefile. - Makefile parser fragility.
hack/build-matrix.sh:40relies onsed -n '/^build:/,/^[^[:space:]]/p'extracting thebuild:recipe up to the next column-0 line. Anchored to that exact target name, it toleratesbuild-deps:(regex^build:does not matchbuild-deps:) — verified by running the script. A future Makefile refactor that splitsbuild:across includes, usesdefine, or renames the target will silently zero the matrix. The parity test at:118-134does guard against this — would still fail to flag mid-target reordering, but that is acceptable.
Caveats
- Phase 5b (upgrade + fresh-install impact): N/A. No files under
packages/, charts, CRDs, migrations, RBAC, ApplicationDefinitions, or committed image digests. The only artifact-relevance question is whether the new matrix-builtinstallerOCI artifact bundles the same digest-patched packages tree as the old serial path. Both paths runmake -C packages/core/installer imagelast; the new path first merges per-package digest fragments viagit applyinfinalize(.github/workflows/pull-requests.yaml:273-283), then runsimage(line 308), which callsflux push artifact --path=packages(packages/core/installer/Makefile:34). End-state packages tree is equivalent. Verified clean. - "First run was green through plan → 25 parallel builds → build-talos → finalize → install → 21/22 app tests" — UNVERIFIABLE without CI access; took as stated.
- Local validation reproduced:
bats hack/build-matrix_test.bats16/16;shellcheck hack/build-matrix.shclean;actionlint -config-file .github/actionlint.yaml .github/workflows/pull-requests.yaml .github/workflows/build-main.yamlclean;make -n SBOM=1 -C packages/apps/mariadb image | grep sbomshows--sbom=true, withoutSBOM=1no match. - Commits all use Conventional Commits with
ci(build):/docs(agents):prefixes, no Claude attribution, release-note block present (NONE).
Recommended follow-ups
- Add
overwrite: truetopr-patchandpr-patch-fragment-*upload-artifact steps to makefinalizere-runs idempotent. - Tighten the PR-body wording on the self-hosted-runner decommission to "no build path" (it is already correct in the surrounding sentences).
- Long-term: consider a declarative way to express cross-package tag dependencies (a sibling file next to the dependent package's Makefile that
hack/build-matrix.shreads), so the seaweedfs/objectstorage-controller pattern is not the sole template for future deps.
The matrix selector's full_rebuild_pattern omitted the root Go module dirs (api/, cmd/, internal/, pkg/). Eight matrix image Dockerfiles COPY those dirs, so a PR changing only root Go source emitted an empty matrix, skipped the build job, and let e2e install stale digests — the Go change was never exercised. Add the four dirs to the pattern and pin it with bats cases for api/- and pkg/-only diffs forcing the full matrix. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…apply
- Run hack/build-matrix_test.bats and hack/select-e2e_test.bats in the checks
job so a selector parse regression is caught in CI rather than shipping.
- curl -fsSL the flux install so an HTTP error page is not piped to sudo bash.
- git apply --3way in finalize so two fragments touching adjacent lines of a
shared file merge instead of the second apply aborting the whole step.
- Add least-privilege permissions: {contents: read} to the plan and checks jobs.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Aleksei Sviridkin (@lexfrei) addressed in
The branch still conflicts with |
make unit-tests already runs every hack/*.bats via bats-unit-tests (hack/cozytest.sh, a pure-shell bats runner that needs no bats binary), so the explicit step that apt-installed bats and re-ran build-matrix_test.bats / select-e2e_test.bats was redundant. The selector suites still run in CI through make unit-tests. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Reconcile the parallel-matrix PR build (#2983) with main, which since this branch diverged landed a competing single-VM build restructure (#2939) and test-impact-analysis E2E (#2559). Per maintainer decision, #2983's matrix build supersedes #2939: - .github/workflows/pull-requests.yaml: keep #2983's plan/checks/build(matrix)/build-talos/finalize jobs; keep main's newer resolve_assets/e2e (carrying #2559 test-impact E2E). The build->e2e artifact interface (pr-patch + talos-image) is identical on both sides, so e2e depends on finalize/build-talos with no input rewiring. main's top-level permissions and SHA-pinned action refs applied across all jobs. - All other files merged cleanly (only pull-requests.yaml conflicted). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Completes the SHA-pinning of pull-requests.yaml — the three setup-buildx-action references were the only remaining tag-pinned (@V3) actions after the merge. Pin them to v3.12.0's commit SHA so every action in the workflow is digest-pinned. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Re-sync the release-promotion PR (#3017) onto the #2983 base, which now contains main. The conflicts were release-pipeline files, resolved keeping #3017's write-once + promotion logic and grafting main's hardening: - tags.yaml: keep #3017's is_stable gate on the api-submodule tag and the auto-opened release PR; take main's SHA-pinned actions. - auto-release.yaml: kept deleted (#3017 removes the nightly auto-bump; main's hardening of a soon-deleted file is moot). - installer/values.yaml: keep #3017's platformVersion field + comment; take main's v1.5.0 operator image. actionlint clean; installer + promote-retag unit tests pass. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/agents/e2e-testing.md (1)
70-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify that only the package matrix is narrowed.
The workflow still always builds Talos for non-docs PRs and builds the installer in
finalize, so “rebuilds only that image” overstates the new behavior.Proposed wording
-- **It trims only the per-app test loop, not the expensive stages.** `Prepare environment` and `Install Cozystack` (the full platform install) run regardless of the selection. TIA only decides which `make test-apps-<app>` targets run in the final step. A perfect narrow saves the matrix tail, not the bulk of wall-time. (Image *builds* are scoped separately — by `hack/build-matrix.sh`, on the PR diff — so an app-only PR rebuilds only that image; that is a distinct mechanism from TIA's app-loop trimming.) +- **It trims only the per-app test loop, not the expensive stages.** `Prepare environment` and `Install Cozystack` (the full platform install) run regardless of the selection. TIA only decides which `make test-apps-<app>` targets run in the final step. A perfect narrow saves the matrix tail, not the bulk of wall-time. (Package image builds are scoped separately — by `hack/build-matrix.sh`, on the PR diff — while the Talos and installer/finalize legs still run for non-docs PRs; that is a distinct mechanism from TIA's app-loop trimming.)🤖 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 `@docs/agents/e2e-testing.md` at line 70, The wording in the e2e testing docs overstates what TIA narrows; update the explanation around the app loop and the build-matrix behavior so it clearly says only the package/test matrix is trimmed, not the full workflow. In the section describing `Prepare environment`, `Install Cozystack`, and the `hack/build-matrix.sh`-driven image selection, remove the claim that an app-only PR “rebuilds only that image” and instead note that Talos still builds for non-docs PRs and the installer still builds in `finalize`..github/workflows/pull-requests.yaml (1)
117-120: 🩺 Stability & Availability | 🟡 MinorAvoid masking failed Flux downloads.
On GitHub Actions,
set -o pipefailis not enabled by default. In these installs,curl -ffailing (e.g., due to 404 or network error) exits with a non‑zero code, but the failure is masked because the pipeline's exit code is determined by the last command (sudo bash) which can exit 0 when reading empty stdin. Withoutpipefail, the step passes even though Flux was not installed.Apply a safe install pattern that downloads the script first and only executes it if the download succeeds.
Proposed fix
- command -v flux >/dev/null \ - || curl -fsSL https://fluxcd.io/install.sh | sudo bash + if ! command -v flux >/dev/null; then + install_script="$(mktemp)" + curl -fsSL https://fluxcd.io/install.sh -o "$install_script" + sudo bash "$install_script" + rm -f "$install_script" + fiLine references:
.github/workflows/pull-requests.yaml: 117–120.github/workflows/pull-requests.yaml: 271–274🤖 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 @.github/workflows/pull-requests.yaml around lines 117 - 120, The Flux install step is masking download failures because the curl-to-sudo bash pipeline can succeed even when the download fails; update the workflow’s build toolchain install in the relevant setup step to download the install script first and only run it after a successful download. Use the existing `command -v flux` check as the entry point, then replace the direct pipe in that step with a safe two-step download-and-execute pattern so the `pull-requests.yaml` install block fails immediately if fetching the script fails.
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
435-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the regular PR artifact source comment.
E2E downloads the Talos disk from
build-talosandpr.patchfromfinalize, not directly from the per-packagebuildjobs.Proposed wording
- # ▸ Regular PR path – download artefacts produced by the *build* jobs + # ▸ Regular PR path – download the Talos disk from build-talos and pr.patch from finalize🤖 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 @.github/workflows/pull-requests.yaml at line 435, Update the artifact source comment in the regular PR path to match the actual download flow used by the E2E job. The current note near the regular PR artifact handling in pull-requests.yaml incorrectly says artifacts come from the per-package build jobs; revise it to reflect that the Talos disk is downloaded from build-talos and pr.patch comes from finalize, and keep the wording aligned with the surrounding artifact download logic.
🤖 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 @.github/workflows/pull-requests.yaml:
- Around line 235-239: The Talos image upload step in the pull-request workflow
does not fail when the artifact path is missing, which can hide build/output
drift until a later job. Update the Upload Talos image step that uses
actions/upload-artifact to set the missing-file behavior to error so the
build-talos job fails immediately if _out/assets/nocloud-amd64.raw.xz is not
present.
- Around line 349-355: The GitHub App token generated in the
`actions/create-github-app-token` steps is too broad because it only sets
`owner`, which can expose access to all repos in the organization and default
app permissions. Update both `Generate GitHub App token` steps to include the
current repository via the `repositories` input and explicitly scope the token
to read-only content permissions. Keep the fix localized to the `app-token`
step(s) in this workflow so the token is restricted to the intended repo.
---
Outside diff comments:
In @.github/workflows/pull-requests.yaml:
- Around line 117-120: The Flux install step is masking download failures
because the curl-to-sudo bash pipeline can succeed even when the download fails;
update the workflow’s build toolchain install in the relevant setup step to
download the install script first and only run it after a successful download.
Use the existing `command -v flux` check as the entry point, then replace the
direct pipe in that step with a safe two-step download-and-execute pattern so
the `pull-requests.yaml` install block fails immediately if fetching the script
fails.
In `@docs/agents/e2e-testing.md`:
- Line 70: The wording in the e2e testing docs overstates what TIA narrows;
update the explanation around the app loop and the build-matrix behavior so it
clearly says only the package/test matrix is trimmed, not the full workflow. In
the section describing `Prepare environment`, `Install Cozystack`, and the
`hack/build-matrix.sh`-driven image selection, remove the claim that an app-only
PR “rebuilds only that image” and instead note that Talos still builds for
non-docs PRs and the installer still builds in `finalize`.
---
Nitpick comments:
In @.github/workflows/pull-requests.yaml:
- Line 435: Update the artifact source comment in the regular PR path to match
the actual download flow used by the E2E job. The current note near the regular
PR artifact handling in pull-requests.yaml incorrectly says artifacts come from
the per-package build jobs; revise it to reflect that the Talos disk is
downloaded from build-talos and pr.patch comes from finalize, and keep the
wording aligned with the surrounding artifact download logic.
🪄 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: 4ea8ddfe-4137-44c0-8240-ad3c50ce643c
📒 Files selected for processing (2)
.github/workflows/pull-requests.yamldocs/agents/e2e-testing.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/agents/e2e-testing.md (1)
70-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify that only the package matrix is narrowed.
The workflow still always builds Talos for non-docs PRs and builds the installer in
finalize, so “rebuilds only that image” overstates the new behavior.Proposed wording
-- **It trims only the per-app test loop, not the expensive stages.** `Prepare environment` and `Install Cozystack` (the full platform install) run regardless of the selection. TIA only decides which `make test-apps-<app>` targets run in the final step. A perfect narrow saves the matrix tail, not the bulk of wall-time. (Image *builds* are scoped separately — by `hack/build-matrix.sh`, on the PR diff — so an app-only PR rebuilds only that image; that is a distinct mechanism from TIA's app-loop trimming.) +- **It trims only the per-app test loop, not the expensive stages.** `Prepare environment` and `Install Cozystack` (the full platform install) run regardless of the selection. TIA only decides which `make test-apps-<app>` targets run in the final step. A perfect narrow saves the matrix tail, not the bulk of wall-time. (Package image builds are scoped separately — by `hack/build-matrix.sh`, on the PR diff — while the Talos and installer/finalize legs still run for non-docs PRs; that is a distinct mechanism from TIA's app-loop trimming.)🤖 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 `@docs/agents/e2e-testing.md` at line 70, The wording in the e2e testing docs overstates what TIA narrows; update the explanation around the app loop and the build-matrix behavior so it clearly says only the package/test matrix is trimmed, not the full workflow. In the section describing `Prepare environment`, `Install Cozystack`, and the `hack/build-matrix.sh`-driven image selection, remove the claim that an app-only PR “rebuilds only that image” and instead note that Talos still builds for non-docs PRs and the installer still builds in `finalize`..github/workflows/pull-requests.yaml (1)
117-120: 🩺 Stability & Availability | 🟡 MinorAvoid masking failed Flux downloads.
On GitHub Actions,
set -o pipefailis not enabled by default. In these installs,curl -ffailing (e.g., due to 404 or network error) exits with a non‑zero code, but the failure is masked because the pipeline's exit code is determined by the last command (sudo bash) which can exit 0 when reading empty stdin. Withoutpipefail, the step passes even though Flux was not installed.Apply a safe install pattern that downloads the script first and only executes it if the download succeeds.
Proposed fix
- command -v flux >/dev/null \ - || curl -fsSL https://fluxcd.io/install.sh | sudo bash + if ! command -v flux >/dev/null; then + install_script="$(mktemp)" + curl -fsSL https://fluxcd.io/install.sh -o "$install_script" + sudo bash "$install_script" + rm -f "$install_script" + fiLine references:
.github/workflows/pull-requests.yaml: 117–120.github/workflows/pull-requests.yaml: 271–274🤖 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 @.github/workflows/pull-requests.yaml around lines 117 - 120, The Flux install step is masking download failures because the curl-to-sudo bash pipeline can succeed even when the download fails; update the workflow’s build toolchain install in the relevant setup step to download the install script first and only run it after a successful download. Use the existing `command -v flux` check as the entry point, then replace the direct pipe in that step with a safe two-step download-and-execute pattern so the `pull-requests.yaml` install block fails immediately if fetching the script fails.
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
435-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the regular PR artifact source comment.
E2E downloads the Talos disk from
build-talosandpr.patchfromfinalize, not directly from the per-packagebuildjobs.Proposed wording
- # ▸ Regular PR path – download artefacts produced by the *build* jobs + # ▸ Regular PR path – download the Talos disk from build-talos and pr.patch from finalize🤖 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 @.github/workflows/pull-requests.yaml at line 435, Update the artifact source comment in the regular PR path to match the actual download flow used by the E2E job. The current note near the regular PR artifact handling in pull-requests.yaml incorrectly says artifacts come from the per-package build jobs; revise it to reflect that the Talos disk is downloaded from build-talos and pr.patch comes from finalize, and keep the wording aligned with the surrounding artifact download logic.
🤖 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 @.github/workflows/pull-requests.yaml:
- Around line 235-239: The Talos image upload step in the pull-request workflow
does not fail when the artifact path is missing, which can hide build/output
drift until a later job. Update the Upload Talos image step that uses
actions/upload-artifact to set the missing-file behavior to error so the
build-talos job fails immediately if _out/assets/nocloud-amd64.raw.xz is not
present.
- Around line 349-355: The GitHub App token generated in the
`actions/create-github-app-token` steps is too broad because it only sets
`owner`, which can expose access to all repos in the organization and default
app permissions. Update both `Generate GitHub App token` steps to include the
current repository via the `repositories` input and explicitly scope the token
to read-only content permissions. Keep the fix localized to the `app-token`
step(s) in this workflow so the token is restricted to the intended repo.
---
Outside diff comments:
In @.github/workflows/pull-requests.yaml:
- Around line 117-120: The Flux install step is masking download failures
because the curl-to-sudo bash pipeline can succeed even when the download fails;
update the workflow’s build toolchain install in the relevant setup step to
download the install script first and only run it after a successful download.
Use the existing `command -v flux` check as the entry point, then replace the
direct pipe in that step with a safe two-step download-and-execute pattern so
the `pull-requests.yaml` install block fails immediately if fetching the script
fails.
In `@docs/agents/e2e-testing.md`:
- Line 70: The wording in the e2e testing docs overstates what TIA narrows;
update the explanation around the app loop and the build-matrix behavior so it
clearly says only the package/test matrix is trimmed, not the full workflow. In
the section describing `Prepare environment`, `Install Cozystack`, and the
`hack/build-matrix.sh`-driven image selection, remove the claim that an app-only
PR “rebuilds only that image” and instead note that Talos still builds for
non-docs PRs and the installer still builds in `finalize`.
---
Nitpick comments:
In @.github/workflows/pull-requests.yaml:
- Line 435: Update the artifact source comment in the regular PR path to match
the actual download flow used by the E2E job. The current note near the regular
PR artifact handling in pull-requests.yaml incorrectly says artifacts come from
the per-package build jobs; revise it to reflect that the Talos disk is
downloaded from build-talos and pr.patch comes from finalize, and keep the
wording aligned with the surrounding artifact download logic.
🪄 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: 4ea8ddfe-4137-44c0-8240-ad3c50ce643c
📒 Files selected for processing (2)
.github/workflows/pull-requests.yamldocs/agents/e2e-testing.md
🛑 Comments failed to post (2)
.github/workflows/pull-requests.yaml (2)
235-239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail immediately when the Talos disk artifact is missing.
The downstream E2E job requires
talos-image; withoutif-no-files-found: error, a path/output drift can surface later as a confusing download failure instead of failing inbuild-talos.Proposed fix
with: name: talos-image path: _out/assets/nocloud-amd64.raw.xz + if-no-files-found: error📝 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.- name: Upload Talos image uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: talos-image path: _out/assets/nocloud-amd64.raw.xz if-no-files-found: error🤖 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 @.github/workflows/pull-requests.yaml around lines 235 - 239, The Talos image upload step in the pull-request workflow does not fail when the artifact path is missing, which can hide build/output drift until a later job. Update the Upload Talos image step that uses actions/upload-artifact to set the missing-file behavior to error so the build-talos job fails immediately if _out/assets/nocloud-amd64.raw.xz is not present.
349-355: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🌐 Web query:
actions/create-github-app-token v1 repositories permission-contents inputs💡 Result:
The actions/create-github-app-token action allows you to manage token permissions and repository access through specific input parameters [1][2]. Repository Access The repositories input is an optional parameter that accepts a comma-separated or newline-separated list of repository names to which the generated token should have access [3][2][4]. If both owner and repositories inputs are left empty, the token is scoped to the current repository [3][2][4]. If owner is provided but repositories is empty, the token is scoped to all repositories in that owner's installation [3][2][4]. Permission Management To control the permissions granted to the generated token, you can use inputs prefixed with permission- [1][2]. For example, the permission-contents input allows you to specify the access level for repository contents, commits, branches, releases, and merges, accepting values of read or write [5][6][7]. By default, the token inherits all permissions assigned to the GitHub App installation [3][2]. It is recommended to explicitly define only the required permissions using these inputs to follow the principle of least privilege [1][2]. Any available permission can be requested by prefixing its name with permission- (e.g., permission-pull-requests for pull-requests) [1][2]. Note: While older versions (like v1.9.2) existed, current documentation and best practices emphasize using explicit permission inputs [1][5][2]. Ensure that the GitHub App installation itself has the necessary permissions granted by an administrator, as the action cannot grant permissions beyond what is authorized for the installation [1][2].
Citations:
- 1: https://github.com/actions/create-github-app-token
- 2: https://github.com/actions/create-github-app-token?tab=readme-ov-file
- 3: https://github.com/actions/create-github-app-token/tree/v1.9.2
- 4: https://github.com/actions/create-github-app-token/blob/34c66235f6ccc38eb01b7aed860890bdf40db33c/README.md
- 5: actions/create-github-app-token@0e0aa99
- 6: https://github.com/actions/create-github-app-token/blob/df432cee/action.yml
- 7: https://github.com/actions/create-github-app-token/blob/93c1f04d6f14ea5b416e7a8dfd80446101c6adef/action.yml
Scope GitHub App tokens to the current repository with read-only content permissions.
Both
create-github-app-tokensteps (lines 349–355 and 418–425) currently omit therepositoriesinput. Combined with theownerinput, this grants the token access to all repositories in thecozystackorganization and inherits the App's default permissions.Update both steps to restrict access to the current repository and explicitly define read-only permissions:
Proposed fix
with: app-id: ${{ secrets.COZYSTACK_CI_APP_ID }} private-key: ${{ secrets.COZYSTACK_CI_PRIVATE_KEY }} owner: cozystack + repositories: ${{ github.event.repository.name }} + permission-contents: read📝 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.- name: Generate GitHub App token id: app-token uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 with: app-id: ${{ secrets.COZYSTACK_CI_APP_ID }} private-key: ${{ secrets.COZYSTACK_CI_PRIVATE_KEY }} owner: cozystack repositories: ${{ github.event.repository.name }} permission-contents: read🧰 Tools
🪛 zizmor (1.26.1)
[error] 355-355: dangerous use of GitHub App tokens (github-app): token granted access to all repositories for this owner's app installation
(github-app)
[error] 351-351: dangerous use of GitHub App tokens (github-app): app token inherits blanket installation permissions
(github-app)
🤖 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 @.github/workflows/pull-requests.yaml around lines 349 - 355, The GitHub App token generated in the `actions/create-github-app-token` steps is too broad because it only sets `owner`, which can expose access to all repos in the organization and default app permissions. Update both `Generate GitHub App token` steps to include the current repository via the `repositories` input and explicitly scope the token to read-only content permissions. Keep the fix localized to the `app-token` step(s) in this workflow so the token is restricted to the intended repo.Source: Linters/SAST tools
- scope create-github-app-token to the current repo with read-only contents permission (was org-wide, inheriting the app's blanket installation permissions); both token steps only read releases/assets - fail-fast the Talos image upload when the disk is missing (if-no-files-found: error) so drift fails in build-talos, not later as a confusing e2e download error - download the flux install script to a tempfile before executing it, so a failed curl is not masked by the piped `sudo bash` exiting 0 - fix the regular-PR artifact-source comment (Talos disk comes from build-talos, pr.patch from finalize) and the TIA scope wording in docs/agents/e2e-testing.md 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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pull-requests.yaml (1)
443-447: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid persisting the workflow token into the copied E2E workspace.
actions/checkoutpersists${{ github.token }}by default. This job only needs local history, then copies the checkout into/tmp/$SANDBOX_NAMEand uploads diagnostics, so disable persisted credentials here.🔒 Proposed fix
- name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/pull-requests.yaml around lines 443 - 447, The Checkout code step in the pull-requests workflow is persisting the default workflow token into the workspace that gets copied to /tmp/$SANDBOX_NAME. Update the actions/checkout usage in that job to disable persisted credentials while keeping fetch-depth: 0 for local history, so the copied E2E workspace and uploaded diagnostics do not contain github.token.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In @.github/workflows/pull-requests.yaml:
- Around line 443-447: The Checkout code step in the pull-requests workflow is
persisting the default workflow token into the workspace that gets copied to
/tmp/$SANDBOX_NAME. Update the actions/checkout usage in that job to disable
persisted credentials while keeping fetch-depth: 0 for local history, so the
copied E2E workspace and uploaded diagnostics do not contain github.token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd190d27-29c9-4b17-b189-7fe1111fc8a4
📒 Files selected for processing (2)
.github/workflows/pull-requests.yamldocs/agents/e2e-testing.md
✅ Files skipped from review due to trivial changes (1)
- docs/agents/e2e-testing.md
Resolve conflict in .github/workflows/pull-requests.yaml: keep the new per-package build matrix and fold in main's docker.io->mirror.gcr.io base-image routing (#3042). Apply buildkitd-config: hack/buildkitd.toml to all three new build legs (build, build-talos, finalize) so the mirror fix covers the whole decomposed pipeline, not just the conflicted leg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the blocker from the prior review is resolved, and the non-blocking items are addressed.
Re-reviewed at the current head.
Blocker (root Go source changes rebuilt no images, staling e2e): resolved. hack/build-matrix.sh now includes api/, cmd/, internal/, and pkg/ in full_rebuild_pattern, so a change under any of those forces the full matrix; the plan -> any=true -> build gate then runs the rebuild before finalize and e2e. The selector suite pins this with new cases (a root Go change under api/ and under pkg/ each force the full matrix), and make unit-tests runs the bats selector tests in CI.
Non-blocking, now addressed: the selector bats suite runs in CI (the checks job runs make unit-tests); finalize applies digest fragments with git apply --3way plus the one-package-per-fragment invariant comment; the base merge conflict is resolved.
One non-blocking item remains (fine to defer): the flux install now uses curl --fail to a temp file instead of piping straight to sudo bash, but it is still unpinned and unverified by checksum — worth pinning a version + checksum until flux is baked into the runner image.
On CI: the only red was E2E Tests on the first attempt, which failed in the install/bootstrap phase (linstor HelmRelease not Ready within the timeout, snapshot-controller crashlooping, cozystack-platform PackageSources stuck InProgress) — components this change does not touch (the diff is 7 CI/build-tooling files). All image builds and the finalize step were green, so the assembled tree was complete; this matches the known first-attempt bootstrap timeout and the job was retried. If the retry reproduces the same install timeout, that points to a platform-install issue independent of this build-pipeline change and worth investigating separately. Recommend letting the retried E2E finish green before merge.
Retracting this approval as premature — for a build-pipeline change the retried E2E run is the end-to-end proof that the matrix rebuild actually works, and it has not gone green yet (attempt 1 timed out during install; attempt 2 is still in progress). The code blocker is resolved and unit-pinned, so once E2E completes green I will re-approve; if it reproduces the install timeout that is a separate platform issue to investigate.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the prior blocker is resolved and unit-pinned; approving at the code level.
The blocker (root Go source changes rebuilt no images, staling e2e) is fixed: hack/build-matrix.sh now includes api/, cmd/, internal/, pkg/ in full_rebuild_pattern, and the plan -> any=true -> build gate runs the rebuild before finalize/e2e. The selector suite pins this with new cases for a root Go change under api/ and pkg/, and make unit-tests runs the bats selectors in CI (green). The non-blocking items are addressed; the remaining flux-install pin/checksum is fine to defer.
This approval is code-level. The retried E2E run is the end-to-end proof and the merge gate (branch protection) — not this review — will hold the merge until it is green; if new commits land to fix CI, this approval will be dismissed and re-requested automatically. On the attempt-1 failure: it was an install/bootstrap timeout (linstor not Ready, snapshot-controller crashlooping, platform PackageSources stuck InProgress) in components this 7-file CI/build-tooling change does not touch, so it reads as the known first-attempt bootstrap flake rather than a regression here. If the retry reproduces the identical install timeout, that is a platform issue to investigate independently of this PR.
…-2937 Resolve the .github/workflows/pull-requests.yaml conflict: keep this branch's per-package build matrix and carry main's `git diff --binary` patch fix (#e566992) onto every fragment and pr.patch capture, so provider bundles that package image targets regenerate (components*.gz) survive fragment reassembly in finalize and reach the patch e2e applies. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
0b9f5a1
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — re-affirming after the branch was brought up to date with main.
My prior approval was dismissed when two origin/main merge commits landed on the branch. The only change to this PR's own build logic since then is in .github/workflows/pull-requests.yaml: the per-package fragment, the Talos fragment, and the final pr.patch assembly now use git diff --binary HEAD instead of git diff HEAD. This is correct and necessary — main now vendors binary provider bundles (components*.gz), and a plain git diff emits an unappliable "Binary files differ" stub, so without --binary those regenerated bundles would be dropped from the fragment and finalize reassembly would diverge from a real serial build. It mirrors the same binary-diff fix already on main for the monolithic pr.patch.
The matrix selector (hack/build-matrix.sh) and its unit pins (hack/build-matrix_test.bats) are unchanged from the approved revision: full_rebuild_pattern still forces the full matrix on api/, cmd/, internal/, pkg/ (plus go.mod/go.sum/Makefile/the workflows). The original blocker — root Go edits rebuilding no images and staling e2e — stays resolved.
CI is green including the Finalize/merge-patches job that exercises the binary-patch reassembly end-to-end; E2E is the separate merge gate. No blockers.
## What this PR does Implements the immutable-tag + rc-promotion flow from #2677. A **stable release becomes a renamed release-candidate**: the bytes shipped as `vX.Y.Z` are bit-for-bit the bytes built and e2e-tested as `vX.Y.Z-rc.N`. No tag is ever force-moved, and stable is never rebuilt — it is *promoted* by retagging the rc's existing images. Layered onto the build matrix from #2937/#2983 (this PR is **stacked on `refactor/build-matrix-2937`** and must merge after it). ## The five force-retag sites, removed | Site | Before | After | |---|---|---| | `tags.yaml` api/apps/v1alpha1 tag | `git tag -f` / `push -f` | write-once (create-if-absent, fail if it would move) | | `tags.yaml` release-X.Y.Z branch | `git branch -f` / `push -f` | compare-before-force (no-op if unchanged; staging branch only) | | `pull-requests-release.yaml` stable tag | `git tag -f` / `push -f` | write-once at the PR merge commit (force impossible by construction) | | `pull-requests-release.yaml` maintenance branch | `updateRef force:true` | fast-forward-only | | `auto-release.yaml` patch tags | delete-recreate (cron) | **workflow deleted** — stable only via explicit promote | ## Version decoupling (the enabler) The operator baked its version into the image at build time, so an rc image self-reported the rc string — blocking retag-promotion. It now reads `COZYSTACK_VERSION` from the environment (threaded via `cozystackOperator.platformVersion` → Deployment env, stamped by `make manifests`), falling back to the build-time value when unset. The same image bits can report any release name. The only runtime reader is the telemetry metric `cozy_cluster_info{cozystack_version=...}`. ## Promotion flow `promote-rc.yaml` (`workflow_dispatch`, `rc_tag=vX.Y.Z-rc.N`): 1. Validate the rc release exists and the stable tag does not. 2. `hack/promote-retag.sh` reads the rc's digest-pinned image refs from `packages/*/*/values.yaml` and `skopeo copy`s each — **by digest** — to `:vX.Y.Z` and `:latest`, verifying with `skopeo inspect`. 3. Rewrite the cosmetic `-rc.N` substring in vendored tags to the stable version (the `@sha256` wins regardless), restamp the version-stamped assets (operator manifests, cozypkg, openapi; the heavy Talos assets are copied verbatim from the rc draft), open a `release-X.Y.Z` PR. 4. Merging the PR reuses the existing release-PR e2e and `pull-requests-release.yaml` finalize to cut the **write-once** stable tag at the merge commit and publish the release. Squash is disallowed (the tag needs a real merge commit). `nightly.yaml` cuts write-once `*-nightly.<date>` tags (gated by `NIGHTLY_ENABLED`); `retention.yaml` keeps the newest 14 per line (dry-run by default). ## Validation **Local end-to-end — the core claim is proven, not asserted.** The whole design rests on "copy-by-digest to a new tag preserves the digest, so stable == the e2e-tested rc, bit-for-bit." This was exercised against a real registry (`ttl.sh`) with real `skopeo`: - Pushed a multi-arch image under a `:v1.4.0-rc.2` tag, built a temp tree exercising all three `values.yaml` digest shapes (single `image:` string, split `repository`/`tag`/`digest` map, and the `platformSourceRef` OCI artifact), then ran the actual `hack/promote-retag.sh v1.4.0`. - **Result: `:v1.4.0-rc.2`, `:v1.4.0`, and `:latest` all resolve to the identical digest** (`sha256:fd8d9aa6…`), across all 17 platform manifests. The script's own `skopeo inspect` post-check passed, and an independent re-inspection confirmed it. Local validation caught (and this PR fixes) two bugs that only surface against a real registry — never in static lint: 1. `skopeo copy --multi-arch all --all` — mutually exclusive flags, fatal error; every retag would have failed on CI. Now `--multi-arch all`. 2. The same digest was retagged twice when an image appeared in two value shapes; the ref set is now deduped on the canonical `repo@digest`. **Other local checks:** `go test ./pkg/version` + `go build`/`vet`; `helm template` renders `COZYSTACK_VERSION` in all 3 operator variants (omitted when unset); `make manifests` stamps the version into the install assets; `shellcheck` clean on `promote-retag.sh`; `actionlint` clean and `act -l` resolves the job graph on all workflows; the rc-tag parser and nightly version-math unit-tested (accept/reject + minor-vs-patch bumps). **Still CI-only (cannot be exercised offline):** the full workflow runtime — github-script API calls, OCIR auth, the rc-release / staging-branch preconditions, the nightly `base_ref` push mechanics, and self-hosted runners. The *logic* inside the steps is unit-tested; the *orchestration* is not. Treat it as unproven until the workflows run. ## Out of scope / required follow-up (needs repo admin — not doable from a PR) - Tag-protection rules: `v*` and `api/apps/v1alpha1/*` create-only by the CI app, no delete/update; "limit branches/tags updated in a single push" = 1. - Enable `NIGHTLY_ENABLED` (and `RETENTION_APPLY` when ready) repo variables. - Registry-side pruning of `*-nightly.*` image tags (no OCIR delete parity yet — tracked TODO in `retention.yaml`). ## Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added nightly publishing (mirror-by-digest, disk build, e2e validation) and nightly retention pruning. * Added RC-to-stable promotion via digest retagging (no rebuild). * Operator/installer and console now expose version/platform metadata, including runtime override via `COZYSTACK_VERSION`. * **Bug Fixes** * Enforced write-once tag behavior and fast-forward-only maintenance updates across release workflows. * Made nightly mirroring/selection and retention pruning more selective and safer. * **Tests** * Added Bats coverage for nightly mirroring and RC retagging, plus new Helm/unit checks. * **Documentation** * Updated the release model to center RC promotion and tag immutability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Completes #2937. Replaces the single serial
Buildjob — one ephemeral VM runningmake build(~26 images one after another) that wedged on the shared embedded buildkit under concurrent multi-PR load — with a parallel, diff-scoped matrix.New job graph (regular PR path):
Why finalize is special:
packages/core/installer'simagetarget runsflux push artifact --path=packages, bundling the entire packages tree into the OCI artifact the operator pulls — so it must run after every other package's digest edit is applied, never as a peer matrix job.talosis likewise pulled out of the matrix (its image and nocloud disk share heavy_out/assetsand build once).Gating:
finalize(and thereforee2e, which needs it) depends onchecks, so a red unit/controller test skips the expensive install+e2e path instead of running it for ~1h — the gating the old in-build-job tests gave, without re-coupling the cheap parallel builds.Change-scoping:
hack/build-matrix.shparses the root Makefilebuild:target (single source of truth) and emits only the packages a PR touches — most PRs build ~0–1 images — escalating to the full set on a shared-dependency change (cozy-lib, the build macros,go.mod, the build workflows). It fansseaweedfsout toobjectstorage-controller(cross-package sidecar-tag dependency). 16 bats cases cover the selector.SBOM:
SBOM=1wires buildx--sbomthroughcommon-envs.mk(off by default). It stays off in the matrix until an OCIR referrers/attestation probe confirms the registry accepts attestations.Decommission: the cache warmer (
build-main.yaml) moves to the ephemeral pool, so no build path depends on the dedicated self-hosted runner. The runner remains referenced only by the e2edebugbreakpoint path; fully de-registering it is a separate infra step.Validation
hack/build-matrix_test.bats16/16;actionlintclean (custom runner labels registered in.github/actionlint.yaml);make -nconfirms--sbomis off by default and on withSBOM=1.pull-requests.yaml, sobuild-matrix.shforces a full matrix — the PR exercises the new pipeline on itself. First run was green throughplan → 25 parallel builds → build-talos → finalize → install → 21/22 app tests; the only red was the pre-existingbucket/BucketAccess flake (install and the other 21 suites passed on the matrix-built tree), which validates the decomposition end-to-end.Follow-ups (tracked on #2937)
SBOM=1in the matrix if supported.debugbreakpoint path is its only remaining user).Release note
Summary by CodeRabbit