[WIP] API for VM Infra Reconfigure field form #1253
Conversation
config/api.yml
Outdated
| @@ -2647,7 +2658,7 @@ | |||
| :identifier: picture_new | |||
| :policies: | |||
| :description: Policies | |||
| :identifier: miq_policy | |||
| :identifier: miq_policy,, | |||
There was a problem hiding this comment.
I think we can support an Array here directly?
There was a problem hiding this comment.
will move these under :vms: => :resource_collections: => :reconfigure:
|
(out of band) update: Going to move these to vms_controller.rb#reconfigure |
|
suggestion: (was out of band) putting here so it doesn't get lost # @param type Vm
def reconfigure_resource(type, id, data = nil)
api_resource(type, id, "Reconfigure") do |vm|
options = {}
if data[:disk_add]
ensure_supports(type, vm, :reconfigure_disk)
end
request = VmReconfigureRequest.make_request(@request_id, options, current_user)
{}
end
end |
jaisejose1123
force-pushed
the
api_vm_infra
branch
from
6d09236
to
b6e56f9
Compare
| end | ||
|
|
||
| @reconfig_values[:cb_memory] = false #if @request_id != new we need to change false | ||
| @reconfig_values[:cb_cpu] = false #if @request_id != new we need to change false |
There was a problem hiding this comment.
#@reconfig_values[:cb_memory] = !!(@req && @req.options[:vm_memory]) # default for checkbox is false for new request
#@reconfig_values[:cb_cpu] = !!(@req && (@req.options[:number_of_sockets] || @req.options[:cores_per_socket])) # default for checkbox is false for new request
| @@ -1,4 +1,5 @@ | |||
| require 'jbuilder' | |||
| require 'byebug' | |||
| @@ -132,6 +133,9 @@ def resource_search(id, type, klass = nil, key_id = nil) | |||
| target = | |||
| if respond_to?("find_#{type}") | |||
| public_send("find_#{type}", id) | |||
| elsif type == "vm_infras" | |||
| vm_infra_reconfigure_form(type,klass,id) | |||
| #find_resource(klass, key_id, id) | |||
There was a problem hiding this comment.
please remove this too if not needed.
| module Api | ||
| class VmInfrasController < BaseProviderController | ||
| end | ||
|
|
||
| end |
| id.split(',').map(&:to_i).each do |id| | ||
| if id.nil? || (key_id == "id" && !id.integer?) | ||
| raise BadRequestError, "Invalid #{klass} #{key_id} #{id || "nil"} specified" | ||
| end | ||
| end | ||
| id.split(/\s*,\s*/) |
There was a problem hiding this comment.
Hm this doesn't seem right, we have an existing method for bulk resource actions already that doesn't require comma separated IDs, https://www.manageiq.org/docs/reference/latest/api/overview/bulk_query.html
| elsif type == "vm_infras" | ||
| vm_infra_reconfigure_form(type,klass,id) |
There was a problem hiding this comment.
We shouldn't add anything action specific into the base renderer class or base resource_search method. This kind of thing should belong in the VmsController
There was a problem hiding this comment.
Use collection_class to specify the specific class.
You can pass in something like the InfraManager::VM to basically act as a VmInfraController
| @@ -149,6 +153,185 @@ def filter_resource(target, type, klass) | |||
| res | |||
| end | |||
|
|
|||
| def vm_infra_reconfigure_form(type, klass, id) | |||
There was a problem hiding this comment.
Again, this should be in the VmsController
| end | ||
|
|
||
| def get_reconfig_info(reconfigure_ids) | ||
| @reconfigureitems = Vm.find(reconfigure_ids).sort_by(&:name) |
There was a problem hiding this comment.
NOTE this is bypassing RBAC checks, don't do direct queries like this that is what resource_search is for.
| @@ -0,0 +1,5 @@ | |||
| module Api | |||
| class VmInfrasController < BaseProviderController | |||
There was a problem hiding this comment.
Why add a new controller instead of using the VmsController ?
config/api.yml
Outdated
| :vm_infras: | ||
| :description: Vm Infras | ||
| :identifier: vm_infra | ||
| :options: | ||
| - :collection | ||
| :verbs: *gpppd | ||
| :klass: Vm | ||
| :resource_actions: | ||
| :get: | ||
| - :name: read | ||
| :identifier: vm_infra_view |
There was a problem hiding this comment.
I was expecting a reconfigure action to be added to the vms section, not an entirely new collection/controller
|
@jaisejose1123 could you push a more recent version of this? I'm curious where it currently stands I marked as wip. please remove the WIP when you are ready for review. (or ping me and I can remove the WIP) |
kbrock
changed the title
jaisejose1123
force-pushed
the
api_vm_infra
branch
5 times, most recently
from
14726a8
to
4f96ba5
Compare
|
Implemented VM infrastructures inside the VM controller. |
|
Implemented VM infrastructures inside the VM controller. Api screen short for multiple id's Api screen short for multiple id's |
|
When calling a single ID instead of multiple IDs, it should provide additional data. Consequently, we implemented APIs with different actions. The below image will show the result while calling single id in manager-ui-classic but while calling multiple id's it shows only limited data |
jaisejose1123
force-pushed
the
api_vm_infra
branch
from
79b4ae9
to
7cb123b
Compare
|
Checked commit jaisejose1123@7cb123b with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint app/controllers/api/base_controller/generic.rb
app/controllers/api/vms_controller.rb
spec/requests/authentications_spec.rb
|
| resource_to_jbuilder(type, type, resource, opts).attributes! | ||
| end | ||
|
|
||
| def reconfigure_calculations(mbsize) |
There was a problem hiding this comment.
please move down to private section
| unless id | ||
| data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ") | ||
| raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}" | ||
| end |
There was a problem hiding this comment.
api_resource will ensure an id is there and will resource_search as well.
There was a problem hiding this comment.
Same as my prior comment, suggest this indentation is fixed and the guard clause is moved to the very top of the method and move new_row assignment after it.
|
I assume at some point this will create a |
| def reconfigure_calculations(mbsize) | ||
| humansize = mbsize | ||
| fmt = "MB" | ||
| if mbsize.to_i > 1024 && (mbsize.to_i % 1024).zero? | ||
| humansize = mbsize.to_i / 1024 | ||
| fmt = "GB" | ||
| end | ||
| return humansize.to_s, fmt | ||
| end |
There was a problem hiding this comment.
This seems like something that we should just say "reconfigure values must be in bytes" as part of the API documentation and then handle that in core rather than trying to figure out if it is mb/gb/etc...
| @@ -489,4 +489,4 @@ def credential_types | |||
| end | |||
| end.deep_stringify_keys | |||
| end | |||
| end | |||
| end | |||
There was a problem hiding this comment.
Trailing whitespace
| end | |
| end |
|
Hey @jaisejose1123 , I had a short discussion with @kbrock and below is the outcome.
@jaisejose1123 , could you please update the PR. |
| @@ -1,4 +1,5 @@ | |||
| module Api | |||
| require 'byebug' | |||
There was a problem hiding this comment.
debug code... please remove after done testing.
| @@ -252,4 +252,4 @@ def validate_id(id, key_id, klass) | |||
| end | |||
| end | |||
| end | |||
| end | |||
| end | |||
There was a problem hiding this comment.
missing newline at the end of the file.
| new_row = { | ||
| :id => id.to_s, | ||
| :long_id => id.to_s, | ||
| :cells => [] | ||
| } | ||
|
|
||
| unless id | ||
| data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ") | ||
| raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}" | ||
| end |
There was a problem hiding this comment.
Prefer a guard clause right at the beginning... Do we need to say what id was invalid?
Fixed indents.
| new_row = { | |
| :id => id.to_s, | |
| :long_id => id.to_s, | |
| :cells => [] | |
| } | |
| unless id | |
| data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ") | |
| raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}" | |
| end | |
| unless id | |
| data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ") | |
| raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}" | |
| end | |
| new_row = { | |
| :id => id.to_s, | |
| :long_id => id.to_s, | |
| :cells => [] | |
| } |
| new_row[:cells] << {:hostname => resource&.host&.name} | ||
| new_row[:cells] << {:num_cpu => resource.num_cpu} | ||
| new_row[:cells] << {:cpu_cores_per_socket => resource.cpu_cores_per_socket} | ||
| new_row[:cells] << {:mem_cpu => "#{(resource.mem_cpu.to_i / 1024.0).to_i}GB"} |
There was a problem hiding this comment.
Some of these check if resource exists. Suggest we do resource&. for each in case resource is nil. Or, should we return if resource.nil? or raise an exception?
| unless id | ||
| data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ") | ||
| raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}" | ||
| end |
There was a problem hiding this comment.
Same as my prior comment, suggest this indentation is fixed and the guard clause is moved to the very top of the method and move new_row assignment after it.
| :hdSize => dsize, | ||
| :hdUnit => dunit, | ||
| :add_remove => '', | ||
| :cb_bootable => disk.bootable} |
There was a problem hiding this comment.
@kbrock do you know if we have a better way to access this data? I don't see virtual columns for most of this data. At the least, we can probably do includes and selects.
|
By utilizing the following GET API, we can gather the VM resources for the reconfigure field form for single id Response by this we can collect all the related information like lans,disks,network_adapters,hardware etc... |
|
For multiple IDs, we can retrieve the data using bulk querying post Apihttp://localhost:3000/api/vms?attributes=name,num_cpu,cpu_cores_per_socket,mem_cpu,vendor,type,host.name,hardware.guest_devices,lans,hardware.disks,hardware.cdroms by passing json
Response includes all the related information like lans,disks,network_adapters,hardware etc... |
|
Most of this data should be retrieved using the standard We do run into problems when accessing |
|
Updated the multiple id's api to Get request Resopnse |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
1 similar comment
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
Related PR
ManageIQ/manageiq#22980
API for VM Infra Reconfigure field form
Before -
/vm_infra/reconfigure_form_fields/new,2983,4671After
Get request for multiple ids
GET /api/vm_infras/4671,2983Get request for single id
GET /api/vm_infras/4671Todo: API response