Uh oh!
There was an error while loading. Please reload this page.
✨ Reformat error wrapping for resolved bundle status - #1938
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| errMissingRBAC = "pre-authorization failed: service account requires the following permissions to manage cluster extension:\n" + | ||
| " Namespace:\"\" APIGroups:[] Resources:[services] Verbs:[list,watch]\n" + | ||
| " Namespace:\"test-namespace\" APIGroups:[*] Resources:[certificates] Verbs:[create]\n" |
There was a problem hiding this comment.
I would suggest using backticks to format multi-line strings and/or strings containing quotation marks:
| errMissingRBAC="pre-authorization failed: service account requires the following permissions to manage cluster extension:\n"+ | |
| " Namespace:\"\" APIGroups:[] Resources:[services] Verbs:[list,watch]\n"+ | |
| " Namespace:\"test-namespace\" APIGroups:[*] Resources:[certificates] Verbs:[create]\n" | |
| errMissingRBAC= `pre-authorization failed: service account requires the following permissions to manage cluster extension: | |
| Namespace:"" APIGroups:[] Resources:[services] Verbs:[list,watch] | |
| Namespace:"test-namespace"APIGroups:[*] Resources:[certificates] Verbs:[create] | |
| ` |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #1938 +/- ##
==========================================
+ Coverage 66.32% 66.66% +0.33%
==========================================
Files 75 75 Lines 6326 6326 ==========================================
+ Hits 4196 4217 +21 + Misses 1866 1844 -22 - Partials 264 265 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // End the error in a newline since this is a heavily formatted collection of errors such as missing RBAC rules | ||
| // and we don't want it to run along into any following errors, i.e. the status error wrapping in clusterextension_controller |
There was a problem hiding this comment.
Is there an example of where this actually happens? If so, that's probably because we are not using errors.Join in that location?
(i.e. if we used errors.Join all the way up the stack, we'd get newlines between all the errors, but no extraneous newlines otherwise)
There was a problem hiding this comment.
That will put our error message at the beginning of the string, and I could see how that could make formatting look strange. Maybe we should reformat that string so that the error still ends up at the end. Would that resolve the problem you are seeing with the lack of a trailing new line?
There was a problem hiding this comment.
Yep, that's the specific location where this is happening. If it's fine to rework this, then I'll do that instead and make it use Join().
There was a problem hiding this comment.
Yeah, let's do that instead. But in this case it isn't a different error. This is just adding some context to this error. I think we should try to just rewrite the wrapping such that it is <context>: <underlying error>
kuiwang02
commented
Apr 27, 2025
/lgtm |
Adds another test case to helm_test.go for when PreAuthorize() returns missing RBAC rules. Merges the noOpPreauthorizer and errPreAuthorizer into one mockPreAuthorizer in helm_test.go. Reverses the structure of the error string in wrapErrorWithResolutionInfo such that the context is first and the error follows. Signed-off-by: Tayler Geiger <tayler@redhat.com>
New changes are detected. LGTM label has been removed. |
Fix#1923
Adds another test case to helm_test.go for when PreAuthorize() returns missing RBAC rules.
Description
Reviewer Checklist