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.

Sign up for GitHub

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

Use OpenStruct over MiqHashStruct #17359

Closed
wants to merge 1 commit into from
Closed

Use OpenStruct over MiqHashStruct #17359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions app/models/miq_host_provision_workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,15 @@ def ws_find_matching_ci(allowed_method, keys, match_str, klass)
end

def self.from_ws(*args)
# Move optional arguments into the MiqHashStruct object
# Move optional arguments into the OpenStruct object
prov_args = args[0, 6]
prov_options = MiqHashStruct.new(:values => args[6], :ems_custom_attributes => args[7], :miq_custom_attributes => args[8])
prov_options = OpenStruct.new(:values => args[6], :ems_custom_attributes => args[7], :miq_custom_attributes => args[8])
prov_args << prov_options
MiqHostProvisionWorkflow.from_ws_ver_1_x(*prov_args)
end

def self.from_ws_ver_1_x(version, user, template_fields, vm_fields, requester, tags, options)
options = MiqHashStruct.new if options.nil?
options = OpenStruct.new if options.nil?
_log.warn("Web-service host provisioning starting with interface version <#{version}> by requester <#{user.userid}>")

init_options = {:use_pre_dialog => false, :request_type => request_type(parse_ws_string(template_fields)[:request_type])}
Expand Down
12 changes: 6 additions & 6 deletions app/models/miq_provision_virt_workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,8 +724,8 @@ def self.from_ws(*args)
version = args.first.to_f
return from_ws_ver_1_0(*args) if version == 1.0

# Move optional arguments into the MiqHashStruct object
prov_options = MiqHashStruct.new(
# Move optional arguments into the OpenStruct object
prov_options = OpenStruct.new(
:values => args[6],
:ems_custom_attributes => args[7],
:miq_custom_attributes => args[8],
Expand Down Expand Up @@ -938,7 +938,7 @@ def ws_customize_fields(values, _fields, data)
end

def self.from_ws_ver_1_x(version, user, template_fields, vm_fields, requester, tags, options)
options = MiqHashStruct.new if options.nil?
options = OpenStruct.new if options.nil?
_log.warn("Web-service provisioning starting with interface version <#{version}> by requester <#{user.userid}>")

init_options = {:use_pre_dialog => false, :request_type => request_type(parse_ws_string(template_fields)[:request_type]), :initial_pass => true}
Expand Down Expand Up @@ -1045,11 +1045,11 @@ def create_hash_struct_from_vm_or_template(vm_or_template, options)
:v_total_snapshots => vm_or_template.v_total_snapshots,
:evm_object_class => :Vm}
data_hash[:cloud_tenant] = vm_or_template.cloud_tenant if vm_or_template.respond_to?(:cloud_tenant)
hash_struct = MiqHashStruct.new(data_hash)
hash_struct.operating_system = MiqHashStruct.new(
hash_struct = OpenStruct.new(data_hash)
hash_struct.operating_system = OpenStruct.new(
:product_name => vm_or_template.operating_system.product_name
) if vm_or_template.operating_system
hash_struct.ext_management_system = MiqHashStruct.new(
hash_struct.ext_management_system = OpenStruct.new(
:name => vm_or_template.ext_management_system.name
) if vm_or_template.ext_management_system
if options[:include_datacenter] == true
Expand Down
15 changes: 8 additions & 7 deletions app/models/miq_request_workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require 'enumerator'
require 'miq-hash_struct'
require 'ostruct'

class MiqRequestWorkflow
include Vmdb::Logging
Expand Down Expand Up @@ -113,6 +113,7 @@ def init_from_dialog(init_values)
init_values[field_name] = [val, field_values[:values][val]]
else
field_values[:values].each do |tz|
next unless tz.kind_of?(Array)
if tz[1].to_i_with_method == val.to_i_with_method
# Save [value, description] for timezones array
init_values[field_name] = [val, tz[0]]
Expand Down Expand Up @@ -641,7 +642,7 @@ def tag_symbol
end

def build_ci_hash_struct(ci, props)
nh = MiqHashStruct.new(:id => ci.id, :evm_object_class => ci.class.base_class.name.to_sym)
nh = OpenStruct.new(:id => ci.id, :evm_object_class => ci.class.base_class.name.to_sym)
props.each { |p| nh.send("#{p}=", ci.send(p)) }
nh
end
Expand Down Expand Up @@ -843,7 +844,7 @@ def find_classes_under_ci(item, klass)

def load_ems_node(item, log_header)
@ems_xml_nodes ||= {}
klass_name = if item.kind_of?(MiqHashStruct)
klass_name = if item.kind_of?(OpenStruct)
Object.const_get(item.evm_object_class)
else
item.class.base_class
Expand All @@ -855,7 +856,7 @@ def load_ems_node(item, log_header)

def ems_has_clusters?
found = each_ems_metadata(nil, EmsCluster) { |ci| break(ci) }
return found.evm_object_class == :EmsCluster if found.kind_of?(MiqHashStruct)
return found.evm_object_class == :EmsCluster if found.kind_of?(OpenStruct)
false
end

Expand Down Expand Up @@ -1023,7 +1024,7 @@ def customization_spec_to_hash_struct(ci)

def load_ar_obj(ci)
return load_ar_objs(ci) if ci.kind_of?(Array)
return ci unless ci.kind_of?(MiqHashStruct)
return ci unless ci.kind_of?(OpenStruct)
ci.evm_object_class.to_s.camelize.constantize.find_by(:id => ci.id)
end

Expand Down Expand Up @@ -1243,7 +1244,7 @@ def set_ws_field_value(values, key, data, dialog_name, dlg_fields)
field_values = dlg_field[:values]
_log.info("processing key <#{dialog_name}:#{key}(#{data_type})> with values <#{field_values.inspect}>")
if field_values.present?
result = if field_values.first.kind_of?(MiqHashStruct)
result = if field_values.first.kind_of?(OpenStruct)
found = field_values.detect { |v| v.id == set_value }
[found.id, found.name] if found
elsif data_type == :array_integer
Expand Down Expand Up @@ -1284,7 +1285,7 @@ def set_ws_field_value_by_display_name(values, key, data, dialog_name, dlg_field
field_values = dlg_field[:values]
_log.info("processing key <#{dialog_name}:#{key}(#{data_type})> with values <#{field_values.inspect}>")
if field_values.present?
result = if field_values.first.kind_of?(MiqHashStruct)
result = if field_values.first.kind_of?(OpenStruct)
found = field_values.detect { |v| v.send(obj_key).to_s.downcase == find_value }
[found.id, found.send(obj_key)] if found
else
Expand Down
8 changes: 4 additions & 4 deletions spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@

context "with allowed customization templates" do
it "#allowed_customization_templates" do
expect(workflow.allowed_customization_templates.first).to be_a(MiqHashStruct)
expect(sysprep_workflow.allowed_customization_templates.first).to be_a(MiqHashStruct)
expect(workflow.allowed_customization_templates.first).to be_a(OpenStruct)
expect(sysprep_workflow.allowed_customization_templates.first).to be_a(OpenStruct)
end

it "should retrieve cloud-init templates when cloning" do
Expand All @@ -35,7 +35,7 @@

result = workflow.allowed_customization_templates(options)
customization_template = workflow.instance_variable_get(:@values)[:customization_template_script]
template_hash = result.first.instance_variable_get(:@hash)
template_hash = result.first.instance_variable_get(:@table)
Copy link
Member Author

@NickLaMuro NickLaMuro Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: This change and the one below it are here because the previous version of the spec is grabbing the internal variable inside the MiqHashStruct, and using that to test the resulting values. @hash is the internal hash variable in MiqHashStruct, and @table is the equivalent in OpenStruct.

I personally think it make more sense to avoid the instance_variable_get all together, and just do a send(:attr) instead, but I didn't want to change the tests that I hardly understand in the first place. They were added recent, however, in these two PRs:

Copy link
Member

Choose a reason for hiding this comment

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

You can just call result.first.to_h if all you want is the hash. That works equivalently in MiqHashStruct and OpenStruct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. To be fair, I didn't write this test in the first place, so I was just trying to keep the spec as close to as it was originally. I can change it to the .to_h suggestion if you prefer.


expect(customization_template).to eq cloud_init_template.script
expect(template_hash).to be_a(Hash)
Expand All @@ -53,7 +53,7 @@

result = sysprep_workflow.allowed_customization_templates(options)
customization_template = sysprep_workflow.instance_variable_get(:@values)[:customization_template_script]
template_hash = result.first.instance_variable_get(:@hash)
template_hash = result.first.instance_variable_get(:@table)

expect(customization_template).to eq sysprep_template.script
expect(template_hash).to be_a(Hash)
Expand Down
2 changes: 1 addition & 1 deletion spec/models/miq_provision_virt_workflow_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@
host1 = FactoryGirl.create(:host_vmware, :ems_id => ems.id)
src_vm = FactoryGirl.create(:vm_vmware, :host => host1, :ems_id => ems.id)
allow(workflow).to receive(:source_vm_rbac_filter).and_return([src_vm])
expect(workflow.ws_find_template_or_vm("", "VMWARE", "asdf-adsf", "asdfadfasdf")).to be_a(MiqHashStruct)
expect(workflow.ws_find_template_or_vm("", "VMWARE", "asdf-adsf", "asdfadfasdf")).to be_a(OpenStruct)
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/models/miq_request_workflow_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@
let(:storage) { FactoryGirl.create(:storage) }

it 'filters out storage_clusters not in same ems' do
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => MiqHashStruct.new(:id => ems.id))
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => OpenStruct.new(:id => ems.id))
storage_cluster1 = FactoryGirl.create(:storage_cluster, :name => 'test_storage_cluster1', :ems_id => ems.id)
storage_cluster2 = FactoryGirl.create(:storage_cluster, :name => 'test_storage_cluster2', :ems_id => ems.id + 1)
storage_cluster1.add_child(storage)
Expand Down