Skip to content
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

[V2V] Provide placeholder params for the ConversionHost#disable method #18691

Merged
merged 4 commits into from
Apr 26, 2019
Merged

[V2V] Provide placeholder params for the ConversionHost#disable method #18691

merged 4 commits into from
Apr 26, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Apr 25, 2019

Currently the disable_queue method will fail with wrong number of arguments (given 2, expected 0). This is because the disable_queue method calls the queue_configuration method which automatically passes params and args to whatever the op is.

So, this PR just adds a couple dummy placeholder parameters in order to comply with the interface.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1703254

@djberg96
Copy link
Contributor Author

@miq-bot add_label bug, transformation, hammer/yes

@djberg96
Copy link
Contributor Author

@miq-bot add_reviewer @agrare

@miq-bot miq-bot requested a review from agrare April 26, 2019 12:51
@agrare
Copy link
Member

agrare commented Apr 26, 2019

@djberg96 can you add specs to cover these cases?

@djberg96
Copy link
Contributor Author

@agrare Working on it. I'm actually somewhat baffled that the existing disable_queue spec didn't blow up on it:

https://github.com/ManageIQ/manageiq/blob/master/spec/models/conversion_host/configurations_spec.rb#L143-L159

@agrare
Copy link
Member

agrare commented Apr 26, 2019

@djberg96 that doesn't actually deliver the message so it never tried to call the disable method, you could call msg = MiqQueue.first; msg.deliver

@djberg96
Copy link
Contributor Author

djberg96 commented Apr 26, 2019

@agrare Ok, added a spec, and confirmed that it fails if the params are removed. Only downside is that I couldn't get it to work without resorting to allow_any_instance_of to work, hope that's ok for now.

Or, if you have any suggestions... :)

@djberg96
Copy link
Contributor Author

Specifically, if I use the usual allow(conversion_host).to receive(:disable_conversion_host_role), it's still trying to call ansible_playbook via the disable_conversion_host_role method.

This results in a undefined method userid' for nil:NilClasserror coming from theansible_playbook` method, which it should never be trying to call.

Does it have a different internal reference when the disable method is called? Not sure yet.

@agrare
Copy link
Member

agrare commented Apr 26, 2019

@djberg96 I think that's because MiqQueue is doing a query for the record so it is getting its own copy not the mocked object from the spec. You could possibly doing something like allow(ConversionHost).to_receive(find).with(conversion_host.id).and_return(conversion_host)

@djberg96
Copy link
Contributor Author

@agrare Fantastic, thanks, updated.

@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2019

Checked commits https://github.com/djberg96/manageiq/compare/c6515e93ed4ee0826e184e606efa12a67fe30221~...6941105053dac1d57e010a8e3624666a38906314 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare agrare merged commit 32fa034 into ManageIQ:master Apr 26, 2019
@agrare agrare self-assigned this Apr 26, 2019
@agrare agrare added this to the Sprint 110 Ending Apr 29, 2019 milestone Apr 26, 2019
simaishi pushed a commit that referenced this pull request Apr 29, 2019
[V2V] Provide placeholder params for the ConversionHost#disable method

(cherry picked from commit 32fa034)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1704457
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit f64f7bccbbd178e2c21eb025a2d7fe0a5ff5b626
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Apr 26 14:37:36 2019 -0400

    Merge pull request #18691 from djberg96/fix_disable_queue_args
    
    [V2V] Provide placeholder params for the ConversionHost#disable method
    
    (cherry picked from commit 32fa0345ed8822482d3e6a1e5aa03dfcf4dacb24)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1704457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants