Fix result CR status not updated on retry#41
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughcreateIdempotent now, on AlreadyExists, fetches the existing Result CR, merges new .status.conditions and result-type-specific status via copyResultStatus, and applies a status patch so the stored CR reflects the newly-built status. ChangesStatus Update on Existing Result CRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @jameswnl. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
jameswnl
force-pushed
the
fix/result-cr-update-on-retry
branch
2 times, most recently
from
0ce6363 to
d4d2a37
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controller/proposal/results_test.go (1)
117-177: ⚡ Quick winAdd retry-overwrite coverage for the other result kinds.
copyResultStatusnow manually maps four concrete status types, but the new regression coverage only exercisesAnalysisResult. A table-drivenAlreadyExistssuite forExecutionResult,VerificationResult, andEscalationResult—including sandbox replacement—would catch the same stale-status regression if any non-analysis branch drifts.🤖 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 `@controller/proposal/results_test.go` around lines 117 - 177, The test covers only AnalysisResult and misses regressions in the other concrete status branches of copyResultStatus; add a table-driven variant of TestCreateIdempotent_AlreadyExists_OverwritesStaleFailure that iterates over the other kinds ("ExecutionResult", "VerificationResult", "EscalationResult" and sandbox replacement cases), constructing appropriate existing and new CR objects for each kind, calling createIdempotent with the fake client, and asserting that stale FailureReason, Conditions and Options are correctly overwritten for each type; reference copyResultStatus and createIdempotent to ensure each branch is exercised and reuse the existing test's verification logic (Get + check Status.Options, FailureReason, Conditions) inside the loop.controller/proposal/results.go (1)
306-317: ⚡ Quick winUse the repo error-constant pattern for the new status-update errors.
These new
fmt.Errorfcalls hardcode the message strings inline. Please lift them intoErr...constants and wrap from those constants so the new paths match the repository’s Go error convention. As per coding guidelines,**/*.go:Define errors as constants using const ErrFoo = "…"and wrap them withfmt.Errorf("%s: %w", …)for context`.🤖 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 `@controller/proposal/results.go` around lines 306 - 317, The inline fmt.Errorf messages in the status-update path should follow the repo pattern: add package-level const error strings (e.g., const ErrGetExistingStatus = "get existing status" and const ErrUpdateExistingStatus = "update existing status") and replace the two fmt.Errorf calls that wrap getErr and patchErr with fmt.Errorf("%s: %w", ErrGetExistingStatus, getErr) and fmt.Errorf("%s: %w", ErrUpdateExistingStatus, patchErr) respectively; keep the existing context (kind, obj.GetName()) in the surrounding message if desired, but ensure the new constants are used as the wrapped base per the repository convention (affecting the calls around c.Get(...), existing, patched, and c.Status().Patch(...)).
🤖 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.
Nitpick comments:
In `@controller/proposal/results_test.go`:
- Around line 117-177: The test covers only AnalysisResult and misses
regressions in the other concrete status branches of copyResultStatus; add a
table-driven variant of
TestCreateIdempotent_AlreadyExists_OverwritesStaleFailure that iterates over the
other kinds ("ExecutionResult", "VerificationResult", "EscalationResult" and
sandbox replacement cases), constructing appropriate existing and new CR objects
for each kind, calling createIdempotent with the fake client, and asserting that
stale FailureReason, Conditions and Options are correctly overwritten for each
type; reference copyResultStatus and createIdempotent to ensure each branch is
exercised and reuse the existing test's verification logic (Get + check
Status.Options, FailureReason, Conditions) inside the loop.
In `@controller/proposal/results.go`:
- Around line 306-317: The inline fmt.Errorf messages in the status-update path
should follow the repo pattern: add package-level const error strings (e.g.,
const ErrGetExistingStatus = "get existing status" and const
ErrUpdateExistingStatus = "update existing status") and replace the two
fmt.Errorf calls that wrap getErr and patchErr with fmt.Errorf("%s: %w",
ErrGetExistingStatus, getErr) and fmt.Errorf("%s: %w", ErrUpdateExistingStatus,
patchErr) respectively; keep the existing context (kind, obj.GetName()) in the
surrounding message if desired, but ensure the new constants are used as the
wrapped base per the repository convention (affecting the calls around
c.Get(...), existing, patched, and c.Status().Patch(...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: febb71c4-c5a0-41d3-82c3-3a1d50df3288
📒 Files selected for processing (2)
controller/proposal/results.gocontroller/proposal/results_test.go
When a proposal step is retried and the result CR name collides with an existing one, createIdempotent previously returned nil on AlreadyExists, leaving stale failure data in the CR. This caused the AnalysisResult to show a failed status even when the retry succeeded. Now on AlreadyExists, the existing CR is fetched and its status is patched with the new result data (options, actions, checks, conditions, sandbox info, failure reason) while preserving the original metadata and spec. - Use comma-ok type assertions in copyResultStatus for safety - Add test for the core retry scenario: stale failure data overwritten with success data, verifying FailureReason is cleared Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jameswnl
force-pushed
the
fix/result-cr-update-on-retry
branch
from
d4d2a37 to
6543a47
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
Summary
resultCRName),createIdempotentsilently returnednilonAlreadyExists, leaving stale failure data in the CRAnalysisResult(and other result types) to show failed status/conditions even when the retry succeeded — the proposal would showAnalyzed: True (Complete)but the result CR still contained the old failure reasonAlreadyExists, the existing CR is fetched and its status is patched with the latest result data (options, actions, checks, conditions, sandbox info, failure reason)Test plan
TestCreateIdempotent_AlreadyExistsupdated to verify status is updated (not skipped)go test ./controller/proposal/... -count=1🤖 Generated with Claude Code
Summary by CodeRabbit