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