[PATCH conductor 1/3] BZ 768089: Refactor provider account matching to facilitate stubbing
Scott Seago
sseago at redhat.com
Tue Dec 20 15:31:28 UTC 2011
On 12/20/2011 05:33 AM, Jan Provaznik wrote:
> On 12/20/2011 07:13 AM, Scott Seago wrote:
>> Instance.match was one long 70 line method, which wasn't conducive to
>> stubbing out parts of it for rspec tests.
>> ---
>> src/app/models/instance.rb | 51
>> +++++++-----------------------------
>> src/app/models/provider_account.rb | 31 ++++++++++++++++++++++
>> src/app/util/taskomatic.rb | 11 +++++---
>> 3 files changed, 48 insertions(+), 45 deletions(-)
>>
>> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
>> index 08aa3b6..083c01f 100644
>> --- a/src/app/models/instance.rb
>> +++ b/src/app/models/instance.rb
>> @@ -184,6 +184,15 @@ class Instance< ActiveRecord::Base
>> Aeolus::Image::Warehouse::ImageBuild.find(image_build_uuid) if
>> image_build_uuid
>> end
>>
>> + def build
>> + image_build || (image.nil? ? nil : image.latest_pushed_build)
>> + end
>> +
>> + # defined as class method to facilitate stubbing out for testing
>
> Instance.any_instance(:provider_images_for_match).stub(...) does the
> required trick, so it can be defined as instance method.
>
Ahh, good catch. I missed that option. I'll refactor as instance method.
>> + def self.provider_images_for_match(image, provider)
>
> First passed param is instance, not image, so s/image/instance/ in
> whole method could be less confusing (if this still will be class
> method).
>
This was a simple typo, but with the above it won't matter anymore.
>> + (image.build ? image.build.provider_images : []).select {|pi|
>> pi.provider_name == provider.name}
>
> image.build could be called only once (or 'build' method could cache
> the result), because one 'build' call means fetching of 2 objects from
> warehouse including all params => many requests.
>
Good catch. I'm just moving around what was there before, but I'll
refactor this to not make that extra call.
>> + end
>> +
>> def assembly_xml
>> @assembly_xml ||= AssemblyXML.new(self[:assembly_xml].to_s)
>> end
>> @@ -346,50 +355,10 @@ class Instance< ActiveRecord::Base
>> errors<< I18n.t('instances.errors.image_not_found', :b_uuid=>
>> image_build_uuid, :i_uuid => image_uuid) if image_build.nil? and
>> image.nil?
>> return [[], errors] unless errors.empty?
>>
>> - build = image_build || image.latest_pushed_build
>> - provider_images = build ? build.provider_images : []
>> matched = []
>> pool.pool_family.provider_accounts.each do |account|
>> - unless account.provider.enabled?
>> - errors<< I18n.t('instances.errors.must_be_enabled',
>> :account_name => account.name)
>> - next
>> - end
>> + account.instance_matches(self, matched, errors)
>>
>> - # match_provider_hardware_profile returns a single provider
>> - # hardware_profile that can satisfy the input hardware_profile
>> - hwp =
>> HardwareProfile.match_provider_hardware_profile(account.provider,
>> -
>> hardware_profile)
>> - unless hwp
>> - errors<<
>> I18n.t('instances.errors.hw_profile_match_not_found', :account_name
>> => account.name)
>> - next
>> - end
>> - account_images = provider_images.select {|pi| pi.provider_name
>> == account.provider.name}
>> - if account_images.empty?
>> - errors<<
>> I18n.t('instances.errors.image_not_pushed_to_provider', :account_name
>> => account.name)
>> - next
>> - end
>> - if account.quota.reached?
>> - errors<<
>> I18n.t('instances.errors.provider_account_quota_reached',
>> :account_name => account.name)
>> - next
>> - end
>> - if requires_config_server? and account.config_server.nil?
>> - errors<<
>> I18n.t('instances.errors.no_config_server_available', :account_name
>> => account.name)
>> - next
>> - end
>> - account_images.each do |pi|
>> - if not frontend_realm.nil?
>> - brealms = frontend_realm.realm_backend_targets.select
>> {|brealm_target| brealm_target.target_provider == account.provider}
>> - if brealms.empty?
>> - errors<< I18n.t('instances.errors.realm_not_mapped',
>> :frontend_realm_name => frontend_realm.name)
>> - next
>> - end
>> - brealms.each do |brealm_target|
>> - matched<< Match.new(pool.pool_family, account, hwp, pi,
>> brealm_target.target_realm)
>> - end
>> - else
>> - matched<< Match.new(pool.pool_family, account, hwp, pi, nil)
>> - end
>> - end
>> end
>>
>> [matched, errors]
>> diff --git a/src/app/models/provider_account.rb
>> b/src/app/models/provider_account.rb
>> index 88ac8d4..0ba423b 100644
>> --- a/src/app/models/provider_account.rb
>> +++ b/src/app/models/provider_account.rb
>> @@ -338,6 +338,37 @@ class ProviderAccount< ActiveRecord::Base
>> end
>> end
>>
>> + def instance_matches(instance, matched, errors)
>> + if !provider.enabled?
>> + errors<< I18n.t('instances.errors.must_be_enabled',
>> :account_name => name)
>> + elsif quota.reached?
>> + errors<<
>> I18n.t('instances.errors.provider_account_quota_reached',
>> :account_name => name)
>> + # match_provider_hardware_profile returns a single provider
>> + # hardware_profile that can satisfy the input hardware_profile
>> + elsif !(hwp =
>> HardwareProfile.match_provider_hardware_profile(provider,
>> instance.hardware_profile))
>> + errors<<
>> I18n.t('instances.errors.hw_profile_match_not_found', :account_name
>> => name)
>> + elsif (account_images =
>> Instance.provider_images_for_match(instance,provider)).empty?
>> + errors<<
>> I18n.t('instances.errors.image_not_pushed_to_provider', :account_name
>> => name)
>> + elsif instance.requires_config_server? and config_server.nil?
>> + errors<<
>> I18n.t('instances.errors.no_config_server_available', :account_name
>> => name)
>> + else
>
> I like multiple separate 'ifs' more - a user gets all errors together,
> so for example he doesn't have to spend time with pushing an image to
> a provider because quota is reached and he can't launch an instance
> anyway.
>
Sure -- perhaps I can refactor this too. Again, I was preserving the
previous behavior where the code was exiting the loop on error. I'll
revisit when I make the above fixes.
>> + account_images.each do |pi|
>> + if not instance.frontend_realm.nil?
>> + brealms =
>> instance.frontend_realm.realm_backend_targets.select {|brealm_target|
>> brealm_target.target_provider == provider}
>> + if brealms.empty?
>> + errors<< I18n.t('instances.errors.realm_not_mapped',
>> :frontend_realm_name => instance.frontend_realm.name)
>> + next
>> + end
>> + brealms.each do |brealm_target|
>> + matched<<
>> Instance::Match.new(instance.pool.pool_family, self, hwp, pi,
>> brealm_target.target_realm)
>> + end
>> + else
>> + matched<< Instance::Match.new(instance.pool.pool_family,
>> self, hwp, pi, nil)
>> + end
>> + end
>> + end
>> + end
>> +
>> private
>>
>> def self.apply_search_filter(search)
>> diff --git a/src/app/util/taskomatic.rb b/src/app/util/taskomatic.rb
>> index 9750873..3629706 100644
>> --- a/src/app/util/taskomatic.rb
>> +++ b/src/app/util/taskomatic.rb
>> @@ -32,10 +32,7 @@ module Taskomatic
>>
>> task.state = Task::STATE_RUNNING
>> task.save!
>> -
>> - Rails.logger.info "Task instance create completed with key
>> #{dcloud_instance.id} and state #{dcloud_instance.state}"
>> - task.instance.external_key = dcloud_instance.id
>> - task.instance.state =
>> dcloud_to_instance_state(dcloud_instance.state)
>> + handle_instance_state(task.instance,dcloud_instance)
>> task.instance.save!
>> rescue HttpException => ex
>> task.failure_code = Task::FAILURE_PROVIDER_CONTACT_FAILED
>> @@ -48,6 +45,12 @@ module Taskomatic
>> end
>> end
>>
>> + def self.handle_instance_state(instance, dcloud_instance)
>> + Rails.logger.info "Task instance create completed with key
>> #{dcloud_instance.id} and state #{dcloud_instance.state}"
>> + instance.external_key = dcloud_instance.id
>> + instance.state = dcloud_to_instance_state(dcloud_instance.state)
>> + end
>> +
>> def self.do_action(task, action)
>> task.time_started = Time.now
>>
>
> Hi,
> some minor notes inline.
>
> Jan
Thanks for the review -- minor refactoring is forthcoming.
Scott
More information about the aeolus-devel
mailing list