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