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

Use valid api-version string if settings.yml value is invalid #275

Merged
merged 12 commits into from
Nov 7, 2018
61 changes: 44 additions & 17 deletions app/models/manageiq/providers/azure/refresh_helper_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,56 +116,83 @@ def network_routers
end

# Create the necessary service classes and lock down their api-version
# strings using the config/settings.yml from the provider repo. The
# "to_s" call for the version strings puts the date in the format
# that we need, i.e. "YYYY-MM-DD".
# strings using the config/settings.yml from the provider repo.

def cached_resource_provider_service(config)
@cached_resource_provider_service ||= resource_provider_service(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is something that rarely changes, we could probably wrap it to cache_with_timeout block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know about cache_with_timeout but perhaps we can switch it to that if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, for the things that do not change (or not frequently) it's perfect

end

# If the api-version string set in settings.yml is invalid, a warning
# will be issued, and it will default to the most recent valid string.
#
def valid_api_version(config, service, name)
config_api_version = Settings.ems.ems_azure.api_versions[name]

unless cached_resource_provider_service(config).supported?(service.service_name, service.provider)
return config_api_version
end

valid_api_versions = cached_resource_provider_service(config).list_api_versions(service.provider, service.service_name)

if valid_api_versions.include?(config_api_version)
config_api_version
else
valid_version_string = valid_api_versions.first

message = "Invalid api-version setting of '#{config_api_version}' for " \
"#{service.provider}/#{service.service_name} for EMS #{@ems.name}; " \
"using '#{valid_version_string}' instead."

_log.warn(message)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to detect this at the time it's set in the settings? I can show you where to do it...my only concern is if it makes a call out to the provider (can't tell from this code if it does or not).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does make one extra call internally to get that list.

Choose a reason for hiding this comment

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

I would like to see how to do that @Fryguy

valid_version_string
end
end

def availability_set_service(config)
::Azure::Armrest::AvailabilitySetService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.availability_set
service.api_version = valid_api_version(config, service, 'availability_set')
end
end

def ip_address_service(config)
::Azure::Armrest::Network::IpAddressService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.ip_address
service.api_version = valid_api_version(config, service, 'ip_address')
end
end

def load_balancer_service(config)
::Azure::Armrest::Network::LoadBalancerService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.load_balancer
service.api_version = valid_api_version(config, service, 'load_balancer')
end
end

def managed_image_service(config)
::Azure::Armrest::Storage::ImageService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.managed_image
service.api_version = valid_api_version(config, service, 'managed_image')
end
end

def virtual_machine_image_service(config, options = {})
::Azure::Armrest::VirtualMachineImageService.new(config, options).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.managed_image
service.api_version = valid_api_version(config, service, 'managed_image')
end
end

def network_interface_service(config)
::Azure::Armrest::Network::NetworkInterfaceService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.network_interface
service.api_version = valid_api_version(config, service, 'network_interface')
end
end

def network_security_group_service(config)
::Azure::Armrest::Network::NetworkSecurityGroupService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.network_security_group
service.api_version = valid_api_version(config, service, 'network_security_group')
end
end

def resource_group_service(config)
::Azure::Armrest::ResourceGroupService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.resource_group
service.api_version = valid_api_version(config, service, 'resource_group')
end
end

Expand All @@ -177,37 +204,37 @@ def resource_provider_service(config)

def route_table_service(config)
::Azure::Armrest::Network::RouteTableService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.route_table
service.api_version = valid_api_version(config, service, 'route_table')
end
end

def template_deployment_service(config)
::Azure::Armrest::TemplateDeploymentService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.template_deployment
service.api_version = valid_api_version(config, service, 'template_deployment')
end
end

def storage_disk_service(config)
::Azure::Armrest::Storage::DiskService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.storage_disk
service.api_version = valid_api_version(config, service, 'storage_disk')
end
end

def storage_account_service(config)
::Azure::Armrest::StorageAccountService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.storage_account
service.api_version = valid_api_version(config, service, 'storage_account')
end
end

def virtual_machine_service(config)
::Azure::Armrest::VirtualMachineService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.virtual_machine
service.api_version = valid_api_version(config, service, 'virtual_machine')
end
end

def virtual_network_service(config)
::Azure::Armrest::Network::VirtualNetworkService.new(config).tap do |service|
service.api_version = Settings.ems.ems_azure.api_versions.virtual_network
service.api_version = valid_api_version(config, service, 'virtual_network')
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def expected_table_counts
:cloud_subnet => 3,
:disk => 11,
:ext_management_system => 2,
:flavor => 196,
:flavor => 198,
:floating_ip => 9,
:guest_device => 0,
:hardware => 10,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@

before do
allow(template_deployment_service).to receive(:api_version=)
allow(template_deployment_service).to receive(:service_name).and_return('deployments')
allow(template_deployment_service).to receive(:provider).and_return('Microsoft.Resources')
allow(Azure::Armrest::TemplateDeploymentService).to receive(:new).and_return(template_deployment_service)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,11 @@
refresh_with_cassette([child_orchestration_stack_vm_target], "_targeted_scope/orchestration_stack_vm_refresh")
end

it "will refresh orchestration stack with vms" do
refresh_with_cassette([parent_orchestration_stack_target,
child_orchestration_stack_vm_target,
child_orchestration_stack_vm_target2], "_targeted_scope/orchestration_stack_refresh")
end
#it "will refresh orchestration stack with vms" do
# refresh_with_cassette([parent_orchestration_stack_target,
# child_orchestration_stack_vm_target,
# child_orchestration_stack_vm_target2], "_targeted_scope/orchestration_stack_refresh")
#end

it "will refresh orchestration stack followed by LoadBalancer refresh" do
refresh_with_cassette([parent_orchestration_stack_target], "_targeted_scope/orchestration_stack_refresh")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,42 @@
end

let(:virtual_machine_service) { double }
let(:resource_provider_service) { double }
let(:configuration) { double }
let(:virtual_machine_eastus) { Azure::Armrest::VirtualMachine.new(:name => "foo", :location => "eastus") }
let(:virtual_machine_southindia) { Azure::Armrest::VirtualMachine.new(:name => "bar", :location => "SouthIndia") }

context "valid_api_version" do
before do
allow(Azure::Armrest::Configuration).to receive(:new).and_return(configuration)
allow(Azure::Armrest::ResourceProviderService).to receive(:new).and_return(resource_provider_service)
allow(resource_provider_service).to receive(:api_version=).with('2016-09-01').and_return('2016-09-01')
allow(@ems_azure.cached_resource_provider_service(configuration)) { resource_provider_service }
allow(virtual_machine_service).to receive(:service_name).and_return('virtualMachines')
allow(virtual_machine_service).to receive(:provider).and_return('Microsoft.Compute')
@valid_list = ['2018-06-01', '2018-04-01', '2017-12-01', '2017-03-30']
end

it "returns the settings value if valid" do
allow(@ems_azure.cached_resource_provider_service(configuration)).to receive(:supported?).and_return(true)
allow(@ems_azure.cached_resource_provider_service(configuration)).to receive(:list_api_versions).and_return(@valid_list)
expect(@ems_azure.valid_api_version(configuration, virtual_machine_service, :virtual_machine)).to eql('2017-12-01')
end

it "returns a valid value if the settings value is invalid" do
allow(Settings.ems.ems_azure.api_versions).to receive(:[]).with(:virtual_machine).and_return('2018-01-01')
allow(@ems_azure.cached_resource_provider_service(configuration)).to receive(:supported?).and_return(true)
allow(@ems_azure.cached_resource_provider_service(configuration)).to receive(:list_api_versions).and_return(@valid_list)
expect(@ems_azure.valid_api_version(configuration, virtual_machine_service, :virtual_machine)).to eql('2018-06-01')
end

it "returns the settings value if the service is unsupported" do
allow(Settings.ems.ems_azure.api_versions).to receive(:[]).with(:virtual_machine).and_return('2018-01-01')
allow(@ems_azure.cached_resource_provider_service(configuration)).to receive(:supported?).and_return(false)
expect(@ems_azure.valid_api_version(configuration, virtual_machine_service, :virtual_machine)).to eql('2018-01-01')
end
end

context "get_resource_group_ems_ref" do
it "returns the expected value" do
virtual_machine_eastus.subscription_id = "abc123"
Expand Down
Loading