Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add helm revoke functionality #50
Add helm revoke functionality #50
Changes from 4 commits
b83fb81
91519f0
5d95d08
4df66ea
d3d0a7a
e38f71d
e08bc98
6655848
aab1704
d57f992
9bdc2e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 one issue I see with this is that you're assuming a certain type of error will be returned: role does not exist. But if a different error occurs here (e.g. "permissions denied") the test won't distinguish between the two.
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.
I'm assuming that I cannot get the role (or role binding, or secret; more or less the same approach for each of these tests). If I can get the role, then something went wrong on the revocation. If there was a permissions problem or otherwise, it would've been thrown in
RevokeAccess()
.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.
If you take the updated labels approach I proposed here (#50 (comment)), you can use the
List
interface instead for a more targeted existence check.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.
+1 on Josh's comment above: we should check which error comes back in all these checks, as these could be returning errors for a variety of reasons, and not all of them are necessarily what we're checking for.
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.
Also, is the real way to test this to try to make requests as the user, check they succeed, revoke the user's access, and check the requests again to make sure they fail?
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 would certainly be the more complete test. However,
kubergrunt
currently doesn't run any tests from the perspective of multiple users. It relies on the kubectl credentials currently available in the executing user's path. Adding the ability to switch from admin/user context is a bit more time than I can afford at the moment unfortunately and would have some downstream implications.