-
Notifications
You must be signed in to change notification settings - Fork 898
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
Convert Vmdb::PermissionsStore to a blacklist #19053
Conversation
end | ||
|
||
def can?(permission) | ||
@permissions.include?(permission) | ||
!@blacklist.include?(permission) |
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.
Hard to tell, but the big change is the !
here.
d501369
to
eedcd65
Compare
Since it's now a blacklist, there's no need for a sample file with the full possible whitelist
eedcd65
to
de224fd
Compare
Checked commit Fryguy@de224fd with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
Looks like a sporadic failure https://travis-ci.org/ManageIQ/manageiq/jobs/563230333#L694-L703 |
Looks good to me. |
This causes CI failure in ui-classic:
|
@martinpovolny @himdel Gah! I said to myself "you know, I should run the UI tests before this gets merged". If I had my bot-driven test system in place, it wouldn't have been a thought...I would have just done it. 😠 |
Even better, if one the Ruby travis tasks from UI ran on each core PR ;-) (just 15 minutes of execution) |
Convert Vmdb::PermissionsStore to a blacklist (cherry picked from commit 7146738)
Ivanchuk backport details:
|
It's uncommon to prevent features, so the permissions store works better as a blacklist. Otherwise, we are forced to add new items to the whitelist everytime we add things, even when most of the time we actually want them.
Other repos should not need to change except that I will need to change our production whitelist to a blacklist after this is merged.
@gtanzillo Please review.
cc @h-kataria @martinpovolny @agrare