fix: specify which object failed in override error message#686
Merged
Yetkin Timocin (ytimocin) merged 2 commits intoMay 13, 2026
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
e827232 to
b6d1622
Compare
Yetkin Timocin (ytimocin)
added a commit
to ytimocin/kubefleet
that referenced
this pull request
May 12, 2026
Address review feedback on PR kubefleet-dev#686: - formatOverrideTarget: fall back to "Unknown" when the unstructured target has an empty Kind, so a malformed snapshot doesn't render as `"" "name"` in user-facing error messages. - applyOverrideRules: drop the NewUnexpectedBehaviorError log wrapper on the IsClusterMatched failure path. The outer applyOverrides wrap already tags this as ErrUserError, so classifying the log line as "unexpected" contradicted the user-error classification. - Add TestFormatOverrideTarget covering namespaced, cluster-scoped, and the new empty-Kind fallback (both scopes). - Trim the verbose doc and test comments introduced by this PR. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Address review feedback on PR kubefleet-dev#686: - formatOverrideTarget: fall back to "Unknown" when the unstructured target has an empty Kind, so a malformed snapshot doesn't render as `"" "name"` in user-facing error messages. - applyOverrideRules: drop the NewUnexpectedBehaviorError log wrapper on the IsClusterMatched failure path. The outer applyOverrides wrap already tags this as ErrUserError, so classifying the log line as "unexpected" contradicted the user-error classification. - applyOverrides: switch the outer wrap from the inline fmt.Errorf("%w: ...", controller.ErrUserError, ...) form to controller.NewUserError(fmt.Errorf(...)) to match the form used by every other call site in the codebase. Behaviorally identical (same Error() string, same errors.Is, same trim output at workgenerator/controller.go:188-194). - Add TestFormatOverrideTarget covering namespaced, cluster-scoped, and the new empty-Kind fallback (both scopes). - Trim the verbose doc and test comments introduced by this PR. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
401b3b6 to
8988ed6
Compare
Britania Rodriguez Reyes (britaniar)
approved these changes
May 12, 2026
63da14c
into
kubefleet-dev:main
16 checks passed
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.
Description of your changes
When an override snapshot fails to apply on a selected resource, the binding's
Overridden=Falsecondition message now identifies which override snapshot and which target object failed, instead of surfacing only the underlying patch error.Before:
After:
Implementation: the wrap is done once at the outer
applyOverridescall site so the existing controller-level message trim atcontroller.go:188-194works correctly without doubled sentinel prefixes.applyOverrideRulesandapplyJSONPatchOverrideno longer callcontroller.NewUserErrorthemselves — they return raw errors, andapplyOverridesdoes the sentinel-tagging once with the snapshot + target context.errors.Is(err, controller.ErrUserError)still holds end-to-end.Boy Scout cleanups limited to the files touched by this fix:
override.go—isClusterScopeResource→isClusterScopedResource; capitalised log messages;cluster.ObjectMeta.Labels→cluster.Labels; doc comments on three unexported functions; redundant// do nothingdropped.override_test.go—unmarshl→unmarshal(3 sites);clusterole→clusterRole; corrected the wrong function name in a test failure diagnostic (TestReplaceClusterLabelKeyVariableswas loggingapplyJSONPatchOverride());expected/expectErr→want/wantErrper project style; test renamedTestApplyOverrides_namespacedScopeResource→_namespaceScopedResource; explanatory comment added for the inverted-lookingIsClusterScopedResource: falseFakeManagerflag.controller_integration_test.go— double-space typos inBy(...)strings.I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
make testpasses (pkg/controllers/workgeneratorruns 86 Ginkgo specs plus unit tests;pkg/utils/conditionandpkg/controllers/placementunaffected and green).TestApplyOverrides_clusterScopedResourceandTestApplyOverrides_namespaceScopedResourcegained awantErrSubstrfield. Four failure cases now assert on the new message format (snapshot identity + target identity), including two cases that previously had nowantErrSubstrand would have surfaced an empty snapshot name post-change.IsClusterMatchederror path throughapplyOverrideRules(an invalidMatchExpressionsOperatormakesmetav1.LabelSelectorAsSelectorfail). This path was previously unexercised inoverride_test.go.Bound ClusterResourceBinding … invalid overridecontext now asserts viaMatchRegexpon the new message structure.Bound ClusterResourceBinding … invalid resource overridemirrors the CRO failure path forResourceOverrideSnapshot. New fixtureinvalidResourceOverrideSnapshot(ro-2) added insuite_test.go.Special notes for your reviewer
applyOverrideRulesandapplyJSONPatchOverrideno longer callcontroller.NewUserError. Re-wrapping an already-tagged error created a doubled"failed to process the request due to a client error: "prefix that the trim atcontroller.go:188-194couldn't strip cleanly. Doing the wrap once atapplyOverrideskeeps the%w: <suffix>shape the trim expects.controller.go:188-194and its//TODO: check if it's user error and set a different failed reasonare unchanged; cleaning that up is a separate refactor.EventonOverridden=Falsesokubectl describe bindingshows the same enriched message.Overridden=Falserollup condition message with the failing cluster names instead of just a count.