Two comments on this patch.
Firstly, I think the patch needs to be renamed to something along the lines of:
"Added check for Account on Provider, when building for provider type"
Also,
1 small issue:
When I try to build an image, for a provider with no CloudAccount. I do not see an error message, it just takes me to the builds screen and nothing happens. (Tested with Mock Provider)
Could you please add an error for this, and a test to go with it
Thanks Jan
Martyn ----- Original Message ----- From: jprovazn@redhat.com To: deltacloud-devel@lists.fedorahosted.org Sent: Monday, November 29, 2010 2:51:16 PM GMT +00:00 GMT Britain, Ireland, Portugal Subject: [deltacloud-devel] [PATCH aggregator 2/3] Multiple providers support when building template
From: Jan Provaznik jprovazn@redhat.com
When user builds template, CE chooses first provider of specified build type which has at least one cloud account. --- src/app/controllers/builds_controller.rb | 21 +++++---------------- src/app/models/image.rb | 10 +++++++--- src/app/models/provider.rb | 8 ++++++++ src/app/views/builds/new.haml | 4 ++-- src/config/image_descriptor_targets.yml | 2 +- src/spec/factories/replicated_image.rb | 8 +++++++- src/spec/models/image_spec.rb | 17 +++++++++++++++++ src/spec/models/template_spec.rb | 9 +++------ 8 files changed, 50 insertions(+), 29 deletions(-)
diff --git a/src/app/controllers/builds_controller.rb b/src/app/controllers/builds_controller.rb index 820f28b..6ffdd98 100644 --- a/src/app/controllers/builds_controller.rb +++ b/src/app/controllers/builds_controller.rb @@ -1,5 +1,5 @@ class BuildsController < ApplicationController - before_filter :require_user + before_filter [:require_user, :check_permission]
def section_id 'build' @@ -37,21 +37,6 @@ class BuildsController < ApplicationController @tpl.upload_template unless @tpl.uploaded errors = {} params[:targets].each do |target| - # FIXME: for beta release we check explicitly that provider and provider - # account exists - unless provider = Provider.find_by_cloud_type(target) - flash_error('Error while trying to build image', - 'no provider account' => "There is no provider of '#{target}' type, \ -you can add provider on <a href="#{url_for :controller => 'provider'}">the providers page.</a>") - render :action => 'new' and return - end - if provider.cloud_accounts.empty? - flash_error('Error while trying to build image', - 'no provider account' => "There is no provider account for '#{target}' \ -provider, you can add account on <a href="#{url_for :controller => 'provider', \ -:action => 'accounts', :id => provider.id}">the provider accounts page</a>") - render :action => 'new' and return - end begin Image.build(@tpl, target) rescue @@ -87,4 +72,8 @@ provider, you can add account on <a href="#{url_for :controller => 'provider', flash.now[:error][:failures] ||= {} flash.now[:error][:failures].merge!(errs) end + + def check_permission + require_privilege(Privilege::IMAGE_MODIFY) + end end diff --git a/src/app/models/image.rb b/src/app/models/image.rb index dfe8a2a..1619289 100644 --- a/src/app/models/image.rb +++ b/src/app/models/image.rb @@ -60,22 +60,26 @@ class Image < ActiveRecord::Base def self.build(template, target) # FIXME: This will need to be enhanced to handle multiple # providers of same type, only one is supported right now - if img = Image.find_by_template_id(template.id, :conditions => {:target => target}) + if img = Image.find_by_template_id_and_target(template.id, target) # TODO: we currently silently ignore requests for building image which is # already built (or is building now) return img end
+ unless provider = Provider.find_by_target_with_account(target) + raise "There is no provider for '#{target}' type with valid account." + end + Image.transaction do img = Image.create!( - :name => "#{template.xml.name}/#{target}", + :name => "#{template.name}/#{target}", :target => target, :template_id => template.id, :status => Image::STATE_QUEUED ) ReplicatedImage.create!( :image_id => img.id, - :provider_id => Provider.find_by_cloud_type(target) + :provider_id => provider ) end return img diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb index cff34c2..6a195b0 100644 --- a/src/app/models/provider.rb +++ b/src/app/models/provider.rb @@ -111,6 +111,14 @@ class Provider < ActiveRecord::Base cloud_accounts.collect {|account| account.pools}.flatten.uniq end
+ # returns first provider of cloud_type which has at least one cloud account + def self.find_by_target_with_account(cloud_type) + Provider.all(:conditions => {:cloud_type => cloud_type}).each do |p| + return p unless p.cloud_accounts.empty? + end + nil + end + # TODO: implement or remove - this is meant to contain a hash of # supported cloud_types to use in populating form, though if we # infer that field, we don't need this. diff --git a/src/app/views/builds/new.haml b/src/app/views/builds/new.haml index 4cddb6f..06523a9 100644 --- a/src/app/views/builds/new.haml +++ b/src/app/views/builds/new.haml @@ -48,8 +48,8 @@ %ul.block - @all_targets.each do |target_id, target| %li - = check_box_tag 'targets[]', target_id, false - = label_tag 'targets[]', target['name'] + = check_box_tag 'targets[]', target_id, false, :id => target_id + = label_tag target_id, target['name']
= submit_tag "Submit to Build", :name => "build" = submit_tag "Cancel", :name => "cancel" diff --git a/src/config/image_descriptor_targets.yml b/src/config/image_descriptor_targets.yml index e711534..771ddfc 100644 --- a/src/config/image_descriptor_targets.yml +++ b/src/config/image_descriptor_targets.yml @@ -15,4 +15,4 @@ ec2:
# For development, just uncomment this target to do fake image builds #mock: -# name: Mock \ No newline at end of file +# name: Mock diff --git a/src/spec/factories/replicated_image.rb b/src/spec/factories/replicated_image.rb index f989380..fa3f662 100644 --- a/src/spec/factories/replicated_image.rb +++ b/src/spec/factories/replicated_image.rb @@ -2,4 +2,10 @@ Factory.define :replicated_image do |ri| ri.association :image ri.association :provider ri.sequence(:provider_image_key) { |n| "provider_image_key#(n)" } -end \ No newline at end of file + ri.uploaded true + ri.registered true +end + +Factory.define :mock_replicated_image, :parent => :replicated_image do |i| + i.provider { |p| p.association(:mock_provider) } +end diff --git a/src/spec/models/image_spec.rb b/src/spec/models/image_spec.rb index 030e792..b1b42a8 100644 --- a/src/spec/models/image_spec.rb +++ b/src/spec/models/image_spec.rb @@ -34,4 +34,21 @@ describe Image do i.template_id = 1 i.should be_valid end + + it "should not build image if image already exists for specified target" do + old = Factory.build(:image) + old.save! + new = Image.build(old.template, old.target) + old.id.should == new.id + end + + it "should build image if there is provider and cloud account for specified target" do + acc = Factory.build(:mock_cloud_account) + acc.save! + tpl = Factory.build(:template) + tpl.save! + img = Image.build(tpl, 'mock') + Image.find(img).should == img + ReplicatedImage.find_by_image_id(img.id).should_not be_nil + end end diff --git a/src/spec/models/template_spec.rb b/src/spec/models/template_spec.rb index 406febd..94de1df 100644 --- a/src/spec/models/template_spec.rb +++ b/src/spec/models/template_spec.rb @@ -9,12 +9,9 @@ describe Template do end
it "should return list of providers who provides images built from this template" do - tpl = Factory.build(:template) - img = Factory.build(:image, :template_id => tpl) - provider = Factory.build(:mock_provider) - rimg = ReplicatedImage.new(:provider_id => provider, :image_id => img) - rimg.save - tpl.providers.size.should eql(1) + rimg = Factory.build(:mock_replicated_image) + rimg.save! + rimg.image.template.providers.size.should eql(1) end
it "should not be valid if template name is too long" do
deltacloud-devel@lists.fedorahosted.org