Skip to content
This repository was archived by the owner on Nov 14, 2024. It is now read-only.

feat: check for resources #9

Merged
merged 5 commits into from
Aug 27, 2021
Merged

Conversation

johnstcn
Copy link
Member

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.

@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #15962: Checks for required cluster API resources.

@johnstcn johnstcn requested a review from jawnsy August 26, 2021 14:25
@johnstcn johnstcn self-assigned this Aug 26, 2021
results := make([]*api.CheckResult, 0)
dc := k.client.Discovery()
lists, err := dc.ServerPreferredResources()
Copy link
Contributor

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

Copy link
Member Author

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))
Copy link
Contributor

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

Copy link
Member Author

@johnstcn johnstcn Aug 27, 2021

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.

@johnstcn johnstcn merged commit 8afac90 into main Aug 27, 2021
@johnstcn johnstcn deleted the cianjohnston/ch15962/check_for_resources branch August 27, 2021 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants