ci(docker): split all-in-one into post-matrix job, exclude shibboleth from build_all#14252
Conversation
…hibboleth from build_all - Add fail-fast: false to prevent one image failure cancelling others - Remove all-in-one from matrix; add docker-aio job with needs: [docker] so it only starts after all other images complete (replacing the bogus sleep-loop) - Remove docker-jans-shibboleth and docker-jans-all-in-one from ALL_SERVICES so neither runs when build_all is checked; shibboleth still buildable via its individual workflow_dispatch input - Increase docker-aio timeout to 90m to accommodate the post-wait build Signed-off-by: moauto <54212639+mo-auto@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workflow disables matrix fail-fast, removes ChangesDocker Image Build Workflow Adjustments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…m build_all - Add fail-fast: false so one image failure does not cancel others - Replace sleep-loop with gh api polling in the Prepare step: all-in-one waits for auth-server, config-api, configurator, fido2, persistence-loader, scim, and casa to reach a terminal state before building (no code duplication) - Remove docker-jans-shibboleth from ALL_SERVICES so it is skipped on build_all; still buildable via its individual workflow_dispatch input Signed-off-by: moauto <54212639+mo-auto@users.noreply.github.com>
60b6175 to
75d00dd
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-docker-image.yml (1)
111-115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
all-in-oneis still wired as a matrix child, not a post-matrix job.Line 139 removes it from the service list that drives
workflow_dispatch build_all, nightly schedule, and the initial-push fallback, so those paths now skip AIO entirely. But because it still lives in thedockermatrix, any changed-dir/tag path that does build it still runs under the matrix's 60-minute timeout and consumes a parallel slot instead of becoming the 90-minuteneeds: [docker]post-job described in this PR.Also applies to: 139-140
🤖 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-docker-image.yml around lines 111 - 115, The matrix still includes "all-in-one" in the docker-images list (matrix: docker-images: [... "all-in-one" ...]) which keeps it running under the matrix’s 60-minute timeout and consumes a parallel slot; remove "all-in-one" from that docker matrix and instead implement it as the separate post-matrix job described in the PR: create a new job named e.g. docker_all_in_one that has needs: [docker], timeout-minutes: 90, and is wired into the same service list/dispatch/nightly/fallback paths (the same places that drive workflow_dispatch build_all) so AIO runs as the 90-minute post-job rather than as a matrix child.
🤖 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-docker-image.yml:
- Around line 277-289: The polling loop for "docker ($img)" using CONCLUSION
inside for attempt in $(seq 1 60) currently falls through if no terminal state
is observed, allowing the AIO build to proceed; update the logic so that after
the loop finishes (i.e., timed out without hitting success/skipped/failure), you
treat that as a failure and abort: after the for-loop, check if CONCLUSION is
not "success" or "skipped" and if so echo a clear error message including the
job name ("docker ($img)") and exit 1 — keep existing early exits for explicit
failure states and use the same CONCLUSION variable and job name string to
locate where to add this post-loop timeout handling.
---
Outside diff comments:
In @.github/workflows/build-docker-image.yml:
- Around line 111-115: The matrix still includes "all-in-one" in the
docker-images list (matrix: docker-images: [... "all-in-one" ...]) which keeps
it running under the matrix’s 60-minute timeout and consumes a parallel slot;
remove "all-in-one" from that docker matrix and instead implement it as the
separate post-matrix job described in the PR: create a new job named e.g.
docker_all_in_one that has needs: [docker], timeout-minutes: 90, and is wired
into the same service list/dispatch/nightly/fallback paths (the same places that
drive workflow_dispatch build_all) so AIO runs as the 90-minute post-job rather
than as a matrix child.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c417f49e-5c51-4acf-854e-00b353877210
📒 Files selected for processing (1)
.github/workflows/build-docker-image.yml
After the poll loop exhausts all 60 attempts without seeing success/skipped, exit 1 instead of falling through and starting the AIO build anyway. Signed-off-by: moauto <54212639+mo-auto@users.noreply.github.com>
Summary
fail-fast: falseadded to the matrix strategy — previously one failing image (e.g. shibboleth) cancelled all other buildsall-in-onesplit into a separatedocker-aiojob withneeds: [docker]— it now waits for all matrix jobs to fully complete before starting, replacing the bogussleep 30 × 12loop that was only a ~6-minute fixed delaybuild_all— removed fromALL_SERVICES; it still exists in the matrix and can be triggered via its individualworkflow_dispatchinputdocker-jans-all-in-oneremoved fromALL_SERVICES— AIO is no longer in the matrix so the entry was deaddocker-aiotimeout set to 90m (vs 60m) to accommodate the full post-wait buildAIO actual dependencies (from Dockerfile FROMs)
(shibboleth and link are commented out in the AIO Dockerfile)
Test plan
dockermatrix jobs run in parallel; a shibboleth failure does not cancel othersdocker-aiojob appears in the workflow run after all matrix jobs finishbuild_all=truedoes not trigger shibbolethshibboleth=truestill triggers shibbolethSummary by CodeRabbit