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

Modify the enable ConversionHost::Configurations#enable method to handle arguments more robustly #18336

Merged
merged 3 commits into from
Jan 11, 2019
Merged

Modify the enable ConversionHost::Configurations#enable method to handle arguments more robustly #18336

merged 3 commits into from
Jan 11, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jan 7, 2019

In the course of working on the REST API interface for the enable method, I realized there were two issues. First, we cannot guarantee that the keys to the params argument will be symbols or strings. Second, the constantize method will fail if the case of the argument isn't set properly, which IMO is an implementation detail that a consumer of the REST API should not have to worry about.

For the first case, I felt the best thing to do was to simply symbolize all of the arguments so we know we're always dealing with symbols.

For the second case, I modified the call first downcase then classify the resource_type argument so that constantize won't fail. Thus, someone using the REST API can pass "vm", "VM", or "Vm" and it will Just Work.

Update: Made some similar changes to the enable_queue method as well.

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

@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2019

Checked commits https://github.com/djberg96/manageiq/compare/2abf1551ab021b56d5b3d2686573886400b270cd~...0f91297d59449eb8b3232364e5077e890082ae91 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. 🏆

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 9, 2019

It looks to me like the tests have passed on Travis, but github is still showing them as pending for some reason.

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 9, 2019

@jameswnl Look alright?

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 9, 2019

@jameswnl I think there's some kind of side-effect happening in MiqTask.generic_action_with_callback, but I haven't nailed it down yet.

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 9, 2019

@jameswnl With some help from @jrafanie and others, there's some pass by reference argument mangling happening within MiqTask.generic_action_with_callback.

In fact, the :task_id is getting added to the params in the current specs, it just wasn't apparent because it's modifying the original params, whereas with my approach a copy was being made, so it was checking the copy vs the original, which caused the failure.

So, there are two options. One is to leave it the way it is right now. The other is to use symbolize_keys! so that it uses the original (modified) parameter. I'm going to leave it as-is for now, unless you object.

@jameswnl
Copy link
Contributor

jameswnl commented Jan 9, 2019

@djberg96 ok, got it. I am ok with leaving it as-is.

@djberg96 djberg96 closed this Jan 11, 2019
@djberg96 djberg96 reopened this Jan 11, 2019
@kbrock kbrock added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 11, 2019
@kbrock kbrock self-assigned this Jan 11, 2019
@kbrock kbrock merged commit 933ff48 into ManageIQ:master Jan 11, 2019
@djberg96
Copy link
Contributor Author

djberg96 commented Apr 4, 2019

@miq-bot remove_label hammer/no

@miq-bot miq-bot removed the hammer/no label Apr 4, 2019
@djberg96
Copy link
Contributor Author

djberg96 commented Apr 4, 2019

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Apr 22, 2019
Modify the enable ConversionHost::Configurations#enable method to handle arguments more robustly

(cherry picked from commit 933ff48)

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

Hammer backport details:

$ git log -1
commit a40a0211ab3b41cd16a617a77678bcffa5e41b75
Author: Keenan Brock <[email protected]>
Date:   Fri Jan 11 13:15:50 2019 -0500

    Merge pull request #18336 from djberg96/conversion_host_params
    
    Modify the enable ConversionHost::Configurations#enable method to handle arguments more robustly
    
    (cherry picked from commit 933ff48245f76831e0c00efbd9739ffad60e04ab)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702023

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.

5 participants