ci: reuse release image for edge - #63
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe image workflow now serializes publishing, skips marked development commits, and assigns ChangesImage release workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant GitHubActions
participant ImageRegistry
ReleaseWorkflow->>GitHubActions: Create development commit with skip trailer
GitHubActions->>GitHubActions: Skip marked master image build
GitHubActions->>ImageRegistry: Publish release version, latest, and edge tags
Possibly related PRs
🚥 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 |
|
/agentic_review |
Code Review by Qodo
Context used✅ Compliance rules (platform):
31 rules 1.
|
|
/agentic_review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
PR Summary by QodoCI: reuse release-built image for edge and serialize tag/branch publishes
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
| group: weather-briefing-image | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
1. Edge rollback via reruns 🐞 Bug ≡ Correctness
With a single global concurrency group and cancel-in-progress: false, the mutable :edge tag is updated by whichever workflow run finishes last, so a manually rerun (or otherwise delayed) run for an older ref can execute after newer builds and overwrite :edge with an older digest, effectively rolling edge backwards even though newer immutable tags (sha/version) remain correct. The current notes describe serialization as avoiding concurrent edge updates but do not warn about this last-writer/rerun rollback behavior, which can cause operators to misinterpret the guarantees of the concurrency policy.
Agent Prompt
## Issue description
The image publishing workflow serializes all runs into one concurrency group and does not cancel in-progress runs, so the mutable `:edge` tag follows last-writer semantics: whichever run completes last updates `edge`. This allows manual reruns (or other delayed executions) of older tag/master refs to execute after newer builds and overwrite `:edge` with an older digest, effectively rolling `edge` backward; the documentation also needs to clearly state this limitation so operators don’t assume serialization implies “newest ref always wins.”
## Issue Context
- The workflow triggers on both `master` pushes and release tag pushes.
- The manifest-publish step tags `:edge` for both tag builds and branch builds.
- With a global concurrency group and `cancel-in-progress: false`, `edge` is determined by the last completed publisher across all executions, including manual reruns (e.g., for diagnostics).
- `docs/notes.md` currently explains the serialization boundary (avoiding concurrent writes) but does not warn that reruns/delayed runs of older refs can still overwrite `edge` after newer runs.
## Fix Focus Areas
- .github/workflows/image.yml[17-19]
- .github/workflows/image.yml[101-134]
- docs/notes.md[9-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 84073f5 |
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 @.github/workflows/image.yml:
- Around line 18-28: Replace the static concurrency group in the
weather-briefing workflow at .github/workflows/image.yml:18-28 with a
ref-dependent group such as github.workflow plus github.ref, preserving the
existing cancellation behavior and trailer-based job skip. Remove the
single-group concurrency explanation from docs/notes.md:7-25 and describe the
master edge-build protection solely through the Weather-Briefing-Skip-Edge-Image
commit trailer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 424bcfbb-1bc9-4157-896d-fd4cdf28146e
📒 Files selected for processing (5)
.github/workflows/image.yml.github/workflows/release.ymldocs/design.mddocs/notes.mddocs/requirements.md
| group: weather-briefing-image | ||
| cancel-in-progress: false | ||
|
|
||
| env: | ||
| WEATHER_BRIEFING_IMAGE: ${{ secrets.DOCKERHUB_USERNAME }}/weather-briefing | ||
|
|
||
| jobs: | ||
| weather-briefing: | ||
| if: >- | ||
| github.ref_type == 'tag' || | ||
| !contains(github.event.head_commit.message, 'Weather-Briefing-Skip-Edge-Image: true') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Race condition risking cancelled release builds due to static concurrency group.
GitHub Actions cancels previously pending workflow runs in the same concurrency group when multiple events are queued simultaneously. An atomic push triggers both the tag and master events at the exact same time. If the master event is processed second and queued, it will silently cancel the pending tag event. Since the generated master commit is explicitly designed to skip the build (via the Weather-Briefing-Skip-Edge-Image trailer), this cancellation race will result in the release image not being built at all.
The explicit commit trailer already cleanly prevents the edge tag race by skipping the master build entirely, making the shared concurrency group unnecessary and actively dangerous for atomic pushes.
.github/workflows/image.yml#L18-L28: Revert the workflow to use a ref-dependent concurrency group (e.g.,group: ${{ github.workflow }}-${{ github.ref }}) to isolate the tag queue from the master queue, ensuring the release tag build is never cancelled by the skipped master build.docs/notes.md#L7-L25: Remove the explanation of the singleweather-briefing-imageconcurrency group. Update the text to rely purely on the commit trailer mechanism to explain how the masteredgebuild conflict is safely bypassed.
📍 Affects 2 files
.github/workflows/image.yml#L18-L28(this comment)docs/notes.md#L7-L25
🤖 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/image.yml around lines 18 - 28, Replace the static
concurrency group in the weather-briefing workflow at
.github/workflows/image.yml:18-28 with a ref-dependent group such as
github.workflow plus github.ref, preserving the existing cancellation behavior
and trailer-based job skip. Remove the single-group concurrency explanation from
docs/notes.md:7-25 and describe the master edge-build protection solely through
the Weather-Briefing-Skip-Edge-Image commit trailer.
This PR updates the release flow so edge uses the freshly built image from the release tag commit and avoids a separate build trigger path. Also keeps release versioning behavior unchanged from previous iteration.
Summary by CodeRabbit
New Features
latest, andedgetags together.edgeand the commit-specific image tag.Bug Fixes
edgetag.Documentation