Deprecate imagestreamtag acceptance

[bradmwilliams]

This PR contains the following changes:

1. release-controller-api

   • Derive the release phase status from the ReleasePayload conditions instead of annotations on the release imagestreamtag
   • Replace release.openshift.io/message annotation reads with PayloadOverride.Reason from the ReleasePayload spec

2. release-tool.py:

   • Remove the legacy imagestreamtag patching path from — accept/reject now exclusively patches the ReleasePayload

3. release-payload-controller:

   • Fix a race condition in ComputeJobState() that could prematurely reject a release while blocking jobs still had retries outstanding

Summary by CodeRabbit

• New Features
  • Added an admin mode to the release tool for elevated accept/reject actions.
• Bug Fixes
  • Improved phase/status handling across release listings, details, and dashboards by basing resolution on release payload state.
  • Corrected failing/rejection evaluation and link/alert behavior when phases are unknown or job/aggregate states are empty.
• Tests
  • Expanded unit tests for release phase derivation and job state computation, including unknown/empty edge cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9635c40c-1fe2-42ad-8eb4-792970aa7f8c

📥 Commits

Reviewing files that changed from the base of the PR and between 33bd6bb and 3853d63.

📒 Files selected for processing (2)

• cmd/release-controller-api/http.go
• hack/release-tool.py

🚧 Files skipped from review as they are similar to previous changes (2)

• cmd/release-controller-api/http.go
• hack/release-tool.py

---

📝 Walkthrough

Walkthrough

Release phase lookup now comes from ReleasePayload data instead of tag annotations across the API and release tool. Job-state aggregation and payload phase fallback logic were updated, along with related tests. The release tool now patches ReleasePayloads and supports admin impersonation.

Changes

Release phase migration

Layer / File(s)	Summary
Job state and phase fallback pkg/releasepayload/jobstatus/helpers.go, pkg/releasepayload/jobstatus/helpers_test.go, pkg/release-controller/release_payload.go, pkg/release-controller/release_payload_test.go, pkg/cmd/release-payload-controller/payload_accepted_controller_test.go, pkg/cmd/release-payload-controller/payload_rejected_controller_test.go	ComputeJobState now treats empty aggregate states as undetermined and prioritizes pending. GetReleasePhase now returns Ready or Pending from ConditionPayloadCreated, with tests covering the updated outcomes.
Phase resolution helpers cmd/release-controller-api/http_helper.go	Adds resolvePhase over ReleasePayload lookup and updates phase alerting, linkability, final-state checks, and previous-release selection to consume resolved phase values.
HTTP handlers and dashboard logic cmd/release-controller-api/http.go, cmd/release-controller-api/http_candidate.go	Release APIs, release-info rendering, dashboard failure checks, stream filtering, and candidate selection now call c.resolvePhase(...); template maps remove phaseAlert.
Release tool patching and admin options hack/release-tool.py	Accept/reject actions now patch ReleasePayload, --admin adds system:admin impersonation, approval uses the updated signature, and exception handling is adjusted across command paths.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hop through payloads, phase by phase,
No tag annotations in the olden maze.
Pending, Ready, Unknown too,
The release winds change in a fresher hue.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.35% which is insufficient. The required threshold is 80.00%.	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 matches the main change: moving acceptance handling away from imagestreamtag-based logic.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

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)

cmd/release-controller-api/http.go (1)

2232-2244: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle maxUnready == 0 before counting phases.

When maxUnready is 0, this loop is skipped and the function returns true, so Line 2195 marks the stream as failing even though Line 2187 treats 0 as “no unready limit”. That makes dashboard status wrong for configs that leave MaxUnreadyReleases unset.

Suggested fix

 func isReleaseFailing(phases []string, maxUnready int) bool {
+	if maxUnready <= 0 {
+		return false
+	}
 	unreadyCount := 0
 	for i := 0; unreadyCount < maxUnready && i < len(phases); i++ {
 		switch phases[i] {

🤖 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 `@cmd/release-controller-api/http.go` around lines 2232 - 2244, The
isReleaseFailing function treats maxUnready == 0 as failing because the loop is
skipped and it returns true, which conflicts with the intended “no unready
limit” behavior. Update isReleaseFailing to explicitly handle maxUnready == 0 as
non-failing before the phase counting logic, and keep the existing
ReleasePhaseReady and ReleasePhaseAccepted handling intact so the dashboard
status remains correct when MaxUnreadyReleases is unset.

🤖 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 `@hack/release-tool.py`:
- Line 586: The approval flow is missing admin impersonation, so `approval()`
still runs as the normal user even when `--admin` is enabled. Update the
`approval` function to accept the same `options` dict used elsewhere and wrap
its operations in `oc.options(options)` before any accept/reject/delete actions.
Also update the caller that invokes `approval()` to pass through the parsed
`--admin` options so the releasepayload label writes and approval actions run
with the intended impersonation.

---

Outside diff comments:
In `@cmd/release-controller-api/http.go`:
- Around line 2232-2244: The isReleaseFailing function treats maxUnready == 0 as
failing because the loop is skipped and it returns true, which conflicts with
the intended “no unready limit” behavior. Update isReleaseFailing to explicitly
handle maxUnready == 0 as non-failing before the phase counting logic, and keep
the existing ReleasePhaseReady and ReleasePhaseAccepted handling intact so the
dashboard status remains correct when MaxUnreadyReleases is unset.

🪄 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: 1b0b76a6-772a-416a-85f0-83c952c8f2c2

📥 Commits

Reviewing files that changed from the base of the PR and between ac520a0 and 21030f3.

📒 Files selected for processing (10)

• cmd/release-controller-api/http.go
• cmd/release-controller-api/http_candidate.go
• cmd/release-controller-api/http_helper.go
• hack/release-tool.py
• pkg/cmd/release-payload-controller/payload_accepted_controller_test.go
• pkg/cmd/release-payload-controller/payload_rejected_controller_test.go
• pkg/release-controller/release_payload.go
• pkg/release-controller/release_payload_test.go
• pkg/releasepayload/jobstatus/helpers.go
• pkg/releasepayload/jobstatus/helpers_test.go

[bradmwilliams]

/label tide/merge-method-squash

[coderabbitai]

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)

hack/release-tool.py (1)

591-594: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale missing-payload diagnostic.

Line 593 now looks up releasepayload/{release}, but the error still says imagestreamtag, which will mislead operators on lookup failures.

Proposed fix

-                    logger.error(f'Unable to locate imagestreamtag: releasepayload/{release}')
+                    logger.error(f'Unable to locate releasepayload: {namespace}/releasepayload/{release}')

🤖 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 `@hack/release-tool.py` around lines 591 - 594, The missing-payload diagnostic
in the release lookup path is stale and refers to imagestreamtag even though
releasepayload/{release} is being queried. Update the logger.error message in
the payload lookup block of the release-tool flow to mention
releasepayload/{release} and keep the wording aligned with the selector used in
that branch.

🤖 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 `@hack/release-tool.py`:
- Line 863: The call to approval() is still passing the obsolete
release_image_stream argument, which no longer matches the function signature
and causes a positional argument mismatch. Update the invocation in the
release-tool flow to match approval(ctx, options, namespace, release, team,
accept, reject, delete, check, execute) by removing the stale imagestream
argument while keeping the remaining arguments in the correct order.

---

Outside diff comments:
In `@hack/release-tool.py`:
- Around line 591-594: The missing-payload diagnostic in the release lookup path
is stale and refers to imagestreamtag even though releasepayload/{release} is
being queried. Update the logger.error message in the payload lookup block of
the release-tool flow to mention releasepayload/{release} and keep the wording
aligned with the selector used in that branch.

🪄 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: f8d0d8fd-4b54-4ea4-a0f3-ee0b4e15349a

📥 Commits

Reviewing files that changed from the base of the PR and between 21030f3 and 7929e5a.

📒 Files selected for processing (1)

• hack/release-tool.py

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexNPavel, bradmwilliams

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [AlexNPavel,bradmwilliams]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@bradmwilliams: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.