-
Notifications
You must be signed in to change notification settings - Fork 58
Cleanup of customCAs spec
#527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
+ Coverage 58.63% 59.43% +0.79%
==========================================
Files 56 56
Lines 6634 5132 -1502
==========================================
- Hits 3890 3050 -840
+ Misses 2472 1805 -667
- Partials 272 277 +5
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:
|
pmatseykanets
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks reasonable.
| if s.cluster.Spec.CustomCAs == nil { | ||
| return nil, nil, fmt.Errorf("customCAs not found") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be unnecessary given the conditional on L333.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true. I just wanted to be extra safe in case someone calls this method and forgets to check that. :)
| var ( | ||
| volumes []v1.Volume | ||
| mounts []v1.VolumeMount | ||
| sortedCertIDs = sortedKeys(caCertMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be
| sortedCertIDs = slices.Sorted(maps.Keys(caCertMap)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a different note is there any reason why it should be sorted? If yes is there any reason not to pre-sort it as it's hardcoded?
Moving
customCAsfield to be a pointer. This will avoid creating an empty struct by default, when not needed:Also moving the
enabledto true as default, and thesecretNameas a required field.This will result in not having the customCAs if not provided, and force the user providing the secretName.
This way the
validateCustomCACertscheck should not be needed, but...Result: