🐛 OCPBUGS-55165 Permissions preflight error output fixes

[trgeiger]

Description

This PR contains 2 changes:

• Adds an errors.Join() to properly expand a list of errors that might include nil errors
• Adds explicit rule examples to 2 of the test cases

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	d3edc3e
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6807c31754c4cd00086a9fbe
😎 Deploy Preview	https://deploy-preview-1934--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 65.91%. Comparing base (e529653) to head (d3edc3e).

Files with missing lines	Patch %	Lines
internal/operator-controller/authorization/rbac.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1934      +/-   ##
==========================================
- Coverage   66.01%   65.91%   -0.10%     
==========================================
  Files          70       70              
  Lines        6182     6182              
==========================================
- Hits         4081     4075       -6     
- Misses       1841     1845       +4     
- Partials      260      262       +2     

Flag	Coverage Δ
e2e	45.09% <0.00%> (ø)
unit	55.12% <33.33%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bentito]

We should probably add a comment here saying why this newline is needed.

But I thought about it a bit more and I wonder if we should try to not put the newlines in the error, make the tests assert on the error contents without the newline and then post-process newlines and maybe bullets for each point, into the rendered return:

so like:

       // join descriptions with commas; formatting (newlines/bullets) will be applied at error render time
       preAuthErrors = append(preAuthErrors,
           fmt.Errorf("service account requires the following permissions to manage cluster extension: %s",
               strings.Join(missingRuleDescriptions, ", ")))
   }
	if authErr != nil {
		preAuthErrors = append(preAuthErrors, fmt.Errorf("authorization evaluation error: %w", authErr))
	}
   if len(preAuthErrors) > 0 {
       // Render the collected errors with line breaks for clarity
       var b strings.Builder
       b.WriteString("pre-authorization failed:")
       for _, e := range preAuthErrors {
           b.WriteString("\n  - ")
           b.WriteString(e.Error())
       }
       return errors.New(b.String())
   }
   return nil

[bentito]

it avoids the nolint and is cleaner maybe

[trgeiger]

Changing the missing rules join is going to make the rules output into one giant long string. It ends up as one single error in preAuthErrors so your loop is only adding a newline at the last rule.

[bentito]

Hmm, not my intention, now I gotta go try it maybe not just suggest it ;-) Was hoping for that range with comma separated errors to solve the nolint and allow for building the errors into human readable form with a return to separate them from the context.

[joelanford]

Maybe we simply need to errors.Join(preAuthErrors...) with no other changes?

	if len(preAuthErrors) > 0 {
		return fmt.Errorf("pre-authorization failed: %v", errors.Join(preAuthErrors...))
	}

[trgeiger]

Change made, it seems to work

[bentito]

LGTM