Force targets to be an array so we can each them in cb run on multiple objects #18056
Conversation
|
@miq-bot add_label bug |
d-m-u
changed the title
app/models/custom_button.rb
Outdated
| @@ -96,7 +96,7 @@ def invoke(target, source = nil) | |||
| end | |||
|
|
|||
| def publish_event(source, target, args) | |||
| target.kind_of?(Array) ? target.each { |t| create_event(source, t, args) } : create_event(source, target, args) | |||
| target.size > 1 ? target.each { |t| create_event(source, t, args) } : create_event(source, target, args) | |||
There was a problem hiding this comment.
I think target.kind_of?(Enumerable) would be more accurate, right?
There was a problem hiding this comment.
Could we simplify this line to: Array(target).each { |t| create_event(source, t, args) }
d-m-u
force-pushed
the
use_whoops_size_rather_than_possibly_nonexistent_array
branch
from
405c88d
to
24b0e80
Compare
d-m-u
changed the title
|
@d-m-u 'bdunne, tinaafitz' is an invalid reviewer, ignoring... |
spec/models/custom_button_spec.rb
Outdated
| let(:vm3) { FactoryGirl.create(:vm_vmware) } | ||
| let!(:vm) { FactoryGirl.create(:vm_vmware) } | ||
| let!(:vm2) { FactoryGirl.create(:vm_vmware) } | ||
| let!(:vm3) { FactoryGirl.create(:vm_vmware) } |
There was a problem hiding this comment.
Why the change to let!. Variables vm2 and vm3 are only used in one test but will not get created for each.
There was a problem hiding this comment.
@bdunne, is allow(Vm).to receive(all) ... better than let! here? Or ... something else?
There was a problem hiding this comment.
Are they needed for the rest of the tests? If not, https://github.com/ManageIQ/manageiq/pull/18056/files#diff-71376e22701f18c6f0be76915e41ef84R331 should create them, so you shouldn't need let! or the allow(Vm).to receive(:all)....
There was a problem hiding this comment.
To be clear, I did this change for https://github.com/ManageIQ/manageiq/pull/18056/files#diff-71376e22701f18c6f0be76915e41ef84R349.
d-m-u
force-pushed
the
use_whoops_size_rather_than_possibly_nonexistent_array
branch
from
24b0e80
to
e1ad456
Compare
spec/models/custom_button_spec.rb
Outdated
| let(:vm3) { FactoryGirl.create(:vm_vmware) } | ||
| let(:vm) { FactoryGirl.create(:vm_vmware) } | ||
| let(:vm2) { FactoryGirl.create(:vm_vmware) } | ||
| let(:vm3) { FactoryGirl.create(:vm_vmware) } |
There was a problem hiding this comment.
I like that after a few rounds of changes the sole purpose here is to undo the alignment with the let blocks below. 😝
d-m-u
force-pushed
the
use_whoops_size_rather_than_possibly_nonexistent_array
branch
from
e1ad456
to
e9b1983
Compare
|
Checked commit d-m-u@e9b1983 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
d-m-u
deleted the
use_whoops_size_rather_than_possibly_nonexistent_array
branch
…ibly_nonexistent_array Force targets to be an array so we can each them in cb run on multiple objects (cherry picked from commit ead55ec) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628224
|
Hammer backport details: |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628224
... #18042 failed cause it's sometimes not an array that we should be checking for when running CBs on multiple things, mea culpa.
related:
ManageIQ/manageiq-automation_engine#258