[WIP] Attach detach cloud volume api #1141

MelsHyrule · 2022-03-24T19:15:59Z

Fixes:

Replace angular code manageiq-ui-classic#7603

dependent upon:

Required for upgrading _attach.html.haml and _detach.html.haml

Introduces the attach_resource and detach_resource endpoints for cloud volumes.

kbrock

Just rebase and make the same change to both controller actions:

move the check
add the word " to"
add the resource_search

This looks pretty good to go.

kbrock · 2022-03-31T16:37:59Z

app/controllers/api/cloud_volumes_controller.rb

+      raise BadRequestError, "Must specify a vm_id" if data["vm_id"].nil?
+
+      api_resource(type, id, "Attaching Resource", :supports => :attach_volume) do |cloud_volume|


@agrare What do we want to do in cases like this?

Suggested change

raise BadRequestError, "Must specify a vm_id" if data["vm_id"].nil?

api_resource(type, id, "Attaching Resource", :supports => :attach_volume) do |cloud_volume|

api_resource(type, id, "Attaching Resource to", :supports => :attach_volume) do |cloud_volume|

raise BadRequestError, "Must specify a vm_id" if data["vm_id"].blank?

resource_search(data["vm_id"], :vms)

should we update {:task_id => cloud_volume.detach_volume_queue(User.current_userid, data["vm_id"])} to be instead {:task_id => cloud_volume.detach_volume_queue(User.current_userid, resource_search(data["vm_id"], :vms))} ?

cloud_volume.attach_volume_queue takes an :ems_ref not an :id or Vm object, so you should lookup the VM like @kbrock's suggestion, then pass in vm.ems_ref

app/controllers/api/cloud_volumes_controller.rb

agrare · 2022-03-31T19:29:52Z

spec/requests/cloud_volumes_spec.rb

+      hw = FactoryBot.create(:hardware)
+      vm = FactoryBot.create(:vm_vmware, :hardware => hw)
+      cloud_volume = FactoryBot.create(:cloud_volume_autosde, :ext_management_system => ems)
+      disk = FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)


If you don't need to use disk later on you just need the record you could do

Suggested change

disk = FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)

FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)

And then rubocop won't yell at you

agrare · 2022-04-01T12:27:57Z

app/controllers/api/cloud_volumes_controller.rb

+        raise BadRequestError, "Must specify a vm_id" if data["vm_id"].blank?
+
+        vm = resource_search(data["vm_id"], :vms)
+        {:task_id => cloud_volume.attach_volume_queue(User.current_userid, vm.ems_ref)}


@MelsHyrule we'll also need an optional device parameter here:

Suggested change

{:task_id => cloud_volume.attach_volume_queue(User.current_userid, vm.ems_ref)}

{:task_id => cloud_volume.attach_volume_queue(User.current_userid, vm.ems_ref, data["device"].presence)}

should do the trick

agrare · 2022-04-01T15:15:51Z

spec/requests/cloud_volumes_spec.rb

+      cloud_volume = FactoryBot.create(:cloud_volume_autosde, :ext_management_system => ems)
+
+      api_basic_authorize(action_identifier(:cloud_volumes, :attach, :resource_actions, :post))
+      stub_supports(cloud_volume.class, :attach)


Suggested change

stub_supports(cloud_volume.class, :attach)

stub_supports(cloud_volume.class, :attach_volume)

agrare · 2022-04-01T15:16:00Z

spec/requests/cloud_volumes_spec.rb

+      FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)
+
+      api_basic_authorize(action_identifier(:cloud_volumes, :detach, :resource_actions, :post))
+      stub_supports(cloud_volume.class, :detach)


Suggested change

stub_supports(cloud_volume.class, :detach)

stub_supports(cloud_volume.class, :detach_volume)

agrare · 2022-04-04T15:07:30Z

@kbrock you want to give a second set of 👀

kbrock · 2022-04-07T19:53:24Z

app/controllers/api/cloud_volumes_controller.rb

+      type = @req.collection.to_sym
+      base_klass = collection_class(type)
+
+      ems = resource_search(ems_id, :providers)


ems may be nil

agrare · 2022-04-13T18:41:47Z

app/controllers/api/base_controller/renderer.rb

        type = @req.collection.to_sym
        resource = resource_search(id, type)
        raise BadRequestError, resource.unsupported_reason(:update) unless resource.supports?(:update)

-        render_options(type, :form_schema => resource.params_for_update)
+        if action == "attach"


I don't think overriding the render_update_resource_options method is the way to go here

For example won't we not get here if we are looking for the "attach" action but the resource doesn't support "update" a totally different action? https://github.com/ManageIQ/manageiq-api/pull/1141/files#diff-48fb0a597ebc41e5c8c4fde22100d5577a08a07b84761152f69eaa2f580c4c37R555

Plus "update" is in the name, then we also have it handle other options

agrare · 2022-04-13T18:42:30Z

app/controllers/api/cloud_volumes_controller.rb

@@ -34,7 +34,8 @@ def delete_resource_main_action(_type, cloud_volume, _data)

    def options
      if (id = params["id"])
-        render_update_resource_options(id)
+        action = params["option_action"] || "update"
+        render_update_resource_options(id, action)


Could we send("render_#{action}_resource_options(id)") instead? Then we would localize the methods to the controller that has those actions

miq-bot · 2022-04-14T17:46:19Z

Checked commits MelsHyrule/manageiq-api@d6f3678~...1e867c6 with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint
4 files checked, 1 offense detected

app/controllers/api/base_controller/renderer.rb

❗ - Line 566, Col 75 - Layout/TrailingWhitespace - Trailing whitespace detected.

agrare · 2022-04-18T12:43:55Z

@MelsHyrule I was good with tackling the params_for_attach in a separate PR (ManageIQ/manageiq-ui-classic#8205 (comment)) I think what you had prior to trying to add the OPTIONS call was good if you want to move forward with that and I think @kbrock is tackling a general way of dealing with action options in his other PR

miq-bot · 2022-04-20T18:58:24Z

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

agrare · 2022-04-27T14:50:26Z

@MelsHyrule merged #1147 if you want to build on that

MelsHyrule · 2022-05-06T19:42:26Z

With changes from #1147 this PR is no longer needed, closing now.

MelsHyrule requested a review from bdunne as a code owner March 24, 2022 19:15

miq-bot added the wip label Mar 24, 2022

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch 3 times, most recently from b334134 to c4e0c87 Compare March 31, 2022 16:12

miq-bot added the unmergeable label Mar 31, 2022

kbrock reviewed Mar 31, 2022

View reviewed changes

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch 2 times, most recently from 86f1bfa to 209e14b Compare March 31, 2022 18:33

MelsHyrule requested review from kbrock and agrare March 31, 2022 18:33

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from 5c3968e to f8f0d49 Compare March 31, 2022 18:41

MelsHyrule mentioned this pull request Mar 31, 2022

[WIP] Attach detach cloud volume form ManageIQ/manageiq-ui-classic#8205

Closed

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from f8f0d49 to 6a4e619 Compare March 31, 2022 19:21

agrare reviewed Mar 31, 2022

View reviewed changes

app/controllers/api/cloud_volumes_controller.rb Show resolved Hide resolved

agrare reviewed Mar 31, 2022

View reviewed changes

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch 2 times, most recently from a632f87 to 5a9fc27 Compare March 31, 2022 19:36

MelsHyrule requested a review from agrare March 31, 2022 19:38

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from 72f3796 to 40b2ead Compare March 31, 2022 19:39

agrare removed the unmergeable label Apr 1, 2022

agrare reviewed Apr 1, 2022

View reviewed changes

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from ae95f14 to a73085b Compare April 1, 2022 15:21

MelsHyrule requested a review from agrare April 1, 2022 15:26

agrare approved these changes Apr 4, 2022

View reviewed changes

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch 2 times, most recently from 4c92076 to fd8cef4 Compare April 4, 2022 17:34

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from eb40b49 to f5f6f2f Compare April 4, 2022 17:37

Attach detach cloud volume api

d6f3678

MelsHyrule force-pushed the attach-detach-cloud-volume-api branch from a54725b to d6f3678 Compare April 4, 2022 17:40

testing

aaa61cd

MelsHyrule mentioned this pull request Apr 7, 2022

Add custom schema fields to Amazon attach cloud volume form ManageIQ/manageiq-providers-amazon#756

Merged

kbrock reviewed Apr 7, 2022

View reviewed changes

MelsHyrule added 2 commits April 7, 2022 21:57

still testing

f6797bd

working now

c0c6095

agrare reviewed Apr 13, 2022

View reviewed changes

MelsHyrule added 4 commits April 13, 2022 16:49

send it

6cbc30a

working on spec test

d4f415d

testing stuff

2531fa5

blah

1e867c6

This was referenced Apr 18, 2022

Attach detach cloud volume form (simplified) #1149

Merged

Attach detach cloud volume form conversion to react ManageIQ/manageiq-ui-classic#8230

Merged

miq-bot added the unmergeable label Apr 20, 2022

MelsHyrule closed this May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Attach detach cloud volume api #1141

[WIP] Attach detach cloud volume api #1141

MelsHyrule commented Mar 24, 2022 •

edited by agrare

Loading

kbrock left a comment

kbrock Mar 31, 2022

MelsHyrule Mar 31, 2022

agrare Mar 31, 2022

agrare Mar 31, 2022

agrare Apr 1, 2022

agrare Apr 1, 2022

agrare Apr 1, 2022

agrare commented Apr 4, 2022

kbrock Apr 7, 2022

agrare Apr 13, 2022

agrare Apr 13, 2022

miq-bot commented Apr 14, 2022

agrare commented Apr 18, 2022

miq-bot commented Apr 20, 2022

agrare commented Apr 27, 2022

MelsHyrule commented May 6, 2022

		raise BadRequestError, "Must specify a vm_id" if data["vm_id"].nil?

		api_resource(type, id, "Attaching Resource", :supports => :attach_volume) do \|cloud_volume\|

	disk = FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)
	FactoryBot.create(:disk, :hardware => hw, :backing => cloud_volume)

	{:task_id => cloud_volume.attach_volume_queue(User.current_userid, vm.ems_ref)}
	{:task_id => cloud_volume.attach_volume_queue(User.current_userid, vm.ems_ref, data["device"].presence)}

	stub_supports(cloud_volume.class, :attach)
	stub_supports(cloud_volume.class, :attach_volume)

	stub_supports(cloud_volume.class, :detach)
	stub_supports(cloud_volume.class, :detach_volume)

[WIP] Attach detach cloud volume api #1141

[WIP] Attach detach cloud volume api #1141

Conversation

MelsHyrule commented Mar 24, 2022 • edited by agrare Loading

kbrock left a 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

agrare commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 14, 2022

agrare commented Apr 18, 2022

miq-bot commented Apr 20, 2022

agrare commented Apr 27, 2022

MelsHyrule commented May 6, 2022

MelsHyrule commented Mar 24, 2022 •

edited by agrare

Loading