[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