fix(ci): make stable GitHub releases idempotent - #976
Conversation
f60de95 to
b49a179
Compare
Signed-off-by: Swarom Muley <smuley@nvidia.com>
b49a179 to
6ebf91c
Compare
| required: false | ||
| type: boolean | ||
| default: false | ||
| create-github-release: |
There was a problem hiding this comment.
each day, the parameter list grows
There was a problem hiding this comment.
Which is fine if the default is "do what we want most of the time"
There was a problem hiding this comment.
And I now believe this does NOT do that. This is the opposite of that, imo. Commented further below. I could be wrong.
📝 WalkthroughWalkthroughStable non-dry-run releases now create or confirm Git tags and GitHub Releases regardless of release scope. The workflow removes the manual override and adds tag-SHA validation plus existing-release checks. Documentation describes the unified behavior and retry handling. ChangesRelease gating
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yaml (1)
230-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the release-decision matrix.
Cover stable/all, stable/custom with the flag enabled, stable/custom with it disabled, stable wheels/containers/Helm, and nightly releases. This gate controls a write-permission job, so a small pure decision test would prevent accidental tagging of ordinary subset releases.
🤖 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/release.yaml around lines 230 - 236, Add a pure regression test for the createGitHubRelease decision logic, covering stable/all, stable/custom with createGitHubReleaseRequested enabled and disabled, stable wheels/containers/Helm scopes, and nightly releases. Assert that only the intended stable release combinations return true, preventing ordinary subset releases from enabling the write-permission path.
🤖 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 `@RELEASING.md`:
- Line 71: Complete the stable custom release example in the release
documentation by adding concrete values for source-sha, version, release-scope:
custom, and the required artifact IDs alongside the existing stable release-type
and GitHub-release settings. Ensure the example represents a valid dispatch with
a specific, reproducible outcome.
---
Nitpick comments:
In @.github/workflows/release.yaml:
- Around line 230-236: Add a pure regression test for the createGitHubRelease
decision logic, covering stable/all, stable/custom with
createGitHubReleaseRequested enabled and disabled, stable wheels/containers/Helm
scopes, and nightly releases. Assert that only the intended stable release
combinations return true, preventing ordinary subset releases from enabling the
write-permission path.
🪄 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: Enterprise
Run ID: 4ca7ef68-9001-45f4-9a5c-09928617e4a4
📒 Files selected for processing (2)
.github/workflows/release.yamlRELEASING.md
| if: >- | ||
| needs.plan-release.outputs.release_type == 'stable' && | ||
| needs.plan-release.outputs.release_scope == 'all' && | ||
| needs.plan-release.outputs.create_github_release == 'true' && |
There was a problem hiding this comment.
Why do we need this at all? I don't see any reason not to just remove this and not introduce the new parameter.
An improvement would be to also just output what it would have created with ACT="true", but I emphatically reject needing to explicitly ask for release when I said make a stabe release already. :)
There was a problem hiding this comment.
But we can use stable custom runs to retry failed artifacts too, so we need a way to say whether that run should also cut the tag/release.
There was a problem hiding this comment.
Otherwise it'll fail by default if the tag already exists
There was a problem hiding this comment.
What about checking for the tag first and making it idempotent instead? This adds a random clickbox for what it should do by default.
|
Signed-off-by: Swarom Muley <smuley@nvidia.com>
svvarom
changed the title
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/release.yaml (1)
60-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate GitHub Release creation to
release-scope: all. Stable subset/custom releases still rungh release createhere, so they publish a tag and GitHub Release. If those scopes should stay untagged, add the missing scope check back.🤖 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/release.yaml around lines 60 - 69, Update the GitHub Release creation step in the workflow to run only when release-scope is all, while preserving the existing behavior for stable Helm-only and other scoped releases except preventing tag and GitHub Release publication.
🤖 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/release.yaml:
- Around line 1132-1135: Update the release-check logic around RELEASE_TAG and
gh release view to require a non-empty tag before treating an existing release
as complete. Compare the existing tag’s commit SHA (tag_sha) with SOURCE_SHA,
skipping only when they match; otherwise recreate the tag or fail, and preserve
failure when the required tag is missing.
---
Outside diff comments:
In @.github/workflows/release.yaml:
- Around line 60-69: Update the GitHub Release creation step in the workflow to
run only when release-scope is all, while preserving the existing behavior for
stable Helm-only and other scoped releases except preventing tag and GitHub
Release publication.
🪄 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: Enterprise
Run ID: 79ac2030-5f4e-4f68-ae0c-0d94b810b489
📒 Files selected for processing (2)
.github/workflows/release.yamlRELEASING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- RELEASING.md
| if gh release view "${RELEASE_TAG}" >/dev/null 2>&1; then | ||
| echo "GitHub Release ${RELEASE_TAG} already exists; nothing to do." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require the tag before skipping an existing release. If gh release view succeeds while RELEASE_TAG is missing, this exits 0 and leaves the release untagged. Skip only when tag_sha == SOURCE_SHA; otherwise recreate the tag or fail.
🤖 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/release.yaml around lines 1132 - 1135, Update the
release-check logic around RELEASE_TAG and gh release view to require a
non-empty tag before treating an existing release as complete. Compare the
existing tag’s commit SHA (tag_sha) with SOURCE_SHA, skipping only when they
match; otherwise recreate the tag or fail, and preserve failure when the
required tag is missing.
3 participants
Stable subset and custom runs are also used to finish or retry individual artifacts. The workflow previously keyed GitHub tag/release creation to
release-scope: all, so the successful 0.3.0 custom run skipped its tag and GitHub Release.This makes
stablethe release intent: every successful non-dry-run stable run converges on its version tag and GitHub Release, regardless of artifact scope. Retries are idempotent:The extra
create-github-releasecheckbox and its planner wiring are removed.Incident: https://github.com/NVIDIA-NeMo/nemo-platform/actions/runs/30477833073/job/90703676069
Human attention needed
actionlint .github/workflows/release.yamlandgit diff --checkpass.stablefor a subset with a brand-new version now cuts the version tag/release. This is intentional;release-scopecontrols artifacts only.Validation
actionlint .github/workflows/release.yamlgit diff --checkall,wheels,containers,helm, andcustomscopes share the same release gate.Summary by CodeRabbit
Bug Fixes / Improvements
Documentation