Attach detach cloud volume form (simplified) #1149
Conversation
|
@MelsHyrule Cannot add the following reviewer because they are not recognized: kavyanekkalapu |
|
@MelsHyrule 'kavyanekkalapu' is an invalid assignee, ignoring... |
| if resource.try(:params_for_attach) ## TODO can we make this one line? | ||
| render_options(type, :form_schema => resource.params_for_attach) | ||
| else | ||
| render_options(type, :form_schema => {}) ## {:fields => []} | ||
| end |
There was a problem hiding this comment.
| if resource.try(:params_for_attach) ## TODO can we make this one line? | |
| render_options(type, :form_schema => resource.params_for_attach) | |
| else | |
| render_options(type, :form_schema => {}) ## {:fields => []} | |
| end | |
| render_options(type, :form_schema => resource.try(:params_for_attach) || {}) |
There was a problem hiding this comment.
I think that should get you to a single line as per your TODO.
There was a problem hiding this comment.
Ill try this change in my other PR where I keep render_options https://github.com/ManageIQ/manageiq-api/pull/1141/files#diff-48fb0a597ebc41e5c8c4fde22100d5577a08a07b84761152f69eaa2f580c4c37R565-R569
| action = params["option_action"] || "update" | ||
| send("render_#{action}_resource_options", id) |
There was a problem hiding this comment.
I'm concerned here that someone might pass a strange option_action, which could bypass security in the future. Not sure if it's better to whitelist attach and detach or just have separate calls out to render_attach_resource_options render_update_resource_options.
There was a problem hiding this comment.
This was something that slipped through from the other PR in the split, it will not be merged here so we can look into it more in the other PR https://github.com/ManageIQ/manageiq-api/pull/1141/files#diff-ff1fde5895f3426d678a7f5731cc08c27f3602cda6453a66677502b4731f5173R37-R38
| def render_attach_resource_options(id) | ||
| type = @req.collection.to_sym | ||
| resource = resource_search(id, type) | ||
| raise BadRequestError, resource.unsupported_reason(:attach_volume) unless resource.supports?(:attach_volume) | ||
|
|
||
| if resource.try(:params_for_attach) ## TODO can we make this one line? | ||
| render_options(type, :form_schema => resource.params_for_attach) | ||
| else | ||
| render_options(type, :form_schema => {}) ## {:fields => []} | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Hey @MelsHyrule I thought for this simplified version we were going to drop the OPTIONS / params_for_attach and take care of that in a follow-up
There was a problem hiding this comment.
This function slipped through the cracks when i was simplifying, it's not being used or called so ill remove it now
| action = params["option_action"] || "update" | ||
| send("render_#{action}_resource_options", id) |
There was a problem hiding this comment.
Don't think you're using these options either right now
MelsHyrule
force-pushed
the
attach_detach_api
branch
from
697f324 to
045b3ac
Compare
MelsHyrule
force-pushed
the
attach_detach_api
branch
2 times, most recently
from
e542fd9 to
bb39a1a
Compare
config/api.yml
Outdated
| @@ -801,6 +801,10 @@ | |||
| :identifier: cloud_volume_backup_create | |||
| - :name: restore_backup | |||
| :identifier: cloud_volume_backup_restore | |||
| - :name: attach | |||
There was a problem hiding this comment.
it may make things simpler to call this :attach_volume
I like the name :attach, but since CloudVolume. supports :attach_volume, the consistency may help things.
Not sure if someone else has an opinion here.
huh. Looks like we forgot to add the base case CloudVolume.supports_not :attach_volume
There was a problem hiding this comment.
@kbrock it is in a sub-module (not sure why) https://github.com/ManageIQ/manageiq/blob/master/app/models/cloud_volume/operations.rb#L4-L7
MelsHyrule
force-pushed
the
attach_detach_api
branch
2 times, most recently
from
42903ad to
5017def
Compare
MelsHyrule
force-pushed
the
attach_detach_api
branch
from
ea5c086 to
7e7af8d
Compare
|
Checked commit MelsHyrule@7e7af8d with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint |
|
LGTM, @kbrock ? |
Required for upgrading _attach.html.haml and _detach.html.haml to react as per 7603.
Introduces the attach_resource and detach_resource endpoints for cloud volumes.
This PR is a more simplified version of work done on 1141. Here we have not yet introduced the work done to make
optionsmore generic. That logic needs further testing and for the purposes of this PR falls slightly out of scope.@miq-bot add-reviewer @kbrock
@miq-bot add-reviewer @agrare
@miq-bot add-reviewer @kavyanekkalapu
@miq-bot assign @kavyanekkalapu