Fixes #20564 - ignore locking in migration #370
Conversation
|
Issues: #20564 |
|
Random failures hmmm [test] |
|
Test [test] |
|
So previous [test] failed with: HostTest::location or organizations are not enabled.test_0081_non-admin user with edit_hosts permission can update interface Trying again. |
|
One [test] failed, a different one of course. Test Result (1 failure / +1) |
|
LGTM, not tested yet. Note that this won't benecessary soon, plugin DSL will always update role permissions according to what's defined in engine.rb. |
|
@dLobatog mind quick merge here? this only adds |
| default_permissions[role_name].each do |permission| | ||
| role.add_permissions!(permission) unless role.permission_names.include?(permission.to_sym) | ||
| end | ||
| role.ignore_locking do |r| |
There was a problem hiding this comment.
You don't need to ignore locking twice - if you do it at the class level it will be ignored for all instances.
| default_permissions[role_name].each do |permission| | ||
| role.add_permissions!(permission) unless role.permission_names.include?(permission.to_sym) | ||
| Role.ignore_locking do | ||
| role = Role.find_by_name(role_name) || next |
There was a problem hiding this comment.
Role.unscoped. no user is set here so I think Role.find_by_name will be scoped to not show everything in all orgs/locs
| Role.ignore_locking do | ||
| role = Role.find_by_name(role_name) || next | ||
| default_permissions[role_name].each do |permission| | ||
| role.add_permissions!(permission) unless role.permission_names.include?(permission.to_sym) |
There was a problem hiding this comment.
Nitpick, since it's calling .save! on line 12, use add_permissions(permission) here - add_permissions! will save it once, then twice at the end of the loop.
| @@ -1,12 +1,14 @@ | |||
| default_permissions = Foreman::Plugin.find("foreman_discovery").default_roles | |||
There was a problem hiding this comment.
nitpick, all strings in here can safely be frozen, you can add
# frozen_string_literal: true
at the beginning of the file for better performance & thread safety (I know it's a little ridiculous for a loop with 2 objects but it's easy enough)
lzap
force-pushed
the
ignore-locking-20564
branch
from
8b0db29
to
69dcd52
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Ok amended all remarks thanks for such a triple review that's nice :-) |
| default_permissions[role_name].each do |permission| | ||
| role.add_permissions(permission) unless role.permission_names.include?(permission.to_sym) | ||
| end | ||
| role.update_attributes :origin => "discovery", :description => "Discovery plugin built-in role" |
There was a problem hiding this comment.
update_attributes already saves the role, no need to call .save! after. Set all attributes then save to ensure it's saved only once:
role.add_permissions(permission)
role.origin = 'discovery'
role.description = 'discovery plugin builtin'
role.save(sorry GitHub saved the comment but didn't publish it with the review)
|
@dLobatog just like that? |
lzap
force-pushed
the
ignore-locking-20564
branch
from
69dcd52
to
0058877
Compare
Until there is a way for plugin API to do this for us, we need to ignore locking to prevent this error:
Validation failed: Filters.role is locked for user modifications