Bulk tag multiple VMs #13581

jntullo · 2017-01-19T13:18:01Z

This enhancement to the API gives the ability to tag multiple VMs with multiple tags with one call. Sample request:

POST /api/vms
{ 
  "action" : "assign_tags",
  "resources" : [
    { "href" : "http://localhost:3000/vms/11", "tags" : [ { "category" : "department", "name" : "finance" } ]} 
    { "href" : "http://localhost:3000/vms/12", "tags" : [ { "category" : "department", "name" : "finance" }]}
  ]
}

It will also accept the ID or HREF of a tag.

Sample response:

{"results"=>
  [{"11"=>
     [{"success"=>true,
       "message"=>"Assigning Tag: category:'department' name:'finance'",
       "href"=>"http://www.example.com/api/vms/",
       "tag_category"=>"department",
       "tag_name"=>"finance"}]},
   {"12"=>
     [{"success"=>true, "message"=>"Assigning Tag: category:'department' name:'finance'", "href"=>"http://www.example.com/api/vms/", "tag_category"=>"department", "tag_name"=>"finance"}]}]}

Each individual VM resource will return an array of results for each tag resource passed to it.

Additional PRs will be added to create bulk unassign and bulk assigning on services.

cc: @imtayadeway
@miq-bot add_label enhancement, api, euwe/no
@miq-bot assign @abellotti

Links

Pivotal tracker ticket

abellotti · 2017-01-19T14:45:05Z

app/controllers/api/subcollections/tags.rb

@@ -85,7 +85,7 @@ def parse_tag(data)

      def parse_tag_from_href(data)
        href = data["href"]
-        tag  = if href && href.match(%r{^.*/tags/#{CID_OR_ID_MATCHER}$})
+        tag  = if href && href.match(%r{^.*/tags/#{ArRegion::CID_OR_ID_MATCHER}$})


why was this needed ?

getting uninitialized constant Api::Subcollections::Tags::CID_OR_ID_MATCHER without it in the tests

wow!! so better code coverage with the tests you've added 😍

Looks like there's a few of these around....perhaps this could be addressed in separate PR so it could be backported if needed? I wonder if it regressed when this became a mixin?

@imtayadeway sounds good, will do!

abellotti · 2017-01-19T14:50:39Z

app/controllers/api/vms_controller.rb

+      { id => tag_results }
+    rescue => err
+      { id => action_result(false, err.to_s) }
+    end


See if this can't be generalized, will be easier to implement assign_tags/unassign_tags to all other collections that support the tags subcollection.

Looking at the api.yml, I see bulk assign_tags and unassign_tags could be implemented to the following collections for completion:

blueprints clusters data_stores groups hosts providers resource_pools service_templates services templates tenants users

so generalizing assign_tags_resource and unassign_tags_resource, maybe as a mixin would be a 👍

@abellotti sounds good to me! will do 👍

abellotti · 2017-01-19T14:51:51Z

config/api.yml

@@ -1890,6 +1890,8 @@
        :identifier: vm_scan
      - :name: delete
        :identifier: vm_delete
+      - :name: assign_tags
+        :identifier: vm_tag


I take it unassign_tags will be a separate PR ?

correct 👍

abellotti · 2017-01-19T14:53:33Z

spec/requests/api/vms_spec.rb

+      }
+      expect(response).to have_http_status(:ok)
+      expect(response.parsed_body).to include(expected)
+    end


I would prefer action results similar to when assigning/unassigning tags on a
resource i.e. vms/72/tags, i.e. even though we can target multiple resources,
it's still an action performed (thus result) per tag.

so instead of:

{ "results": [ { "72": [ { "success": true, "message": "Assigning Tag: category:'department' name:'finance'", "href": "http://localhost:3000/api/vms/", "tag_category": "department", "tag_name": "finance" }, { "success": true, "message": "Assigning Tag: category:'location' name:'ny'", "href": "http://localhost:3000/api/vms/", "tag_category": "location", "tag_name": "ny" } ], "75": [ { "success": true, "message": "Assigning Tag: category:'department' name:'finance'", "href": "http://localhost:3000/api/vms/", "tag_category": "department", "tag_name": "finance" } ] } ] }

I'd rather get:

{ "results": [ { "success": true, "message": "Assigning Tag: category:'department' name:'finance'", "href": "http://localhost:3000/api/vms/72", "tag_category": "department", "tag_name": "finance" }, { "success": true, "message": "Assigning Tag: category:'location' name:'ny'", "href": "http://localhost:3000/api/vms/72", "tag_category": "location", "tag_name": "ny" }, { "success": true, "message": "Assigning Tag: category:'department' name:'finance'", "href": "http://localhost:3000/api/vms/75", "tag_category": "department", "tag_name": "finance" } ] }

jntullo · 2017-01-20T19:40:22Z

app/controllers/api/subcollections/tags.rb

-                 klass = collection_class(:tags)
-                 klass.find(from_cid(href.split('/').last))
-               end
+        href = parse_href(data['href'])


still some work left to do on this PR but I saw some room for refactoring / reuse which would negate the need for #13597. Thoughts on this approach @abellotti @imtayadeway?

LGTM, although I can't tell from the context if we're saying here that this needs to be a tag url. @abellotti ?

this code should only be defining tag if the href is indeed for a tag, so we can't just simply use parse_href. the code can be generalized if this is needed somewhere where where we send in the collection of interest, (i.e. :tags) then that drives the generalization of the old lines 88-91. Not sure where else we do this though.

jntullo · 2017-01-20T19:41:01Z

spec/requests/api/tag_collections_spec.rb

@@ -478,7 +478,7 @@ def expect_resource_has_tags(resource, tag_names)

      run_post("#{blueprints_url(blueprint.id)}/tags",
               :action   => "assign",
-               :catagory => tag1[:category],
+               :category => tag1[:category],


The refactoring above effort also shed light on the typos in these specs

imtayadeway · 2017-01-24T22:01:14Z

app/controllers/api/base_controller/manager.rb

@@ -140,7 +140,7 @@ def update_multiple_collections(is_subcollection, target, type, resources)
          update_one_collection(is_subcollection, target, type, rid, r)
        end
        raise BadRequestError, "No #{type} resources were specified for the #{action} action" if processed == 0
-        {"results" => results}
+        {"results" => results.flatten}


Interesting. How was this failing before?

@imtayadeway it wasn't failing - I believe this is the first action where it is returning an array of results per resource, so the results were being returned in a nested array ([[]]) rather than how they are now, but .flatten fixes that. Does that make sense?

I wonder if the flattening shouldn't be in lower level, maybe assign_tags_resource.

@abellotti it can't be placed there, because it calls assign_tags_resource for each resource. So even though that is flattened, it is still returning a single array, which results in the results being an array of an array

I just double checked my logic just to be sure and am not sure my explanation above made sense, so here is the full results if the flatten is moved up:

{"results"=> [[{"success"=>true, "message"=>"Assigning Tag: category:'department' name:'finance'", "href"=>"http://www.example.com/api/vms/", "tag_category"=>"department", "tag_name"=>"finance", "tag_href"=>"http://www.example.com/api/tags/10000000002666"}], [{"success"=>true, "message"=>"Assigning Tag: category:'cc' name:'001'", "href"=>"http://www.example.com/api/vms/", "tag_category"=>"cc", "tag_name"=>"001", "tag_href"=>"http://www.example.com/api/tags/10000000002670"}]]}

abellotti · 2017-01-27T20:49:23Z

@jntullo #13671 was merged, can you update here to reference via BaseController::CID_OR_ID_MATCHER ? Thanks.

miq-bot · 2017-01-27T20:51:34Z

This pull request is not mergeable. Please rebase and repush.

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

…tagged resource

update spec references

imtayadeway · 2017-01-30T13:30:47Z

spec/requests/api/vms_spec.rb

@@ -89,7 +89,7 @@ def update_raw_power_state(state, *vms)
  context "Vm software subcollection" do
    let(:sw1) { FactoryGirl.create(:guest_application, :vm_or_template_id => vm.id, :name => "Word")  }
    let(:sw2) { FactoryGirl.create(:guest_application, :vm_or_template_id => vm.id, :name => "Excel") }
-    let(:vm_software_url)      { "#{vms_url(vm.id)}/software"    }


Although I approve of this change, it doesn't seem related to the rest of this PR

@imtayadeway sorry - an accidental commit

miq-bot · 2017-01-30T13:41:47Z

Checked commits jntullo/manageiq@ae60628~...aac19cb 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 · 2017-01-30T18:18:10Z

LGTM!!

chessbyte assigned abellotti Jan 19, 2017

chessbyte added api enhancement labels Jan 19, 2017

abellotti reviewed Jan 19, 2017

View reviewed changes

miq-bot added the euwe/no label Jan 19, 2017

jntullo mentioned this pull request Jan 20, 2017

add full reference to CIDR_OR_ID_MATCHER #13597

Closed

jntullo commented Jan 20, 2017

View reviewed changes

imtayadeway reviewed Jan 24, 2017

View reviewed changes

jntullo mentioned this pull request Jan 27, 2017

Fix cid constant namespacing in the API #13671

Merged

miq-bot added the unmergeable label Jan 27, 2017

Jillian Tullo added 4 commits January 30, 2017 07:54

bulk assign tags to vms

ae60628

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

add individual success / failure message for each tag and vm

864fd96

move assign_tags_resource to tags sub collection to be used by any …

ddb01d5

…tagged resource

refactoring to re-use parse_href

73b4e2b

update spec references

miq-bot removed the unmergeable label Jan 30, 2017

imtayadeway reviewed Jan 30, 2017

View reviewed changes

use parse_id to pass in collection name

aac19cb

abellotti added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 30, 2017

abellotti merged commit a8de3d5 into ManageIQ:master Jan 30, 2017

jntullo mentioned this pull request Mar 21, 2017

add_resource to Service api #14409

Merged

jntullo deleted the enhancement/bulk_tag_vms branch November 28, 2017 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bulk tag multiple VMs #13581

Bulk tag multiple VMs #13581

jntullo commented Jan 19, 2017 •

edited by miq-bot

Loading

abellotti Jan 19, 2017

jntullo Jan 19, 2017

abellotti Jan 19, 2017

imtayadeway Jan 20, 2017

jntullo Jan 20, 2017

abellotti Jan 19, 2017

abellotti Jan 19, 2017

jntullo Jan 19, 2017

abellotti Jan 19, 2017

jntullo Jan 20, 2017

abellotti Jan 19, 2017

jntullo Jan 20, 2017

imtayadeway Jan 24, 2017

abellotti Jan 24, 2017

jntullo Jan 20, 2017

imtayadeway Jan 24, 2017

jntullo Jan 25, 2017

abellotti Jan 25, 2017

jntullo Jan 25, 2017

jntullo Jan 26, 2017

abellotti commented Jan 27, 2017

miq-bot commented Jan 27, 2017

imtayadeway Jan 30, 2017

jntullo Jan 30, 2017

miq-bot commented Jan 30, 2017

abellotti commented Jan 30, 2017

Bulk tag multiple VMs #13581

Bulk tag multiple VMs #13581

Conversation

jntullo commented Jan 19, 2017 • edited by miq-bot Loading

Links

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Jan 27, 2017

miq-bot commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jan 30, 2017

abellotti commented Jan 30, 2017

jntullo commented Jan 19, 2017 •

edited by miq-bot

Loading