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

fix: validate kubernetes checker#7

Merged
johnstcn merged 3 commits intomainfrom
cianjohnston/ch15968/validate_kube_checker
Aug 25, 2021
Merged

fix: validate kubernetes checker#7
johnstcn merged 3 commits intomainfrom
cianjohnston/ch15968/validate_kube_checker

Conversation

@johnstcn
Copy link
Copy Markdown
Member

Call Validate() and ensure that we have all the information we need before proceeding with checks.
Per #6 (comment)

@shortcut-integration
Copy link
Copy Markdown

This pull request has been linked to Clubhouse Story #15968: Checks for RBAC permissions.

@johnstcn johnstcn requested a review from jawnsy August 24, 2021 16:01
@johnstcn johnstcn self-assigned this Aug 24, 2021
opt(checker)
}

// This may be nil. It is the responsibility of the caller to run
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@johnstcn johnstcn merged commit 7362138 into main Aug 25, 2021
@johnstcn johnstcn deleted the cianjohnston/ch15968/validate_kube_checker branch August 25, 2021 09:34
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