Enhanced API to have a task created for provider refreshes #14387
Conversation
abellotti
changed the title
|
marked as WIP. Needs an enhancement for actions creating multiple tasks (i.e. provider_class=provider) where refresh targets multiple managers, we only return the task_id and task_href for the first. Could be done in a separate PR, will need to see how the client handles that. |
abellotti
force-pushed
the
api_provider_refresh_task
branch
from
e15e885
to
c9d68d3
Compare
|
Looks like this is needed as per @mkanoor Un-wipping with caveat that for multi-manager providers, only the task for the first manager is returned as per Issue #14404. /cc @Fryguy @imtayadeway please review, Thanks!! |
abellotti
changed the title
app/models/ext_management_system.rb
Outdated
| @@ -400,14 +400,15 @@ def last_refresh_status | |||
| end | |||
| end | |||
|
|
|||
| def refresh_ems | |||
| def refresh_ems(opts = nil) | |||
There was a problem hiding this comment.
Is there a concern that you might explicitly send the argument nil to this method? If not, opts = {} should suffice (without the following line)
app/models/provider.rb
Outdated
| if missing_credentials? | ||
| raise _("no %{table} credentials defined") % {:table => ui_lookup(:table => "provider")} | ||
| end | ||
| unless authentication_status_ok? | ||
| raise _("%{table} failed last authentication check") % {:table => ui_lookup(:table => "provider")} | ||
| end | ||
| managers.each { |manager| EmsRefresh.queue_refresh(manager) } | ||
| managers.collect { |manager| EmsRefresh.queue_refresh(manager, nil, opts) }.flatten |
There was a problem hiding this comment.
Perhaps flat_map here for a single operation?
spec/requests/api/providers_spec.rb
Outdated
| provider.update_authentication(:default => default_credentials.symbolize_keys) | ||
| allow_any_instance_of(provider.class).to receive_messages(:authentication_status_ok? => true) | ||
|
|
||
| run_post(providers_url(provider.id) + '?provider_class=provider', gen_request(:refresh)) |
There was a problem hiding this comment.
I take it whatever caused us to have to write queries like this is still an issue? 😁
There was a problem hiding this comment.
yes, we'll need to enhance the run_post method to support options like we do with run_get.
and returns the appropriate task_id and task_href in
the action result.
so a successful "refresh" action now return something like:
{
"success": true,
"message": "Provider id:2 name:'rhev36' refreshing",
"task_id": 55,
"task_href": "http://localhost:3000/api/tasks/55",
"href": "http://localhost:3000/api/providers/2"
}
- Supports both ext_management_system and provider (i.e. provider_class=provider)
- Added spec
abellotti
force-pushed
the
api_provider_refresh_task
branch
from
c9d68d3
to
fefaa14
Compare
…t_map over collect.flatten
abellotti
force-pushed
the
api_provider_refresh_task
branch
from
fefaa14
to
30ea361
Compare
spec/requests/api/providers_spec.rb
Outdated
|
|
||
| provider = FactoryGirl.create(:ext_management_system, sample_vmware.symbolize_keys.except(:type)) | ||
| provider.update_authentication(:default => default_credentials.symbolize_keys) | ||
| allow_any_instance_of(provider.class).to receive_messages(:authentication_status_ok? => true) |
There was a problem hiding this comment.
Although still problematic, WDYT about doing something like:
provider.authentication_type(:default).update(:status => "Valid")to avoid the use of any_instance_of?
|
Checked commits abellotti/manageiq@f7715b5~...8d45246 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Anything else needed for this PR ? |
|
@gtanzillo @imtayadeway @gmcculloug @Fryguy This feature is critical for calling a refresh from Ansible Playbooks, without this feature the playbook would have to sleep for an arbitrary amount of time hoping that the refresh has completed. |
Enhanced API to have a task created for provider refreshes and returns the appropriate task_id and task_href in the action result.
so a successful refresh action now return something like:
Added spec