-
Notifications
You must be signed in to change notification settings - Fork 321
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
Give better error when policy was created manually #412
Conversation
a55aec7
to
6b25d9c
Compare
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.
Looks great!
6b25d9c
to
7731bf3
Compare
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.
Looks great! A couple of minor comments, but they are not blocking.
tokenFile, fileCleanup := writeTempFile(t, bootToken) | ||
defer fileCleanup() |
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.
This isn't blocking or anything, but we can also move the cleanup to the writeTempFile
helper function if we call t.Cleanup
there. I like that it declutters the code a bit.
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.
Done!
c.ACL.Tokens.Master = bootToken | ||
}) | ||
require.NoError(err) | ||
svr.WaitForServiceIntentions(t) |
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 wondering if we need this since this command or test aren't creating service intentions.
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.
Changed to WaitForLeader()
In cases where a user creates a policy out-of-band with the same name as one consul-k8s tries to update, give a better error. This is definitely an edge case but has occurred when a user created the cross-namespace-acl-policy manually when trying to work around another issue. This will at least give a better error message in this case.
7731bf3
to
1d929ac
Compare
In cases where a user creates a policy out-of-band with the same name as
one consul-k8s tries to update, give a better error.
This is definitely an edge case but has occurred when a user created the
cross-namespace-acl-policy manually when trying to work around another
issue. This will at least give a better error message in this case.
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: