Aggregates: Do not swallow error

[fwiesel]

The error was actually in result.Err, not err. So handle and return that one.

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Improved error handling to ensure both update and patch operation failures are properly captured and reported together, providing more complete error context.

[coderabbitai]

📝 Walkthrough

Walkthrough

The Reconcile function in the traits controller now preserves and properly composes errors from trait updates and patch applications. Previously, patch errors could overwrite update errors; now both are captured together in the final returned error.

Changes

Cohort / File(s)	Summary
Error Handling in Trait Reconciliation internal/controller/traits_controller.go	Modified error composition logic in Reconcile to preserve the original trait update error while also capturing any subsequent patch application errors, combining both into the final returned error instead of allowing one to overwrite the other.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 An error once lost in the patch's embrace,
Now both errors sit face-to-face,
Combined and preserved with care,
No mistake left unaware! 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Aggregates: Do not swallow error' directly describes the main change: fixing error handling to avoid losing (swallowing) errors in the reconciliation logic.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch do-not-swallow-error

📝 Coding Plan

• Generate coding plan for human review comments

---

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

Tip

Migrating from UI to YAML configuration.

Use the @coderabbitai configuration command in a PR comment to get a dump of all your UI settings in YAML format. You can then edit this YAML file and upload it to the root of your repository to configure CodeRabbit programmatically.

[Copilot]

Pull request overview

Fixes error handling in the traits update reconciliation flow so the actual Placement update error is surfaced instead of being overwritten by a subsequent status patch error.

Changes:

• Use result.Err from openstack.UpdateTraits(...) as the primary error signal.
• Preserve the original update error by errors.Join(err, statusPatchErr) when attempting to patch status conditions on failure.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/controller/traits_controller.go (1)

105-111: ⚠️ Potential issue | 🔴 Critical

GetTraits failure can still be swallowed when status patch succeeds.

On this path, err is overwritten with errors.Join(tc.Status().Patch(...)), so a successful patch can turn a real GetTraits error into nil and report reconcile success.

Suggested fix

 if err != nil {
 	base := hv.DeepCopy()
 	if meta.SetStatusCondition(&hv.Status.Conditions,
 		getTraitCondition(err, "Failed to get current traits from placement")) {
-		err = errors.Join(tc.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
-			k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(TraitsControllerName)))
+		err = errors.Join(err,
+			tc.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
+				k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(TraitsControllerName)))
 	}
 	return ctrl.Result{}, err
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/traits_controller.go` around lines 105 - 111, The current
code overwrites the original GetTraits error by assigning err =
errors.Join(tc.Status().Patch(...)), which can turn a real GetTraits failure
into nil when the patch succeeds; fix this in the block that handles hv/status
patching by capturing the original error (e.g., getTraitsErr := err) before the
patch call, call patchErr := tc.Status().Patch(ctx, hv,
k8sclient.MergeFromWithOptions(base, k8sclient.MergeFromWithOptimisticLock{}),
k8sclient.FieldOwner(TraitsControllerName)), and then set err =
errors.Join(getTraitsErr, patchErr) (or err = getTraitsErr if you prefer
explicit logic) so the original GetTraits error is preserved and combined with
any patch error; adjust the code around hv, meta.SetStatusCondition,
getTraitCondition and the tc.Status().Patch invocation accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@internal/controller/traits_controller.go`:
- Around line 105-111: The current code overwrites the original GetTraits error
by assigning err = errors.Join(tc.Status().Patch(...)), which can turn a real
GetTraits failure into nil when the patch succeeds; fix this in the block that
handles hv/status patching by capturing the original error (e.g., getTraitsErr
:= err) before the patch call, call patchErr := tc.Status().Patch(ctx, hv,
k8sclient.MergeFromWithOptions(base, k8sclient.MergeFromWithOptimisticLock{}),
k8sclient.FieldOwner(TraitsControllerName)), and then set err =
errors.Join(getTraitsErr, patchErr) (or err = getTraitsErr if you prefer
explicit logic) so the original GetTraits error is preserved and combined with
any patch error; adjust the code around hv, meta.SetStatusCondition,
getTraitCondition and the tc.Status().Patch invocation accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a7dd38d1-622f-43bc-827f-e1b7863c2ca1

📥 Commits

Reviewing files that changed from the base of the PR and between dc7b81a and 3057ddd.

📒 Files selected for processing (1)

• internal/controller/traits_controller.go

[github-actions]

Merging this branch will increase overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller	63.09% (+0.04%)	👍

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/traits_controller.go	60.94% (+0.62%)	64 (+1)	39 (+1)	25	👍

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.