Open
Conversation
📝 WalkthroughWalkthroughPropagate and preserve existing errors during status patching in TraitsController; surface and return errors from consumer-allocation deletion in CleanupResourceProvider. Tests added to assert both failure paths. Changes
Sequence Diagram(s)(Skipped — changes are focused error propagation and tests, not a new multi-component control flow that requires diagramming.) 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
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
fwiesel
force-pushed
the
more-no-swallow-fixes
branch
from
7a206a3 to
0a4b33b
Compare
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
0a4b33b to
b872bd8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/traits_controller_test.go (1)
294-298: Assert the error source, not only error presence.This currently passes on any reconcile failure. Please also assert the returned error matches the GetTraits failure path (e.g., contains
500and/or/resource_providers/1234/traits) so the test guards the intended behavior.💡 Suggested tightening
It("should return the GetTraits error", func(ctx SpecContext) { req := ctrl.Request{NamespacedName: hypervisorName} _, err := traitsController.Reconcile(ctx, req) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("500")) + Expect(err.Error()).To(ContainSubstring("/resource_providers/1234/traits")) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/traits_controller_test.go` around lines 294 - 298, The test currently only checks that traitsController.Reconcile returns an error; tighten it to assert the error comes from the GetTraits failure by checking the error message contains the expected indicator (e.g., "500" or the GetTraits endpoint path like "/resource_providers/1234/traits"). Update the spec that calls traitsController.Reconcile(ctx, req) to capture err and then assert Expect(err.Error()).To(ContainSubstring("500")) or Expect(err.Error()).To(ContainSubstring("/resource_providers/1234/traits")) (or use MatchError with ContainSubstring) so the failure is proven to be from the GetTraits path.internal/openstack/placement_test.go (1)
323-338: Make this failure-path test stricter on cause and side effect.Right now it accepts any error and still permits provider deletion. For this scenario, assert the wrapped consumer-deletion error and fail the test if delete provider is called.
💡 Suggested tightening
- // 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) + defer GinkgoRecover() + Fail("should not be called") }) }) @@ err := CleanupResourceProvider(ctx, client.ServiceClient(fakeServer), provider) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("deleteConsumerAllocations failed for consumer consumer-1")) }) })🤖 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 - 338, The test for CleanupResourceProvider should be tightened to (1) simulate a concrete consumer-allocation deletion error in the fake consumer-delete handler and assert that the returned err from CleanupResourceProvider wraps/equals that specific error (use errors.Is or Expect(err).To(MatchError(...)) against the expectedErr), and (2) make the fake provider-delete handler fail the test if it is ever invoked (e.g., call t.Fatalf or set a flag and Expect it to remain false) so the test ensures provider deletion does not occur when consumer deletion fails; update the handlers referenced in the test (the consumer delete handler and the fakeServer.Mux.HandleFunc for deleteProviderURL) and the assertion on the returned err from CleanupResourceProvider accordingly.
🤖 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/controller/traits_controller_test.go`:
- Around line 294-298: The test currently only checks that
traitsController.Reconcile returns an error; tighten it to assert the error
comes from the GetTraits failure by checking the error message contains the
expected indicator (e.g., "500" or the GetTraits endpoint path like
"/resource_providers/1234/traits"). Update the spec that calls
traitsController.Reconcile(ctx, req) to capture err and then assert
Expect(err.Error()).To(ContainSubstring("500")) or
Expect(err.Error()).To(ContainSubstring("/resource_providers/1234/traits")) (or
use MatchError with ContainSubstring) so the failure is proven to be from the
GetTraits path.
In `@internal/openstack/placement_test.go`:
- Around line 323-338: The test for CleanupResourceProvider should be tightened
to (1) simulate a concrete consumer-allocation deletion error in the fake
consumer-delete handler and assert that the returned err from
CleanupResourceProvider wraps/equals that specific error (use errors.Is or
Expect(err).To(MatchError(...)) against the expectedErr), and (2) make the fake
provider-delete handler fail the test if it is ever invoked (e.g., call t.Fatalf
or set a flag and Expect it to remain false) so the test ensures provider
deletion does not occur when consumer deletion fails; update the handlers
referenced in the test (the consumer delete handler and the
fakeServer.Mux.HandleFunc for deleteProviderURL) and the assertion on the
returned err from CleanupResourceProvider accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffad9a31-51e8-44af-8e3b-5a6bca92ae8b
📒 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 (2)
- internal/controller/traits_controller.go
- internal/openstack/placement.go
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