Skip to content

fix: address review findings on the release workflow - #3632

Open
csviri wants to merge 5 commits into
mainfrom
release-followup
Open

csviri wants to merge 5 commits into
mainfrom
release-followup

Conversation

@csviri

@csviri csviri commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #3631, addressing review findings. v5.6.2 already released cleanly through #3631. The tag points at the Release v5.6.2 commit with a non-SNAPSHOT pom, so none of this is a live breakage.

The common thread: every check that can reject a release now runs in publish, before the deploy. Maven Central artifacts are immutable once published, so a check that fails after the deploy leaves a release that can't be retried.

Keep the concurrency group, with an accurate rationale. An earlier commit on this branch removed it, because the default queue keeps only one pending run: with a release running and a second pending, a third cancels the second. That was reverted (9a246ed). Without the group, two releases from the same branch can both deploy, and then one loses the atomic branch push with immutable artifacts and no release commit. A cancelled pending run is the better worst case: it has deployed nothing and can simply be re-run. The comment in release.yml now says this.

Check the tag ancestry in publish, not finalize-release. The check is equivalent against the branch tip, since the release commit is always a child of it, and running it before the deploy means a release cut from an unrelated commit fails while it's still retryable. The tag's existence and its peeling to a commit are tested separately, so a tag that resolves to something other than a commit fails loudly instead of looking like an absent tag and skipping the check.

Lease the tag update. The ancestry check reads the tag as fetched at checkout, but +refs/tags/... would then overwrite whatever the remote holds at push time. The push now leases against the OID observed during the check (the tag ref itself, not the commit it peels to), with an empty expectation when the tag didn't exist. A tag moved by anyone else in between fails the push, and --atomic means the branch doesn't move either.

Guard against a release moving the branch backwards. The next development version is derived from the released version, so releasing a tag older than the branch's own version would set the branch to versions already released. The guard runs in publish, comparing the released version against the branch's development version before the deploy. finalize-release repeats it as a postcondition on the derived next version. This is deliberately not CodeRabbit's suggested fix of restoring the checked-out version: that breaks the common case, since releasing v5.7.0 from main on 5.6.2-SNAPSHOT must land on 5.7.1-SNAPSHOT, not go back to 5.6.2-SNAPSHOT. A monotonicity guard covers both.

branch tag next
5.6.2-SNAPSHOT v5.6.2 5.6.3-SNAPSHOT ok
5.6.2-SNAPSHOT v5.7.0 5.7.1-SNAPSHOT ok
5.6.3-SNAPSHOT v5.0.0 — abort in publish

Keep the pom version parser inline in the workflow. Both jobs read a pom version, so the parser is a single POM_VERSION_PY in the workflow-level env, run with python3 -c (1c19f3e). It deliberately isn't a script in the repo: the jobs check out the release branch, and maintenance branches (5.0.x, 5.2.x, 5.3.x) wouldn't have it.

Document the manual recovery from a rejected push. The concurrency group keeps releases apart, but not a merge landing on the branch while publish runs. That rejects the final push after the deploy, and nothing in the workflow can get past it: re-running the workflow fails in publish on the immutable artifacts, and re-running finalize-release rebuilds on the same stale commit. Both refs are left safe (branch on a SNAPSHOT version, tag where GitHub created it), so a comment on the push step (c91fb9c) now describes finishing the release by hand: tag a release commit built on the deployed commit, then bump the current branch tip to the next SNAPSHOT.

Not changed

Copilot also flagged that the push uses GITHUB_TOKEN, so snapshot-releases.yml won't run for the release commits. That's accurate (it last ran on 832857dfa and not on 0d612f32a/70988d132), but it isn't a regression, since the previous ad-m/github-push-action used the same token, and it's arguably desirable. It also doesn't affect #3631's rationale, where the trigger was always a subsequent human merge onto a branch left at a release version.

Making the workflow itself survive a concurrent merge (building the SNAPSHOT bump on the current branch tip instead of on the release commit) would remove the need for the manual recovery, but it is a larger change to the workflow and is left for a follow-up.

Testing

Run-blocks extracted from the YAML and executed verbatim against scratch repos with a stubbed mvnw and real git 2.55.0, on the current head.

Tag check in publish:

  • Tag absent, or a lightweight or annotated tag on an ancestor → passes.
  • Tag on an unrelated commit → fails, before any deploy.
  • Lightweight or annotated tag pointing at a tree → fails with "does not resolve to a commit".

Version checks in publish:

  • Branch on 5.6.3-SNAPSHOT: v5.6.3, v5.6.4, v5.7.0, v5.10.0, v6.0.0 pass; v5.6.2, v5.0.0, v4.9.9 fail.
  • Branch on 5.10.0-SNAPSHOT: v5.9.9 fails, v5.10.0 passes, so versions compare numerically rather than as text.
  • v5.6.3-rc1 and v5.6 are rejected as not plain major.minor.patch.
  • A pom without the Maven namespace makes the parser fail loudly. POM_VERSION_PY returns 5.6.3-SNAPSHOT for both the root pom and operator-framework-core/pom.xml, which inherits its version from the parent.

Push step:

  • Matching lease on an existing tag → tag moved. Empty lease with the tag absent → tag created.
  • Empty lease with the tag present → rejected.
  • Tag moved by another actor after it was read → rejected on the stale lease, remote branch and tag both untouched.
  • Concurrent merge onto the branch → push rejected, tag unmoved.

The end-to-end runs from the earlier revision (v5.7.0 from 5.7.0-SNAPSHOT → tagged 5.7.0, branch 5.7.1-SNAPSHOT; likewise v5.8.0 and v6.0.0 from 5.7.3-SNAPSHOT) haven't been repeated. The later commits changed only the checks above, the parser's location, and comments.

Summary by CodeRabbit

  • Bug Fixes

    • Added checks to prevent publishing releases based on commits outside the target branch’s history.
    • Added version checks to prevent release preparation or finalization from moving the branch back to an older version.
    • Improved tag updates to avoid overwriting changes made by another release process.
  • Release Process

    • Improved version detection and validation before publishing artifacts.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2026
@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 09db2a4e-df3a-4f1c-9e57-a6dc3029bbdb

📥 Commits

Reviewing files that changed from the base of the PR and between 6d37eef and c91fb9c.

📒 Files selected for processing (1)
  • .github/workflows/release-project-in-dir.yml

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The release workflow validates tag ancestry and version progression. It reads POM versions through an inline Python script and updates the release tag and target branch with an atomic lease-based push.

Changes

Release safety controls

Layer / File(s) Summary
Release ancestry validation
.github/workflows/release-project-in-dir.yml
The publish job fetches full history and tags. It fails if the release tag does not resolve to a commit or its commit is not an ancestor of the target branch.
Development version progression
.github/workflows/release-project-in-dir.yml
POM version reads use an inline Python script, which falls back to the parent version. The workflow rejects release and next-development versions that sort below the current development version.
Atomic release reference update
.github/workflows/release-project-in-dir.yml, .github/workflows/release.yml
The workflow uses an atomic push with a lease on the release tag and a non-forced branch refspec. The release workflow comment describes deployment serialization and push conflicts.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c91fb

No actionable release-workflow risk remains; the change is ready for normal merge checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies fixes to the release workflow, which matches the main changes in the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@csviri
csviri requested a lite review from Copilot September 22, 2026 14:07
@csviri
csviri marked this pull request as ready for review September 22, 2026 14:08
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2026
@openshift-ci
openshift-ci Bot requested review from metacosm and xstefank September 22, 2026 14:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical release-safety issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity

Open (4)
What changed in this PR

This PR hardens Maven release finalization with improved tag validation, version safeguards, and atomic ref updates.

Changes:

  • Removes workflow-level release concurrency.
  • Adds ancestry and monotonic-version checks.
  • Uses leased atomic tag and branch updates.
File Summary
.github/​workflows/​release.yml Removes release concurrency configuration.
.github/​workflows/​release-project-in-dir.yml Adds validation, version guards, and leased atomic pushes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/release-project-in-dir.yml Outdated
Comment thread .github/workflows/release-project-in-dir.yml
Comment thread .github/workflows/release-project-in-dir.yml
Comment thread .github/workflows/release.yml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Two critical workflow issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 High severity

Open (3)
Resolved since last review (3)

Comment thread .github/workflows/release-project-in-dir.yml Outdated
Comment thread .github/workflows/release.yml
@csviri
csviri requested a lite review from Copilot September 23, 2026 15:43
@csviri
csviri marked this pull request as draft September 23, 2026 15:44
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Remove the concurrency group to prevent silent cancellation of pending releases.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Follow-up to #3631.

Remove the concurrency group. It was added to serialize releases, but the
default queue keeps only one pending run: with a release running and a second
pending, a third cancels the second, which would then be tagged on GitHub but
never deployed or finalized - silently. The group was not load-bearing anyway,
since the atomic non-forced branch push already makes a concurrent release fail
visibly. Removing it trades a silent failure back for a loud one.

Check the tag ancestry in publish instead of finalize-release. The check is
equivalent against the branch tip, because the release commit is always a child
of it, and running it before the deploy means a release cut from an unrelated
commit fails while it can still be retried. Maven Central artifacts are
immutable once published.

Lease the tag update. The ancestry check reads the tag as fetched at checkout,
but `+refs/tags/...` would then overwrite whatever the remote holds at push
time. The push now leases against the OID observed during the check - the tag
ref itself, not the commit it peels to - with an empty expectation when the tag
did not exist. A tag moved by anyone else in between fails the push, and
--atomic means the branch does not move either.

Guard against a release moving the branch backwards. The next development
version is derived from the released version, so releasing a tag older than the
branch's own version - which means the wrong branch was selected - would set the
branch to versions that are already released. Deriving is still right for the
common case: releasing v5.7.0 from main on 5.6.2-SNAPSHOT must land on
5.7.1-SNAPSHOT, not go back to 5.6.2-SNAPSHOT.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Two review findings, both about checks running after the deploy has already
put immutable artifacts in Maven Central.

The tag ancestry check tested `refs/tags/<tag>^{commit}` for existence, so a tag
that resolves to something other than a commit looked the same as an absent tag
and skipped the check entirely - then finalize-release failed peeling it, after
publishing. Test the ref and peel it separately so that case fails loudly.

The monotonicity guard ran in finalize-release. A tag older than the branch's
development version but still an ancestor of it would pass publish, deploy, and
only then abort. Move the equivalent check - released version against the
branch's own version - into publish, ahead of the deploy. The guard in
finalize-release stays as a postcondition on the derived version.

Extract pom-version.py, now that both jobs need to read a pom version.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Removing it was the wrong call. Without serialization two releases cut from the
same branch both pass publish and deploy different immutable versions to Maven
Central; only then does the non-forced branch push fail one of them, leaving
published artifacts with no release commit and the tag still on the snapshot
commit. Maven Central does not allow that to be redone.

The failure mode serialization does have - a third release arriving while one
runs and one is pending replaces the pending one - leaves nothing published, so
that release can just be re-run. A recoverable failure beats an unrecoverable
one, which is the opposite of the trade described when this was removed.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
The release jobs check out the target branch, so a script added to the
repo is missing on older maintenance branches and releases from them
would fail. Keep the parser in the workflow itself instead.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Once the artifacts are deployed, neither re-running the workflow nor
re-running finalize-release can get past a push rejected by a concurrent
merge. Describe how to finish the release by hand.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri
csviri marked this pull request as ready for review September 24, 2026 07:09
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants