Uh oh!
There was an error while loading. Please reload this page.
Migrate syncPending to ReleasePayloads - #790
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRelease payload phase handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/release-controller/sync.go (1)
377-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated changelog pre-cache goroutine across both Ready branches.
The
SortedRawReleaseTags+ goroutine block that pre-caches the changelog is identical in the stable Ready branch (Lines 381-409) and the non-stable Ready branch (Lines 461-489). Consider extracting a helper (e.g.,c.precacheChangelog(release, tag)) to remove the duplication and keep the two paths from drifting.Also applies to: 457-489
🤖 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/sync.go` around lines 377 - 409, The changelog pre-cache logic is duplicated in both Ready branches of sync handling, so factor the repeated SortedRawReleaseTags plus goroutine block into a shared helper on the controller, such as c.precacheChangelog(release, tag), and call it from both releasecontroller.ReleasePhaseReady paths. Keep the helper responsible for the same pullspec lookup, GetImageInfo calls, and ChangeLog pre-cache behavior so the two branches stay consistent and don’t drift.
🤖 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 `@cmd/release-controller/sync.go`:
- Around line 410-413: The stable ReleasePhaseFailed path in syncRelease is
dropping the underlying failure diagnostics and should be aligned with the
non-stable Failed branch. Update the ReleasePhaseFailed case to retrieve the job
termination message using ensureJobTerminationMessageRetrieved and wrap the
result with withLog, then pass that richer message into
transitionReleasePhaseFailure instead of the bare reasonAndMessage call. Keep
the behavior consistent with the non-stable failure handling so both branches
preserve the same debugging context.
- Around line 371-374: The ReleasePayload lookup in ensureReleasePayload is
treating a lister NotFound as a hard error, which can happen before the cache
catches up. Update the Get(tag.Name) error handling in ensureReleasePayload to
detect errors.IsNotFound(err) and follow the normal pending/default wait path
instead of returning, while preserving existing behavior for other errors.
---
Nitpick comments:
In `@cmd/release-controller/sync.go`:
- Around line 377-409: The changelog pre-cache logic is duplicated in both Ready
branches of sync handling, so factor the repeated SortedRawReleaseTags plus
goroutine block into a shared helper on the controller, such as
c.precacheChangelog(release, tag), and call it from both
releasecontroller.ReleasePhaseReady paths. Keep the helper responsible for the
same pullspec lookup, GetImageInfo calls, and ChangeLog pre-cache behavior so
the two branches stay consistent and don’t drift.
🪄 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: 90add904-70f9-4689-a484-b5bdf574e9a2
📒 Files selected for processing (1)
cmd/release-controller/sync.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
100bb3a to
318a841Compare@bradmwilliams: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Refactored syncPending(), in the release-controller, to determine the state of the release creation to the ReleasePayload CRD instead of computing it locally from the batch job itself.
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit