9:04 a.m.
New subject: [PATCH aggregator 1/2] Added check for Account on Provider, when building for provider type
From: Jan Provaznik <jprovazn(a)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 | 25 +++++++++----------------
src/app/models/image.rb | 17 +++++++++++------
src/app/models/provider.rb | 8 ++++++++
src/app/views/builds/new.haml | 6 +++---
src/config/image_descriptor_targets.yml | 2 +-
src/spec/factories/replicated_image.rb | 8 +++++++-
src/spec/models/image_spec.rb | 16 ++++++++++++++++
src/spec/models/template_spec.rb | 9 +++------
8 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/src/app/controllers/builds_controller.rb
b/src/app/controllers/builds_controller.rb
index 820f28b..4a1bdc2 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'
@@ -36,28 +36,17 @@ class BuildsController < ApplicationController
@tpl.upload_template unless @tpl.uploaded
errors = {}
+ warnings = []
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 ImageExistsError
+ warnings << $!.message
rescue
errors[target] = $!.message
end
end
+ flash[:warning] = 'Warning: ' + warnings.join unless warnings.empty?
if errors.empty?
redirect_to builds_path
else
@@ -87,4 +76,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..5ca3fd4 100644
--- a/src/app/models/image.rb
+++ b/src/app/models/image.rb
@@ -19,6 +19,8 @@
# Filters added to this controller apply to all controllers in the application.
# Likewise, all the methods added will be available for all controllers.
+class ImageExistsError < Exception;end
+
class Image < ActiveRecord::Base
include SearchFilter
@@ -60,22 +62,25 @@ 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})
- # TODO: we currently silently ignore requests for building image which is
- # already built (or is building now)
- return img
+ if Image.find_by_template_id_and_target(template.id, target)
+ raise ImageExistsError, "An attempted build of this template for the target
'#{target}' already exists"
+ end
+
+ unless provider = Provider.find_by_target_with_account(target)
+ raise "There is no provider for '#{target}' type with valid
account."
end
+ img = nil
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..2cb159e 100644
--- a/src/app/views/builds/new.haml
+++ b/src/app/views/builds/new.haml
@@ -1,4 +1,4 @@
-%h2 BUILD REQUEST
+%h2 Build Request
- form_tag :action => 'create' do
= hidden_field_tag :template_id, @tpl.id
%h3 Deployment Definition
@@ -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..04c8171 100644
--- a/src/spec/models/image_spec.rb
+++ b/src/spec/models/image_spec.rb
@@ -34,4 +34,20 @@ 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!
+ lambda {Image.build(old.template, old.target)}.should raise_error(ImageExistsError)
+ 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
--
1.7.2.3
9:38 a.m.
New subject: [PATCH aggregator 1/2] Added check for Account on Provider, when building for provider type
Ack to this series
----- Original Message -----
From: jprovazn(a)redhat.com
To: deltacloud-devel(a)lists.fedorahosted.org
Sent: Tuesday, December 7, 2010 3:04:10 PM GMT +00:00 GMT Britain, Ireland, Portugal
Subject: [deltacloud-devel] [PATCH aggregator 1/2] Added check for Account on Provider,
when building for provider type
From: Jan Provaznik <jprovazn(a)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 | 25 +++++++++----------------
src/app/models/image.rb | 17 +++++++++++------
src/app/models/provider.rb | 8 ++++++++
src/app/views/builds/new.haml | 6 +++---
src/config/image_descriptor_targets.yml | 2 +-
src/spec/factories/replicated_image.rb | 8 +++++++-
src/spec/models/image_spec.rb | 16 ++++++++++++++++
src/spec/models/template_spec.rb | 9 +++------
8 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/src/app/controllers/builds_controller.rb
b/src/app/controllers/builds_controller.rb
index 820f28b..4a1bdc2 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'
@@ -36,28 +36,17 @@ class BuildsController < ApplicationController
@tpl.upload_template unless @tpl.uploaded
errors = {}
+ warnings = []
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 ImageExistsError
+ warnings << $!.message
rescue
errors[target] = $!.message
end
end
+ flash[:warning] = 'Warning: ' + warnings.join unless warnings.empty?
if errors.empty?
redirect_to builds_path
else
@@ -87,4 +76,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..5ca3fd4 100644
--- a/src/app/models/image.rb
+++ b/src/app/models/image.rb
@@ -19,6 +19,8 @@
# Filters added to this controller apply to all controllers in the application.
# Likewise, all the methods added will be available for all controllers.
+class ImageExistsError < Exception;end
+
class Image < ActiveRecord::Base
include SearchFilter
@@ -60,22 +62,25 @@ 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})
- # TODO: we currently silently ignore requests for building image which is
- # already built (or is building now)
- return img
+ if Image.find_by_template_id_and_target(template.id, target)
+ raise ImageExistsError, "An attempted build of this template for the target
'#{target}' already exists"
+ end
+
+ unless provider = Provider.find_by_target_with_account(target)
+ raise "There is no provider for '#{target}' type with valid
account."
end
+ img = nil
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..2cb159e 100644
--- a/src/app/views/builds/new.haml
+++ b/src/app/views/builds/new.haml
@@ -1,4 +1,4 @@
-%h2 BUILD REQUEST
+%h2 Build Request
- form_tag :action => 'create' do
= hidden_field_tag :template_id, @tpl.id
%h3 Deployment Definition
@@ -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..04c8171 100644
--- a/src/spec/models/image_spec.rb
+++ b/src/spec/models/image_spec.rb
@@ -34,4 +34,20 @@ 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!
+ lambda {Image.build(old.template, old.target)}.should raise_error(ImageExistsError)
+ 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
--
1.7.2.3
_______________________________________________
deltacloud-devel mailing list
deltacloud-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
9:04 a.m.
New subject: [PATCH aggregator 2/2] Template build cucumber tests
From: Jan Provaznik <jprovazn(a)redhat.com>
---
src/features/step_definitions/template_steps.rb | 13 ++++++++++
src/features/support/custom.rb | 4 +++
src/features/support/paths.rb | 3 ++
src/features/template.feature | 28 +++++++++++++++++++++++
4 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/src/features/step_definitions/template_steps.rb
b/src/features/step_definitions/template_steps.rb
index 894bf0a..9d82f3a 100644
--- a/src/features/step_definitions/template_steps.rb
+++ b/src/features/step_definitions/template_steps.rb
@@ -75,3 +75,16 @@ Given /^there is a "([^"]*)" build$/ do |arg1|
image = Factory.build(:image, :template => template)
image.save!
end
+
+When /^I choose this template$/ do
+ choose("ids__#{(a)template.id}")
+end
+
+Given /^there is ec2 cloud account$/ do
+ account = Factory.build(:ec2_cloud_account)
+ account.save!
+end
+
+Given /^there is ec2 build for this template$/ do
+ Image.build(@template, 'ec2')
+end
diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb
index b5440ae..0054a43 100644
--- a/src/features/support/custom.rb
+++ b/src/features/support/custom.rb
@@ -31,6 +31,10 @@ CloudAccount.class_eval do
alias :generate_cloud_account_key_original :generate_cloud_account_key
+ def validate_credentials
+ true
+ end
+
def generate_cloud_account_key
end
diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb
index 1c73ffa..d55f1f3 100644
--- a/src/features/support/paths.rb
+++ b/src/features/support/paths.rb
@@ -68,6 +68,9 @@ module NavigationHelpers
when /the new template page/
url_for new_template_path
+ when /the new template build page/
+ url_for new_build_path
+
when /the template builds page/
url_for builds_path
diff --git a/src/features/template.feature b/src/features/template.feature
index 4ce0151..e4cf867 100644
--- a/src/features/template.feature
+++ b/src/features/template.feature
@@ -87,3 +87,31 @@ Feature: Manage Templates
Then I should see an input "admin-tools"
# test that we see a collection (collections are loaded by default)
And I should see an input "deltacloud"
+
+ Scenario: Build template
+ Given there is a "mock1" template
+ And there is ec2 cloud account
+ And I am on the templates page
+ When I choose this template
+ And I press "Build"
+ Then I should be on the new template build page
+ And I should see "Build Request"
+ When I check "ec2"
+ And I press "Submit to Build"
+ Then I should be on the template builds page
+ And I should see "mock1"
+
+ Scenario: Build template which is already built
+ Given there is a "mock1" template
+ And there is ec2 cloud account
+ And I am on the templates page
+ And there is ec2 build for this template
+ When I choose this template
+ And I press "Build"
+ Then I should be on the new template build page
+ And I should see "Build Request"
+ When I check "ec2"
+ And I press "Submit to Build"
+ Then I should be on the template builds page
+ And I should see "mock1"
+ And I should see "An attempted build of this template for the target
'ec2' already exists"
--
1.7.2.3
4925
days inactive
4925
days old
deltacloud-devel@lists.fedorahosted.org
3 comments
2 participants
participants (2)
-
Jan Provazník -
Martyn Taylor