add_resource to Service api #14409

jntullo · 2017-03-20T16:47:33Z

Adds "add_resource" action to Service resources

POST /api/services
{
   "action" : "add_resource",
   "resources" : [
      { "href" : "http://localhost:3000/api/services/3", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/3", "resource" : { "href" : "http://localhot:3000/api/vms/4 }},
      { "href" : "http://localhost:3000/api/services/4", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/5", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      ...
   ]
}

POST /api/services/:id
{
  "action" : "add_resource",
  "resources" : [
    { "resource" : { "href" : "http://localhost:3000/api/vms/11" }},
    { "resource" : { "href" : "http://localhost:3000/api/vms/12" }},
    { "resource" : { "href" : "http://localhost:3000/api/vms/13" }},
    ...
  ]
}

https://www.pivotaltracker.com/story/show/142057167

@miq-bot add_label enhancement, api, service
@miq-bot assign @abellotti
cc: @mkanoor

miq-bot · 2017-03-20T16:48:18Z

@jntullo Cannot apply the following label because they are not recognized: service

jntullo · 2017-03-20T16:48:28Z

config/api.yml

@@ -1932,6 +1932,8 @@
        :identifier: service_admin
      - :name: delete
        :identifier: service_delete
+      - :name: add_resource
+        :identifier: service_edit


@mkanoor can you verify this product feature used? I didn't see one for assigning resources

@jntullo let's add this action on the collection too for completeness so we support bulk add_resources too. Thanks.

@jntullo I think the service_edit seems appropriate here

abellotti · 2017-03-20T17:09:10Z

spec/requests/api/services_spec.rb

+        'message' => "Assigned resources to Service id:#{svc.id} name:'#{svc.name}'"
+      }
+      expect(response).to have_http_status(:ok)
+      expect(response.parsed_body).to eq(expected)


Hi @jntullo can you add an expect to make sure the svc got the vms added to it ? Thanks.

mkanoor · 2017-03-20T19:09:17Z

@jntullo @abellotti @gmcculloug
We have some concerns about adding any resource to service from external interfaces (Automate Method and now REST API). Previously this feature was available only from Automate Methods and exists as a method on the service model VM Object https://github.com/ManageIQ/manageiq/blob/master/lib/miq_automation_engine/service_models/miq_ae_service_vm.rb#L7

So one of the suggestions that @gmcculloug had was to move this method from the service model to app/models/vm.rb called 'add_to_service' so in this PR we would call

instead of

svc.add_resource(resource)

we would call

resource.add_to_service(service) if resource.respond_to?(:add_to_service)

This would allow us to create other models that can have the add_to_service method implemented if they allow the resource to be added to service and we would not have to change the REST API.

I can create a separate PR for the Vm.add_to_service which we can call from here.

jntullo · 2017-03-20T19:11:26Z

sounds good @mkanoor

mkanoor · 2017-03-20T20:00:00Z

#14416

https://www.pivotaltracker.com/story/show/142057167 add add_resource to the collection

mkanoor · 2017-03-21T13:43:57Z

app/controllers/api/services_controller.rb

+        resource_type, resource_id = parse_href(resource_ref['href'])
+        resource = resource_search(resource_id, resource_type, collection_class(resource_type))
+        raise "Cannot assign #{resource_type} to #{service_ident(svc)}" unless resource.respond_to? :add_to_service
+        resource.add_to_service(svc)


@jntullo Should we make a validation pass first and make sure all elements in the array respond to :add_to_service before we even attempt to even add a single one. Otherwise we would have added some and raised an error for the first one that doesn't respond to :added_to_service and we can't undo.

@mkanoor hm. I can see the case for that. What do you think about sending back an individual response to each resource instead? so the response will tell you which ones failed and which ones succeeded

@abellotti What is the normal pattern in REST API, when we get an array of objects to work with do we send individual responses and how do we handle errors for one of the elements in the array, e.g. if the resource has been deleted?

@abellotti for other similar actions, ie bulk tagging, we limit it to the collection so that we can return all of the individual results. Should we do something similar here?

normal pattern as is an action result showing success/failure for each action. we do bend the rules on bulk assignment of tags on vms via the assign_tags, in which case we return one per assignment for the same reason you mentioned here above, see app/controllers/api/subcollections/tags.rb's assign_tags method. We could do something similar here.

mkanoor · 2017-03-21T14:39:56Z

app/controllers/api/services_controller.rb

+      svc = resource_search(id, type, collection_class(type))
+      data['resources'].each do |resource_ref|
+        resource_type, resource_id = parse_href(resource_ref['href'])
+        resource = resource_search(resource_id, resource_type, collection_class(resource_type))


@jntullo
Should we validate if the resource can be found, since we carry hrefs if the object is deleted in the meantime we won't have a resource?

abellotti · 2017-03-22T13:58:07Z

with the change from:

svc.add_resource(resource)

to:

resource.add_to_service(service)

I wonder if the API signature should change and be an action on the vm, individual or bulk.

POST /api/vms
{
  "action" : "add_to_service",
  "resources" : [
    { "href" : "http://localhost:3000/api/vms/11", "service" : { "href" : "http://localhost:3000/api/services/101" } },
    { "href" : "http://localhost:3000/api/vms/12", "service" : { "href" : "http://localhost:3000/api/services/101" } },
    { "href" : "http://localhost:3000/api/vms/13", "service" : { "href" : "http://localhost:3000/api/services/101" } },
    ...
  ]
}

on individual vm:

POST /api/vms/11
{
  "action" : "add_to_service",
  "resource" : {
     "service" : { "href" : "http://localhost:3000/api/services/101" }
  }
}

supporting href and id in the service reference.

Thoughts ?

jntullo · 2017-03-22T16:57:17Z

depends on if it will be extended to other resources that can be added or not, otherwise the API will need a new action on that resource.

abellotti · 2017-03-22T18:21:16Z

@mkanoor are there plans to support adding other classes to services in the future ? any ideas on which ?

abellotti · 2017-03-22T20:33:58Z

Ok, had a chat with @mkanoor, future resources could include orchestration_templates, load_balancers, and others. While some CI's are meant for the one service, things like load_balancers could be implemented to support multiple services.

@jntullo let's stick with the add_service action on services, but we do need to change the signature. Problem with using href for the vm above is problematic as it does not allow this action to be implemented for bulk, i.e. on the collection.

Could you update to support the following signature:

Targeting a single service:

POST /api/services/:id
{
  "action" : "add_resource",
  "resources" : [
    { "resource" : { "href" : "http://localhost:3000/api/vms/11" }},
    { "resource" : { "href" : "http://localhost:3000/api/vms/12" }},
    { "resource" : { "href" : "http://localhost:3000/api/vms/13" }},
    ...
  ]
}

Doing bulk add_service on multiple services:

POST /api/services
{
   "action" : "add_resource",
   "resources" : [
      { "href" : "http://localhost:3000/api/services/3", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/4", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/5", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      ...
   ]
}

And enable it for collection (and add collection level tests).

Thanks.

jntullo · 2017-03-23T15:14:17Z

@abellotti I pushed up some changes, but I have some concerns about the use of this type of action on a resource because of the expected response (a single action result). I don't think that we have any other examples of returning multiple results for a single resource, unless I am mistaken, because bulk tagging is limited to a collection.

I played around with returning:

{
  "results": [
      { "success": true, "message": "Added resource..." },
      { "success": false, "message": "error message..." }
  ]
}

and wanted to get your thoughts around that. Another consideration is passing back the individual results within the single action result:

{
   "success": true,
   "message": "Added resources to service...",
   "results": [<individual action results>]
}

abellotti · 2017-03-23T15:58:41Z

what do we do today with assign_tags, even though on a collection, for the one resource it targets, you can specify multiple tags with "tags" : [ ... ] ?

jntullo · 2017-03-23T16:16:14Z

@abellotti we return the result of each individual tagging attempt. I think that it might be good to take that same approach with a descriptive description, ie Added resource <resource info> to Service <service info>

abellotti · 2017-03-24T19:07:22Z

I see the issue with resources on the individual service, I still think we should support the same on both resource and for collection for bulk for consistency. Let's just support a single per request (or bulk) via "resource" : { "href" ... } in the payload, and drop the "resources" for now, we can enhance later.

See my diffs in abellotti@c4ab8d2

For adding multiple resources to the one service, this would simply be a bulk call to /api/services, with the href of the same service specified for each request.

POST /api/services/:id
{
  "action" : "add_resource",
  "resource" : {
    { "resource" : { "href" : "http://localhost:3000/api/vms/11" } }
  }
}

Doing bulk add_service on multiple services, also to add multiple vms to a service:

POST /api/services
{
   "action" : "add_resource",
   "resources" : [
      { "href" : "http://localhost:3000/api/services/3", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/3", "resource" : { "href" : "http://localhot:3000/api/vms/4 }},
      { "href" : "http://localhost:3000/api/services/4", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      { "href" : "http://localhost:3000/api/services/5", "resource" : { "href" : "http://localhost:3000/api/load_balancers/12" }},
      ...
   ]
}

abellotti · 2017-03-24T21:48:35Z

Thanks @jntullo for updating the PR. 😍 will merge when 🍏

more specs

miq-bot · 2017-03-24T21:57:24Z

Checked commits jntullo/manageiq@d755f99~...573c8c1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍪

miq-bot added api enhancement labels Mar 20, 2017

miq-bot assigned abellotti Mar 20, 2017

jntullo commented Mar 20, 2017

View reviewed changes

abellotti reviewed Mar 20, 2017

View reviewed changes

Jillian Tullo added 2 commits March 20, 2017 18:22

add multiple resources to a service

d755f99

https://www.pivotaltracker.com/story/show/142057167 add add_resource to the collection

use add_to_resource

b822842

mkanoor reviewed Mar 21, 2017

View reviewed changes

limit to collection and return individual results

d6008f0

add to resource

573c8c1

more specs

abellotti merged commit be47178 into ManageIQ:master Mar 27, 2017

abellotti added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017

jntullo mentioned this pull request Mar 30, 2017

Remove resources from service #14581

Merged

jntullo deleted the enhancement/multiple_vms_to_service branch November 28, 2017 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add_resource to Service api #14409

add_resource to Service api #14409

jntullo commented Mar 20, 2017 •

edited

Loading

miq-bot commented Mar 20, 2017

jntullo Mar 20, 2017

abellotti Mar 20, 2017

mkanoor Mar 20, 2017

abellotti Mar 20, 2017

mkanoor commented Mar 20, 2017

jntullo commented Mar 20, 2017

mkanoor commented Mar 20, 2017

mkanoor Mar 21, 2017

jntullo Mar 21, 2017

mkanoor Mar 21, 2017

jntullo Mar 22, 2017

abellotti Mar 22, 2017

mkanoor Mar 21, 2017

abellotti commented Mar 22, 2017

jntullo commented Mar 22, 2017

abellotti commented Mar 22, 2017

abellotti commented Mar 22, 2017

jntullo commented Mar 23, 2017

abellotti commented Mar 23, 2017

jntullo commented Mar 23, 2017

abellotti commented Mar 24, 2017

abellotti commented Mar 24, 2017

miq-bot commented Mar 24, 2017

add_resource to Service api #14409

add_resource to Service api #14409

Conversation

jntullo commented Mar 20, 2017 • edited Loading

miq-bot commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor commented Mar 20, 2017

jntullo commented Mar 20, 2017

mkanoor commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Mar 22, 2017

jntullo commented Mar 22, 2017

abellotti commented Mar 22, 2017

abellotti commented Mar 22, 2017

jntullo commented Mar 23, 2017

abellotti commented Mar 23, 2017

jntullo commented Mar 23, 2017

abellotti commented Mar 24, 2017

abellotti commented Mar 24, 2017

miq-bot commented Mar 24, 2017

jntullo commented Mar 20, 2017 •

edited

Loading