[api] Add support for Cloud Volume Delete action #15097

AparnaKarve · 2017-05-15T22:44:10Z

Added support for deleting Cloud Volumes via POST.

POST /api/cloud_volumes

{
  "action" : "delete",
  "resources" : { 
    'id': 1,
    'id': 2
  }
}

/cc @abellotti

AparnaKarve · 2017-05-15T23:40:41Z

This would have to be a queued Delete operation.
Setting WIP for now.

@miq-bot add_label wip

imtayadeway · 2017-05-16T15:20:10Z

config/api.yml

    :resource_actions:
      :get:
      - :name: read
        :identifier: cloud_volume
+      :post:
+      - :name: query


This doesn't seem like part of this PR - was this ne cessary?

Yes, you're right, not related to this PR.
I have removed it.

imtayadeway · 2017-05-16T15:20:57Z

config/api.yml

@@ -477,10 +477,17 @@
      :post:
      - :name: query
        :identifier: cloud_volume_show_list
+      - :name: delete


I see there's a test below but I don't think it covers the bulk delete you've added here - can you add one?

The only reason I have the delete here is to avoid the below error.
That's the only purpose it serves.

Failures: 1) Cloud Volumes API can delete cloud volumes through POST Failure/Error: Api::ApiConfig.collections[type][selection][method] .detect { |spec| spec[:name] == action.to_s }[:identifier] NoMethodError: undefined method `[]' for nil:NilClass # ./spec/support/api_helper.rb:113:in `action_identifier' # ./spec/support/api_helper.rb:117:in `collection_action_identifier' # ./spec/requests/api/cloud_volumes_spec.rb:51:in `block (2 levels) in <top (required)>'

I have also tried using

api_basic_authorize action_identifier(:cloud_volumes, :delete, :resource_actions, :post)

but that requires the delete in collection_actions as well.

@imtayadeway What do you think of the above?

@AparnaKarve I think I confused myself :) This is correct - it's the one below that's not covered by a test

The resource_actions delete which is under post is covered by a test.
And that would be the test for it https://github.com/ManageIQ/manageiq/pull/15097/files#diff-3eaddeceef284aecbf30c2f57658d722R52

imtayadeway · 2017-05-16T15:21:59Z

@AparnaKarve can you also add some tests to ensure that RBAC will kick you out if not authorized? Otherwise LGTM!

imtayadeway · 2017-05-16T21:51:58Z

config/api.yml

@@ -468,7 +468,7 @@
    :identifier: cloud_volume
    :options:
    - :collection
-    :verbs: *gp
+    :verbs: *gpppd


I think you only need gpd here....

Since you're adding the 'd' - can you add tests for deleting via DELETE using the collection/member routes, and that RBAC is enforced for both of these too? I have a feeling this may require some more configuration (i.e. adding an explicit entry for delete with the appropriate identifier for each)

AparnaKarve · 2017-05-17T20:42:37Z

@imtayadeway Thanks for all your input so far.
Do you think this is now good to go?

imtayadeway · 2017-05-17T21:20:18Z

spec/requests/api/cloud_volumes_spec.rb

+
+    cloud_volume1 = FactoryGirl.create(:cloud_volume, :ext_management_system => aws, :name => "CloudVolume1")
+
+    api_basic_authorize action_identifier(:cloud_volumes, :delete)


This needs to be action_identifier(:cloud_volumes, :delete, :resource_actions, :delete) - which I think does not exist (yet). Can you add that also confirm that we reject DELETES for users that don't have the appropriate role?

imtayadeway · 2017-05-17T21:21:11Z

spec/requests/api/cloud_volumes_spec.rb

+
+    cloud_volume1 = FactoryGirl.create(:cloud_volume, :ext_management_system => aws, :name => "CloudVolume1")
+
+    api_basic_authorize action_identifier(:cloud_volumes, :delete)


Similarly action_identifier(:cloud_volumes, :delete, :collection_actions, :delete) + test for rejection

imtayadeway · 2017-05-17T21:22:05Z

@AparnaKarve this is looking great, I just have a couple of suggestions above

imtayadeway · 2017-05-17T22:43:35Z

spec/requests/api/cloud_volumes_spec.rb

+    api_basic_authorize collection_action_identifier(:cloud_volumes, :delete, :post)
+
+    expected = {
+      'results' => [


We may not be able to guarantee the order here, so you may want to use a_collection_containing_exactly() over []

imtayadeway

LGTM with just one more comment above. Thanks! 🎈

AparnaKarve · 2017-05-18T16:53:08Z

@imtayadeway I just happened to notice this Coverage increased (+6.5%) to 41.703%, which is great!

Thank you for requesting specs for all possible DELETE cases :)

AparnaKarve · 2017-05-18T17:13:52Z

@miq-bot remove_label wip

AparnaKarve · 2017-05-18T17:16:48Z

@abellotti Please review/merge. Thanks.

abellotti · 2017-05-19T00:48:46Z

config/api.yml

@@ -477,10 +477,21 @@
      :post:
      - :name: query
        :identifier: cloud_volume_show_list
+      - :name: delete
+        :identifier: cloud_volume_delete
+      :delete:


we don't expose DELETE method on the collection itself, so this is not consistent with the others. DELETE targets should be fully identified by the URL itself, even though the standard does not prohibit it, some other vendors just ignore the body. Let's stick to POST for bulk delete unless this is an issue. Thanks.

@abellotti I have removed the DELETE method on the collection and the related specs (6ca849f). Please review. Thanks.

miq-bot · 2017-05-19T17:22:57Z

Checked commits AparnaKarve/manageiq@47a9e32~...6ca849f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

abellotti · 2017-05-22T14:07:23Z

Thanks @AparnaKarve for the enhancement. LGTM!! 👍

AparnaKarve mentioned this pull request May 15, 2017

Redirect delete action to cloud volume controller ManageIQ/manageiq-ui-classic#1331

Merged

miq-bot changed the title ~~[api] Add support for Cloud Volume Delete action~~ [WIP] [api] Add support for Cloud Volume Delete action May 15, 2017

miq-bot added the wip label May 15, 2017

AparnaKarve force-pushed the api_cloud_volume_delete branch from 7a7a9c2 to 7c17a6c Compare May 16, 2017 02:29

imtayadeway reviewed May 16, 2017

View reviewed changes

AparnaKarve force-pushed the api_cloud_volume_delete branch from 7c17a6c to 6906207 Compare May 16, 2017 18:23

chessbyte assigned abellotti May 16, 2017

chessbyte added api enhancement labels May 16, 2017

imtayadeway reviewed May 16, 2017

View reviewed changes

AparnaKarve force-pushed the api_cloud_volume_delete branch from 899b46e to c5ab5ef Compare May 17, 2017 19:06

AparnaKarve added 6 commits May 17, 2017 12:56

Add support for CloudVolume Delete

47a9e32

Add delete_resource method that does queued delete of cloud volumes

25fe755

Spec for multiple cloud volume deletes through POST

813a846

Add RBAC spec that checks for have_http_status(:forbidden)

b65d5fb

PR feedback - removed :post query

c7551bb

Spec for single Cloud Volume delete

c854193

AparnaKarve force-pushed the api_cloud_volume_delete branch 2 times, most recently from 0ef853a to 44b99c0 Compare May 17, 2017 20:14

imtayadeway reviewed May 17, 2017

View reviewed changes

changed verbs to gpd and added specs using run_delete

2006899

AparnaKarve force-pushed the api_cloud_volume_delete branch from 44b99c0 to 2006899 Compare May 17, 2017 22:27

imtayadeway reviewed May 17, 2017

View reviewed changes

imtayadeway approved these changes May 17, 2017

View reviewed changes

Prefer a_collection_containing_exactly() over []

4c1bbf8

imtayadeway approved these changes May 17, 2017

View reviewed changes

AparnaKarve closed this May 18, 2017

AparnaKarve reopened this May 18, 2017

AparnaKarve changed the title ~~[WIP] [api] Add support for Cloud Volume Delete action~~ [api] Add support for Cloud Volume Delete action May 18, 2017

miq-bot removed the wip label May 18, 2017

abellotti reviewed May 19, 2017

View reviewed changes

Removed DELETE method on the collection and the related specs

6ca849f

AparnaKarve force-pushed the api_cloud_volume_delete branch from 6135763 to 6ca849f Compare May 19, 2017 17:17

abellotti merged commit d4e12c9 into ManageIQ:master May 22, 2017

abellotti added this to the Sprint 61 Ending May 22, 2017 milestone May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Add support for Cloud Volume Delete action #15097

[api] Add support for Cloud Volume Delete action #15097

AparnaKarve commented May 15, 2017 •

edited

Loading

AparnaKarve commented May 15, 2017

imtayadeway May 16, 2017

AparnaKarve May 16, 2017

imtayadeway May 16, 2017

AparnaKarve May 16, 2017

AparnaKarve May 16, 2017

imtayadeway May 16, 2017

AparnaKarve May 16, 2017 •

edited

Loading

imtayadeway commented May 16, 2017

imtayadeway May 16, 2017

AparnaKarve commented May 17, 2017

imtayadeway May 17, 2017

imtayadeway May 17, 2017

imtayadeway commented May 17, 2017

imtayadeway May 17, 2017

imtayadeway left a comment

AparnaKarve commented May 18, 2017 •

edited

Loading

AparnaKarve commented May 18, 2017

AparnaKarve commented May 18, 2017

abellotti May 19, 2017

AparnaKarve May 19, 2017

miq-bot commented May 19, 2017

abellotti commented May 22, 2017


		cloud_volume1 = FactoryGirl.create(:cloud_volume, :ext_management_system => aws, :name => "CloudVolume1")

		api_basic_authorize action_identifier(:cloud_volumes, :delete)

[api] Add support for Cloud Volume Delete action #15097

[api] Add support for Cloud Volume Delete action #15097

Conversation

AparnaKarve commented May 15, 2017 • edited Loading

AparnaKarve commented May 15, 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

AparnaKarve May 16, 2017 • edited Loading

Choose a reason for hiding this comment

imtayadeway commented May 16, 2017

Choose a reason for hiding this comment

AparnaKarve commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway commented May 17, 2017

Choose a reason for hiding this comment

imtayadeway left a comment

Choose a reason for hiding this comment

AparnaKarve commented May 18, 2017 • edited Loading

AparnaKarve commented May 18, 2017

AparnaKarve commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 19, 2017

abellotti commented May 22, 2017

AparnaKarve commented May 15, 2017 •

edited

Loading

AparnaKarve May 16, 2017 •

edited

Loading

AparnaKarve commented May 18, 2017 •

edited

Loading