Open
Conversation
📝 WalkthroughWalkthroughUpdate: propagate and preserve existing errors during status patching in TraitsController and return errors from consumer-allocation deletion in CleanupResourceProvider; tests added to assert both failure paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes two cases where real errors were being dropped during cleanup/reconcile flows, ensuring callers get the underlying failure instead of a nil error.
Changes:
- Propagate consumer allocation deletion errors in
CleanupResourceProviderinstead of discarding them. - Preserve the original
GetTraitserror when also patching status conditions inTraitsController.Reconcile. - Add regression tests covering both previously-swallowed error paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/openstack/placement_test.go | Adds a regression test ensuring allocation-delete failures are returned. |
| internal/openstack/placement.go | Returns early when consumer allocation deletion fails (no longer swallows error). |
| internal/controller/traits_controller_test.go | Adds a regression test ensuring GetTraits errors are not lost after status patching. |
| internal/controller/traits_controller.go | Joins the original error with the status patch error instead of overwriting it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/openstack/placement_test.go (1)
323-327: Strengthen this test by asserting provider deletion is never reached.Using a failing handler here would lock in the early-return behavior on consumer deletion errors.
🧪 Proposed test hardening
- // The provider delete succeeds, so that it cannot accidentally surface - // the error for us — only the consumer allocation delete error should be returned. - fakeServer.Mux.HandleFunc(deleteProviderURL, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNoContent) - }) + // Provider delete should not be called when consumer allocation deletion fails. + fakeServer.Mux.HandleFunc(deleteProviderURL, func(w http.ResponseWriter, r *http.Request) { + defer GinkgoRecover() + Fail("should not be called") + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/openstack/placement_test.go` around lines 323 - 327, The provider-delete handler in the test currently returns 204 which lets the provider path succeed and doesn't assert it was never called; change the fakeServer.Mux.HandleFunc for deleteProviderURL so that if that handler is invoked it fails the test (e.g., call t.Fatalf or t.Error with a clear message) to assert provider deletion is never reached, and ensure the closure captures the test *t* properly so the failure is reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/traits_controller_test.go`:
- Around line 297-299: The comment describing a past bug (which says "err is
reassigned to errors.Join(patchErr) instead of errors.Join(originalErr,
patchErr)...") is now outdated and misleading; remove or replace that comment in
the test so it no longer documents pre-fix behavior — locate the comment
referencing errors.Join, patchErr, and GetTraits in the tests (in
traits_controller_test.go) and delete the bug-note or update it to a short,
accurate explanation of the current behavior.
In `@internal/openstack/placement_test.go`:
- Around line 337-339: Remove the outdated bug comment that mentions
deleteConsumerAllocations' return value being discarded in
internal/openstack/placement_test.go; the test and code have been updated so
that error handling is correct, so simply delete the three-line comment
referencing the bug to avoid misleading future readers and keep comments in sync
with the current behavior of deleteConsumerAllocations.
---
Nitpick comments:
In `@internal/openstack/placement_test.go`:
- Around line 323-327: The provider-delete handler in the test currently returns
204 which lets the provider path succeed and doesn't assert it was never called;
change the fakeServer.Mux.HandleFunc for deleteProviderURL so that if that
handler is invoked it fails the test (e.g., call t.Fatalf or t.Error with a
clear message) to assert provider deletion is never reached, and ensure the
closure captures the test *t* properly so the failure is reported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1006a62-4195-4f20-b6bc-8b60a85a42a9
📒 Files selected for processing (4)
internal/controller/traits_controller.gointernal/controller/traits_controller_test.gointernal/openstack/placement.gointernal/openstack/placement_test.go
fwiesel
force-pushed
the
more-no-swallow-fixes
branch
from
0368bf3 to
7a206a3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/openstack/placement.go (1)
154-156: IncludeconsumerIDin propagated delete errors for diagnosability.At Line 155, returning the raw error makes it harder to identify which consumer failed when multiple allocations are processed. Consider wrapping with context.
Suggested patch
- if err := deleteConsumerAllocations(ctx, client, consumerID).Err; err != nil { - return err + if err := deleteConsumerAllocations(ctx, client, consumerID).Err; err != nil { + return fmt.Errorf("failed to delete allocations for consumer %s: %w", consumerID, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/openstack/placement.go` around lines 154 - 156, The delete error returned from deleteConsumerAllocations is missing the consumer identifier; update the error return in the block that calls deleteConsumerAllocations(ctx, client, consumerID) so it wraps or annotates the underlying error with the consumerID (e.g., use fmt.Errorf or errors.Wrapf) to produce a message like "deleteConsumerAllocations failed for consumer %s: %w" referencing the consumerID variable and the original err; modify the return at the call site (the code that currently returns err) to return the wrapped/annotated error instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/openstack/placement.go`:
- Around line 154-156: The delete error returned from deleteConsumerAllocations
is missing the consumer identifier; update the error return in the block that
calls deleteConsumerAllocations(ctx, client, consumerID) so it wraps or
annotates the underlying error with the consumerID (e.g., use fmt.Errorf or
errors.Wrapf) to produce a message like "deleteConsumerAllocations failed for
consumer %s: %w" referencing the consumerID variable and the original err;
modify the return at the call site (the code that currently returns err) to
return the wrapped/annotated error instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe8990c4-61ed-413e-b93d-fe02b2389bf0
📒 Files selected for processing (4)
internal/controller/traits_controller.gointernal/controller/traits_controller_test.gointernal/openstack/placement.gointernal/openstack/placement_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/openstack/placement_test.go
Both in the traits controller as well as in the placement-api calls, some errors were swallowed.
fwiesel
force-pushed
the
more-no-swallow-fixes
branch
from
7a206a3 to
0a4b33b
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both in the traits controller as well as in the placement-api calls, some errors were swallowed.
Summary by CodeRabbit
Bug Fixes
Tests