[PATCH conductor 1/3] BZ 768089: Refactor provider account matching to facilitate stubbing

Jan Provaznik jprovazn at redhat.com
Tue Dec 20 10:33:38 UTC 2011


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.

> +  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).

> +    (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.

> +  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.

> +      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



More information about the aeolus-devel mailing list