diff --git a/app/models/cloud_network.rb b/app/models/cloud_network.rb index e501838d3dc..cea9876a9d1 100644 --- a/app/models/cloud_network.rb +++ b/app/models/cloud_network.rb @@ -47,8 +47,7 @@ class CloudNetwork < ApplicationRecord virtual_total :total_vms, :vms def self.class_by_ems(ext_management_system, _external = false) - # TODO: A factory on ExtManagementSystem to return class for each provider - ext_management_system && ext_management_system.class::CloudNetwork + ext_management_system&.class_by_ems(:CloudNetwork) end def self.tenant_id_clause_format(tenant_ids) diff --git a/app/models/cloud_subnet.rb b/app/models/cloud_subnet.rb index 9e97e9a81ea..69d6e57af76 100644 --- a/app/models/cloud_subnet.rb +++ b/app/models/cloud_subnet.rb @@ -51,8 +51,7 @@ def dns_nameservers_show virtual_total :total_vms, :vms, :uses => :vms def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::CloudSubnet + ext_management_system&.class_by_ems(:CloudSubnet) end def delete_cloud_subnet diff --git a/app/models/cloud_tenant.rb b/app/models/cloud_tenant.rb index 5b0dadbcf7c..dd051bb3cca 100644 --- a/app/models/cloud_tenant.rb +++ b/app/models/cloud_tenant.rb @@ -39,14 +39,14 @@ class CloudTenant < ApplicationRecord virtual_total :total_vms, :vms def self.class_by_ems(ext_management_system) - ext_management_system && ext_management_system.class::CloudTenant + ext_management_system&.class_by_ems(:CloudTenant) end def self.create_cloud_tenant(ems_id, options = {}) ext_management_system = ExtManagementSystem.find_by(:id => ems_id) raise ArgumentError, _("ext_management_system cannot be nil") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:CloudTenant) klass.raw_create_cloud_tenant(ext_management_system, options) end @@ -66,7 +66,7 @@ def self.create_cloud_tenant_queue(userid, ext_management_system, options = {}) } queue_opts = { - :class_name => class_by_ems(ext_management_system).name, + :class_name => ext_management_system.class_by_ems(:CloudTenant).name, :method_name => 'create_cloud_tenant', :priority => MiqQueue::HIGH_PRIORITY, :role => 'ems_operations', diff --git a/app/models/cloud_volume.rb b/app/models/cloud_volume.rb index 5711857cdec..305224ba60b 100644 --- a/app/models/cloud_volume.rb +++ b/app/models/cloud_volume.rb @@ -51,9 +51,7 @@ def self.available end def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from OrchesTration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::CloudVolume + ext_management_system&.class_by_ems(:CloudVolume) end def self.my_zone(ems) @@ -93,7 +91,7 @@ def self.create_volume(ems_id, options = {}) ext_management_system = ExtManagementSystem.find(ems_id) raise ArgumentError, _("ext_management_system cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:CloudVolume) klass.raw_create_volume(ext_management_system, options) end diff --git a/app/models/cloud_volume_snapshot.rb b/app/models/cloud_volume_snapshot.rb index 1cf197f30dc..3a11ba43605 100644 --- a/app/models/cloud_volume_snapshot.rb +++ b/app/models/cloud_volume_snapshot.rb @@ -14,7 +14,7 @@ class CloudVolumeSnapshot < ApplicationRecord virtual_total :total_based_volumes, :based_volumes def self.class_by_ems(ext_management_system) - ext_management_system && ext_management_system.class::CloudVolumeSnapshot + ext_management_system&.class_by_ems(:CloudVolumeSnapshot) end def self.my_zone(ems) diff --git a/app/models/cloud_volume_type.rb b/app/models/cloud_volume_type.rb index b716f1bd39f..0073a593f97 100644 --- a/app/models/cloud_volume_type.rb +++ b/app/models/cloud_volume_type.rb @@ -10,7 +10,7 @@ class CloudVolumeType < ApplicationRecord belongs_to :ext_management_system, :foreign_key => :ems_id, :class_name => "ExtManagementSystem" def self.class_by_ems(ext_management_system) - ext_management_system && ext_management_system.class::CloudVolumeType + ext_management_system&.class_by_ems(:CloudVolumeType) end def self.display_name(number = 1) diff --git a/app/models/ext_management_system.rb b/app/models/ext_management_system.rb index 3f7ff919ffb..f7a78d29cea 100644 --- a/app/models/ext_management_system.rb +++ b/app/models/ext_management_system.rb @@ -1033,21 +1033,18 @@ def self.class_for_ems(class_name, inherit = false) nil end - # Takes a child class (e.g.: "NetworkRouter") and returns the EMS specific version of it + # Factory that takes a child class (e.g.: "NetworkRouter") + # and returns the EMS specific version of it + # This can be overridden case by case in EMS specific implementations # # @param [String|Symbol] class_name name of the child class - # @param [Boolean] inherit - return the base class if the child does not have a specific one (default: false - return nil if no child) # @returns The EMS specific version of the class requested - def class_for_ems(class_name, inherit = false) - self.class.class_for_ems(class_name, inherit) - end - - def self.class_for_ems(class_name, inherit = false) - class_name = class_name.name if class_name.kind_of?(Class) - const_get(class_name, inherit) + def self.class_by_ems(class_name) + const_get(class_name, false) rescue NameError nil end + delegate :class_by_ems, :to => :class # @return [Boolean] true if a datastore exists for this type of ems def self.datastore? diff --git a/app/models/flavor.rb b/app/models/flavor.rb index 1fb3e28e8fe..95485382a08 100644 --- a/app/models/flavor.rb +++ b/app/models/flavor.rb @@ -37,7 +37,7 @@ def name_with_details end def self.class_by_ems(ext_management_system) - ext_management_system.class::Flavor + ext_management_system&.class_by_ems(:Flavor) end def self.tenant_joins_clause(scope) @@ -79,8 +79,7 @@ def self.create_flavor(ems_id, options) raise ArgumentError, _("ems cannot be nil") if ems_id.nil? ext_management_system = ExtManagementSystem.find(ems_id) raise ArgumentError, _("ems cannot be found") if ext_management_system.nil? - - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:Flavor) klass.raw_create_flavor(ext_management_system, options) end diff --git a/app/models/floating_ip.rb b/app/models/floating_ip.rb index af8c420b067..bdc65181716 100644 --- a/app/models/floating_ip.rb +++ b/app/models/floating_ip.rb @@ -22,8 +22,7 @@ def self.available end def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::FloatingIp + ext_management_system&.class_by_ems(:FloatingIp) end def self.display_name(number = 1) diff --git a/app/models/host_initiator.rb b/app/models/host_initiator.rb index dc0ada5c680..eb4c42a8340 100644 --- a/app/models/host_initiator.rb +++ b/app/models/host_initiator.rb @@ -23,9 +23,7 @@ def my_zone end def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from Orchestration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::HostInitiator + ext_management_system&.class_by_ems(:HostInitiator) end def refresh_ems @@ -66,7 +64,7 @@ def self.create_host_initiator(ems_id, options = {}) ext_management_system = ExtManagementSystem.find_by(:id => ems_id) raise ArgumentError, _("ext_management_system cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:HostInitiator) klass.raw_create_host_initiator(ext_management_system, options) end diff --git a/app/models/manageiq/providers/cloud_manager/auth_key_pair.rb b/app/models/manageiq/providers/cloud_manager/auth_key_pair.rb index a223e05f656..1c735da1bb1 100644 --- a/app/models/manageiq/providers/cloud_manager/auth_key_pair.rb +++ b/app/models/manageiq/providers/cloud_manager/auth_key_pair.rb @@ -7,7 +7,7 @@ class ManageIQ::Providers::CloudManager::AuthKeyPair < ::Authentication include_concern 'Operations' def self.class_by_ems(ext_management_system) - ext_management_system.class::AuthKeyPair + ext_management_system&.class_by_ems(:AuthKeyPair) end # Create an auth key pair as a queued task and return the task id. The queue @@ -38,7 +38,7 @@ def self.create_key_pair(ems_id, options) ext_management_system = ExtManagementSystem.find(ems_id) raise ArgumentError, _("ems cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:AuthKeyPair) # TODO(maufart): add cloud_tenant to database table? created_key_pair = klass.raw_create_key_pair(ext_management_system, options) klass.create( diff --git a/app/models/manageiq/providers/cloud_manager/template.rb b/app/models/manageiq/providers/cloud_manager/template.rb index 31ced4f4667..65c205b12c0 100644 --- a/app/models/manageiq/providers/cloud_manager/template.rb +++ b/app/models/manageiq/providers/cloud_manager/template.rb @@ -12,7 +12,7 @@ def snapshot? end def self.class_by_ems(ext_management_system) - ext_management_system.class::Template + ext_management_system&.class_by_ems(:Template) end # Create a cloud image as a queued task and return the task id. The queue @@ -51,7 +51,7 @@ def self.create_image(ems_id, options) ext_management_system = ExtManagementSystem.find(ems_id) raise ArgumentError, _("ems cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:Template) klass.raw_create_image(ext_management_system, options) end diff --git a/app/models/network_router.rb b/app/models/network_router.rb index 77120f75ce1..1d74ee3c687 100644 --- a/app/models/network_router.rb +++ b/app/models/network_router.rb @@ -49,8 +49,7 @@ class NetworkRouter < ApplicationRecord virtual_total :total_vms, :vms def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::NetworkRouter + ext_management_system&.class_by_ems(:NetworkRouter) end private diff --git a/app/models/network_service.rb b/app/models/network_service.rb index 12276e2ba11..372e8e998dd 100644 --- a/app/models/network_service.rb +++ b/app/models/network_service.rb @@ -20,8 +20,7 @@ class NetworkService < ApplicationRecord virtual_total :security_policy_rules_count, :security_policy_rules def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::NetworkService + ext_management_system&.class_by_ems(:NetworkService) end def self.display_name(number = 1) diff --git a/app/models/network_service_entry.rb b/app/models/network_service_entry.rb index 064c99b9fb2..440cb7e8af2 100644 --- a/app/models/network_service_entry.rb +++ b/app/models/network_service_entry.rb @@ -12,7 +12,6 @@ class NetworkServiceEntry < ApplicationRecord has_many :network_service_entries, :foreign_key => :ems_id, :dependent => :destroy def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::NetworkServiceEntry + ext_management_system&.class_by_ems(:NetworkServiceEntry) end end diff --git a/app/models/physical_storage.rb b/app/models/physical_storage.rb index dcea9f8cf2f..afa14097595 100644 --- a/app/models/physical_storage.rb +++ b/app/models/physical_storage.rb @@ -104,14 +104,12 @@ def self.create_physical_storage(ems_id, options = {}) ext_management_system = ExtManagementSystem.find_by(:id => ems_id) raise ArgumentError, _("ext_management_system cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:PhysicalStorage) klass.raw_create_physical_storage(ext_management_system, options) end def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from Orchestration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::PhysicalStorage + ext_management_system&.class_by_ems(:PhysicalStorage) end def self.raw_create_physical_storage(_ext_management_system, _options = {}) diff --git a/app/models/security_group.rb b/app/models/security_group.rb index e3d753a396e..5680bd268b0 100644 --- a/app/models/security_group.rb +++ b/app/models/security_group.rb @@ -36,8 +36,7 @@ def self.non_cloud_network end def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::SecurityGroup + ext_management_system&.class_by_ems(:SecurityGroup) end def update_security_group_queue(userid, options = {}) diff --git a/app/models/security_policy.rb b/app/models/security_policy.rb index eb02d7243d3..d3fb7a0049d 100644 --- a/app/models/security_policy.rb +++ b/app/models/security_policy.rb @@ -16,8 +16,7 @@ class SecurityPolicy < ApplicationRecord virtual_total :rules_count, :rules def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::SecurityPolicy + ext_management_system&.class_by_ems(:SecurityPolicy) end def update_security_policy_queue(userid, options = {}) diff --git a/app/models/security_policy_rule.rb b/app/models/security_policy_rule.rb index 5414e63f940..5a0dfeddafc 100644 --- a/app/models/security_policy_rule.rb +++ b/app/models/security_policy_rule.rb @@ -27,8 +27,7 @@ class SecurityPolicyRule < ApplicationRecord virtual_total :network_services_count, :network_services def self.class_by_ems(ext_management_system) - # TODO: use a factory on ExtManagementSystem side to return correct class for each provider - ext_management_system && ext_management_system.class::SecurityPolicyRule + ext_management_system&.class_by_ems(:SecurityPolicyRule) end def update_security_policy_rule_queue(userid, options = {}) diff --git a/app/models/storage_resource.rb b/app/models/storage_resource.rb index 742827385e6..ea05d46f1bf 100644 --- a/app/models/storage_resource.rb +++ b/app/models/storage_resource.rb @@ -15,8 +15,6 @@ class StorageResource < ApplicationRecord acts_as_miq_taggable def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from Orchestration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::StorageResource + ext_management_system&.class_by_ems(:StorageResource) end end diff --git a/app/models/storage_service.rb b/app/models/storage_service.rb index d451dfe3bdf..3662e086389 100644 --- a/app/models/storage_service.rb +++ b/app/models/storage_service.rb @@ -13,8 +13,6 @@ class StorageService < ApplicationRecord acts_as_miq_taggable def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from Orchestration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::StorageService + ext_management_system&.class_by_ems(:StorageService) end end diff --git a/app/models/volume_mapping.rb b/app/models/volume_mapping.rb index ce7696d1584..3c2f6bc55b1 100644 --- a/app/models/volume_mapping.rb +++ b/app/models/volume_mapping.rb @@ -22,9 +22,7 @@ def my_zone end def self.class_by_ems(ext_management_system) - # TODO(lsmola) taken from Orchestration stacks, correct approach should be to have a factory on ExtManagementSystem - # side, that would return correct class for each provider - ext_management_system && ext_management_system.class::VolumeMapping + ext_management_system&.class_by_ems(:VolumeMapping) end def raw_delete_volume_mapping @@ -78,7 +76,7 @@ def self.create_volume_mapping(ems_id, options = {}) ext_management_system = ExtManagementSystem.find_by(:id => ems_id) raise ArgumentError, _("ext_management_system cannot be found") if ext_management_system.nil? - klass = class_by_ems(ext_management_system) + klass = ext_management_system.class_by_ems(:VolumeMapping) klass.raw_create_volume_mapping(ext_management_system, options) end diff --git a/spec/models/ext_management_system_spec.rb b/spec/models/ext_management_system_spec.rb index 4a425c813ed..d552008ea51 100644 --- a/spec/models/ext_management_system_spec.rb +++ b/spec/models/ext_management_system_spec.rb @@ -659,41 +659,27 @@ end end - describe "#class_for_ems" do + describe "#class_by_ems" do it 'returns a concrete target subclass for a concrete EMS' do ems = FactoryBot.create(:ems_openstack) - expect(ems.class.class_for_ems("Vm", true)).to eq(ems.class::Vm) - expect(ems.class_for_ems("Vm", true)).to eq(ems.class::Vm) - expect(ems.class_for_ems("Vm")).to eq(ems.class::Vm) - expect(ems.class_for_ems(:Vm)).to eq(ems.class::Vm) - expect(ems.class_for_ems(::Vm)).to eq(ems.class::Vm) - end - - # this is documenting what NOT to do - # it is showing that the results are sporadic - it 'expects generic target subclass (not a concrete one)' do - ems = FactoryBot.create(:ems_openstack) - expect(ems.class_for_ems(ems.class::Vm, true)).to eq(ems.class::Vm) - expect(ems.class_for_ems(ems.class::Vm)).to be nil + expect(ems.class_by_ems("Vm")).to eq(ems.class::Vm) + expect(ems.class_by_ems(:Vm)).to eq(ems.class::Vm) end it 'returns nil for unknown class' do ems = FactoryBot.create(:ext_management_system) - expect(ems.class_for_ems("RandomSymbol", true)).to be nil - expect(ems.class_for_ems(:RandomSymbol)).to be nil + expect(ems.class_by_ems(:RandomSymbol)).to be nil end it 'returns a concrete target subclass for a concrete EMS subclass' do ems = FactoryBot.create(:ems_openstack_network) - expect(ems.class_for_ems("NetworkRouter", true)).to eq(ems.class::NetworkRouter) - expect(ems.class_for_ems("NetworkRouter")).to eq(ems.class::NetworkRouter) + expect(ems.class_by_ems("NetworkRouter")).to eq(ems.class::NetworkRouter) end it 'returns the base target class when the EMS does not have a subclass' do ems = FactoryBot.create(:ems_infra) - expect(ems.class_for_ems("NetworkRouter", true)).to eq(::NetworkRouter) - expect(ems.class_for_ems("NetworkRouter")).to eq(nil) + expect(ems.class_by_ems("NetworkRouter")).to eq(nil) end end diff --git a/spec/models/flavor_spec.rb b/spec/models/flavor_spec.rb index 1e93fd5622b..a895a82a18c 100644 --- a/spec/models/flavor_spec.rb +++ b/spec/models/flavor_spec.rb @@ -22,7 +22,7 @@ context 'when calling create_flavor method' do it 'should call raw_create_flavor' do flavor_double = class_double('ManageIQ::Providers::Openstack::CloudManager::Flavor') - allow(subject.class).to receive(:class_by_ems).and_return(flavor_double) + allow(ems.class).to receive(:class_by_ems).and_return(flavor_double) expect(flavor_double).to receive(:raw_create_flavor) subject.class.create_flavor(ems.id, {}) end