fix: validate kubernetes checker #7
Conversation
|
This pull request has been linked to Clubhouse Story #15968: Checks for RBAC permissions. |
internal/checks/kube/kubernetes.go
Outdated
| @@ -40,6 +41,10 @@ func NewKubernetesChecker(client kubernetes.Interface, opts ...Option) *Kubernet | |||
| opt(checker) | |||
| } | |||
|
|
|||
| // This may be nil. It is the responsibility of the caller to run | |||
There was a problem hiding this comment.
just thinking aloud, not a strong opinion by any means -- what do you think about running Validate here, and panicing if an error is returned? it's programmer error in that case, since it means they called Run without Validate, and a panic with a message seems better than a panic caused by a nil later?
In the "correct" case, where a developer calls Validate before Run, then it's never possible for Validate to fail, so there won't be a panic. The only time a panic should occur is when the caller forgets to call Validate before Run, and in that case getting a stack trace and error message could be helpful
the downside of course is that Validate is run twice, but those checks should be relatively inexpensive, so that seems fine to me
There was a problem hiding this comment.
That's actually much nicer.
I don't like the idea of users getting a faceful of stacktrace though, so adding a recover() at the top-level.
Call
Validate()and ensure that we have all the information we need before proceeding with checks.Per #6 (comment)