-
Notifications
You must be signed in to change notification settings - Fork 60
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 migration aimed at removing RBAC on RBAC roles #192
Add migration aimed at removing RBAC on RBAC roles #192
Conversation
Ugh, and tests failing. let me fix that. Left in some PDB statements |
Signed-off-by: Chris Mitchell <[email protected]>
Signed-off-by: Chris Mitchell <[email protected]>
bb778c1
to
b2e663a
Compare
# if so, just delete the access object we know is RBAC, and leave the role | ||
if non_rbac_permissions: | ||
# this will still delete the resource definitions for the access object | ||
access.delete() |
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 actually needs to be rbac_permission
here, I believe?
access.delete() | |
rbac_permission.delete() |
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.
Yep, I think you're right. That's what I get for playing with naming just before submitting >_<
|
||
|
||
|
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.
Just a nit, but we can probably do away with a few of these newlines?
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.
Just checked both branches of the logic here and found one minor issue, but I tried the fix locally and it seemed to worked both ways.
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.
Nice 👍
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 good!! 👍
This seems like it works from some local testing, but definitely needs a couple of extra eyes on it.
Signed-off-by: Chris Mitchell [email protected]