-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
This pull request has been linked to Clubhouse Story #15962: Checks for required cluster API resources. |
results := make([]*api.CheckResult, 0) | ||
dc := k.client.Discovery() | ||
lists, err := dc.ServerPreferredResources() |
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.
the Discovery APIs unfortunately don't accept a context, which means they can't be cancelled -- it's why the version checks re-implement the Discovery stuff
I filed a bug but 🤷♂️ I don't think anything can be done about it, really kubernetes/client-go#1001
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.
That's unfortunate :( At least here, the worst case is the user gets impatient and hits ^C
.
dc := k.client.Discovery() | ||
lists, err := dc.ServerPreferredResources() | ||
if err != nil { | ||
results = append(results, api.ErrorResult(checkName, "unable to fetch api resources from server", err)) |
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.
should this be SKIP rather than FAIL? after all, it's an indeterminate result - we don't know that the API resources are missing, and we don't know that they're available, either
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.
Not being able to check for API resources implies a lack of permissions or some other issue, which I'd argue is a FAIL.
Edit: nah you're right it's a skip. I've to add more resources, so will do this in another PR.
Firstly some renaming of the existing RBACRequirements to a more general kind of ResourceRequirement, and making the requirements a map for easier lookup.
Also moved the placed where the requirements are defined to its own file. It seemed right at the time.
The unit tests for this one are a bit of a mess -- they work for now, but I'd be interested to get some input on making them less horrible.