[V2V] Add conversion_host.id to task options #18540
Conversation
|
|
||
| vddk_url = params.delete(:param_v2v_vddk_package_url) | ||
|
|
||
| conversion_host = new(params) | ||
|
|
||
| unless task_id.nil? |
There was a problem hiding this comment.
Probably change this to if task_id.present? in case it comes through as an empty param instead of nil. I've come to be paranoid about such things when dealing with http.
| task.options[:conversion_host_id] = conversion_host.id | ||
| task.save! | ||
| end | ||
|
|
There was a problem hiding this comment.
I'm ok with this, but I'm thinking it should only happen if we know the conversion_host.save! was successful.
There was a problem hiding this comment.
Won't .save! raise an exception and not get here if it failed?
|
Checked commits fabiendupont/manageiq@c12dd97~...16364c3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| params.delete(:miq_task_id) # The miq_queue.activate_miq_task will stick in a :miq_task_id | ||
| task_id = params.delete(:task_id) # In case this is being called through *_queue which will stick in a :task_id | ||
| miq_task_id = params.delete(:miq_task_id) # The miq_queue.activate_miq_task will stick in a :miq_task_id | ||
| task_id ||= miq_task_id |
There was a problem hiding this comment.
What? no no lets just fix enable_queue to set :miq_task_id and standardize on a single key
There was a problem hiding this comment.
Yup. I didn't like it neither. How would you fix enable_queue ?
There was a problem hiding this comment.
Actually it looks like the only caller (./manageiq-api/app/controllers/api/conversion_hosts_controller.rb) just does task_id = ConversionHost.enable_queue(data.except('auth_user'), data['auth_user']) so where were you seeing a task_id parameter coming from? Because from that it looks like it gets the task_id back rather than passing one in.
There was a problem hiding this comment.
I took that from existing code... @djberg96 could you shed some light here ?
There was a problem hiding this comment.
Yes, the enable_queue method will return the task_id on a create request, but we don't have a direct way to associate the task back to the conversion instance after that.
There was a problem hiding this comment.
But enable_queue isn't passing in the task_id as a parameter right? So the only option should be miq_task_id
There was a problem hiding this comment.
They're injected into the params via the MiqTask.generic_action_with_callback at https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_task.rb#L290-L292
There was a problem hiding this comment.
But those are different hashes, only task_id is passed in to the method opts
|
|
||
| if task_id.present? | ||
| task = MiqTask.find(task_id) | ||
| task.options[:conversion_host_id] = conversion_host.id |
There was a problem hiding this comment.
Is options right? I thought it was context...
There was a problem hiding this comment.
We've used options so much that it sounded like the logical choice.
@djberg96 would you recommend context_data over options ?
There was a problem hiding this comment.
I mean is options even an attribute of an miq_task?
>> MiqTask.create!.options
NoMethodError (undefined method `options' for #<MiqTask:0x000055dc41439fe0>)
There was a problem hiding this comment.
@fdupont-redhat you're probably thinking of an MiqRequestTask, that has an options column but MiqTask does not.
There was a problem hiding this comment.
@agrare Now I'm curious what the difference between those two.
|
Yeah, I was too hasty with the approve button... I don't think we want to use context_data because I want to use that for the |
So you'd have to create the conversion host in |
|
@agare, I think that plan is shot because it doesn't appear that records in |
|
@djberg96 could we maybe use |
|
@mturley I guess there's no reason we couldn't do that. Working on it... |
|
@fdupont-redhat Might be easiest if I just include this as part of #18541. |
|
This pull request is not mergeable. Please rebase and repush. |
|
@agrare @fdupont-redhat I would say yes, close this. |
The conversion host configuration is asynchronous and we need to associate the async task with the conversion host, so that we can manipulate the tasks in the UI. For example, we only show the last configuration task for a given conversion host in the UI. This PR add the conversion host id to the enablement task options. The disable task is not required as it destroys the conversion host.
Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1622728