Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] API for VM Infra Reconfigure field form #1253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/base_controller/generic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -252,4 +252,4 @@ def validate_id(id, key_id, klass)
end
end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline at the end of the file.

93 changes: 93 additions & 0 deletions app/controllers/api/vms_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module Api
require 'byebug'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug code... please remove after done testing.

class VmsController < BaseProviderController
extend Api::Mixins::CentralAdmin
include Api::Mixins::PolicySimulation
Expand Down Expand Up @@ -53,6 +54,98 @@ def edit_resource(type, id, data)
def delete_resource(type, id, _data = nil)
enqueue_ems_action(type, id, "Deleting", :method_name => "destroy")
end

def vm_reconfigure_resource(type, id, data)

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
Comment on lines +60 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer a guard clause right at the beginning... Do we need to say what id was invalid?
Fixed indents.

Suggested change
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 => []
}

resource = resource_search(id, type)
new_row = vm_data_collection(new_row,resource)

opts = {
:name => type.to_s,
:is_subcollection => false,
:expand_resources => true,
:expand_actions => true,
:expand_custom_actions => true
}
resource_to_jbuilder(type, type, resource, opts).attributes!
end

def vm_data_collection(new_row,resource)
new_row[:tree_id] = TreeBuilder.build_node_id(resource) if resource
new_row[:cells] << {:is_checkbox => true}
new_row[:cells] << {:name => resource.name}
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"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

new_row
end

def vm_reconfigure_single_resource(type, id, data)
new_row = {
:id => id.to_s,
:long_id => id.to_s,
:cells => [],
:network_adapters => [],
:disks => []
}

unless id
data_spec = data.collect { |key, val| "#{key}=#{val}" }.join(", ")
raise NotFoundError, "Invalid #{type} resource specified - #{data_spec}"
end
Comment on lines +103 to +106
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is off here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api_resource will ensure an id is there and will resource_search as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resource = resource_search(id, type)

new_row = vm_data_collection(new_row,resource)

resource.hardware.guest_devices.order(:device_name => 'asc').each do |guest_device|
lan = Lan.find_by(:id => guest_device.lan_id)
new_row[:network_adapters] << {:name => guest_device.device_name, :vlan => lan.name, :mac => guest_device.address, :add_remove => ''} unless lan.nil?
end

resource.hardware.disks.order(:filename).each do |disk|
next if disk.device_type != 'disk'
dsize, dunit = reconfigure_calculations(disk.size / (1024 * 1024))
new_row[:disks] << {:hdFilename => disk.filename,
:hdType => disk.disk_type,
:hdMode => disk.mode,
:hdSize => dsize,
:hdUnit => dunit,
:add_remove => '',
:cb_bootable => disk.bootable}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.



end

opts = {
:name => type.to_s,
:is_subcollection => false,
:expand_resources => true,
:expand_actions => true,
:expand_custom_actions => true
}
resource_to_jbuilder(type, type, resource, opts).attributes!
end

def reconfigure_calculations(mbsize)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move down to private section

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
Comment on lines +140 to +148
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


def set_owner_resource(type, id = nil, data = nil)
api_resource(type, id, "Setting owner of") do |vm|
Expand Down
4 changes: 4 additions & 0 deletions config/api.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4659,6 +4659,10 @@
:identifier:
- vm_show_list
- sui_vm_details_view
- :name: vm_reconfigure
:identifier: vm_infras
- :name: vm_reconfigure_single
:identifier: vm_infras_single
- :name: edit
:identifier: vm_edit
- :name: add_event
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/authentications_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -489,4 +489,4 @@ def credential_types
end
end.deep_stringify_keys
end
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace

Suggested change
end
end

Loading