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

Update service template API #14068

Merged
merged 1 commit into from
Mar 13, 2017
Merged

Update service template API #14068

merged 1 commit into from
Mar 13, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Feb 24, 2017

This PR updates the way update on a service template works.

Previously, it was using the generic method, but it requires the update_catalog_item method added in #13811 with the same format required in create and returned via show:

        {
          :name        => 'Updated Template Name',
          :display     => 'false',
          :description => 'a description',
          :config_info => {
            :miq_request_dialog_name => request_dialog.name,
            :placement_auto          => [true, 1],
            :number_of_vms           => [1, '1'],
            :src_vm_id               => [new_vm.id, new_vm.name],
            :vm_name                 => new_vm.name,
            :schedule_type           => ['immediately', 'Immediately on Approval'],
            :instance_type           => [flavor.id, flavor.name],
            :src_ems_id              => [ems.id, ems.name],
            :provision               => {
              :fqname    => ra1.fqname,
              :dialog_id => nil
            },
            :reconfigure             => {
              :fqname    => ra3.fqname,
              :dialog_id => service_dialog.id
            }
          }
        }

@abellotti since update was already implemented, I am not sure if this is considered a breaking change that requires a version bump.

@miq-bot add_label enhancement, api
@miq-bot assign @abellotti

@abellotti
Copy link
Member

how different are the update payloads without and with this PR ?

I think previous release service_templates edit was basic stuff, name/description which should still valid with this PR.

@jntullo
Copy link
Author

jntullo commented Mar 1, 2017

@abellotti the payloads now take in the config_info - name, description etc is all still valid

@abellotti
Copy link
Member

Ahh ok, no need to bump version then.

However, I'm trying to just edit name (no config_info in update payload), and I'm getting the following:

POST /api/service_templates/1

{
  "action" : "edit",
  "resource" : { "name" : "updated_vm_provision_catalog_item" }
}
{
  "error": {
    "kind": "bad_request",
    "message": "Could not update Service Template - undefined method `except' for nil:NilClass",
    "klass": "Api::BadRequestError"
  }
}

Maybe there needs to be a fix to the update_catalog_item() method ?

Could you also add a test that exercises above failure ? i.e. just update name.

Thanks

@jntullo
Copy link
Author

jntullo commented Mar 1, 2017

@abellotti ah well now that you mention that, I guess there is more to it 😳 - the config_info is required, as it expects the same format that it gets from show - the update includes both updating / adding / removing attributes, which is why it's necessary to send back the config_info. I will add validation on it

@jntullo
Copy link
Author

jntullo commented Mar 2, 2017

@abellotti LMK what you think about this update. We can keep it so that it uses the super class unless config_info is present. Added in a test for it too

@abellotti
Copy link
Member

Thanks for adding a test for update with just name attribute.

While this works, it might be better if update_catalog_item() could handle both cases. Could you see what it would take to update that method ? i.e. maybe only call update_service_resources and update_resource_actions only if config_info is present.

@jntullo
Copy link
Author

jntullo commented Mar 2, 2017

@bzwei would appreciate your input here - should the API allow for updating of service_templates without config info (ie can update just basic attributes such as name)? Or should we require config_info?

@bzwei
Copy link
Contributor

bzwei commented Mar 2, 2017

I don't have personal preference. Our current backend code would require it. If we make config_info optional, then more code changes are required.

If from common API practice any field is optional and missing field means do not update, then yes, config_info should be optional too. In reality from our UI usage it seems every field will be provided.

@jntullo
Copy link
Author

jntullo commented Mar 3, 2017

@abellotti going to check for config_info in update_catalog_item in #14147

update service template to return the reload
@miq-bot
Copy link
Member

miq-bot commented Mar 10, 2017

Checked commit jntullo@200ff70 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. ⭐

@abellotti abellotti added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 13, 2017
@abellotti
Copy link
Member

This looks great @jntullo 👍

Thanks @jntullo and @bzwei for making the updates in the model more flexible.

@abellotti abellotti merged commit db29ce2 into ManageIQ:master Mar 13, 2017
@jntullo jntullo deleted the enhancement/update_service_template_api branch November 28, 2017 19:42
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