Signed-off-by: Scott Seago sseago@redhat.com --- .../controllers/image_factory/builds_controller.rb | 36 ++++++++++++++++++++ src/app/models/image.rb | 11 ++++++ src/app/models/provider_image.rb | 6 +++ src/app/views/image_factory/templates/_builds.haml | 12 +++++- src/config/routes.rb | 2 +- src/spec/controllers/builds_controller_spec.rb | 14 ++++++++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/app/views/image_factory/builds/index.haml
diff --git a/src/app/controllers/image_factory/builds_controller.rb b/src/app/controllers/image_factory/builds_controller.rb index b6efe81..a9901e5 100644 --- a/src/app/controllers/image_factory/builds_controller.rb +++ b/src/app/controllers/image_factory/builds_controller.rb @@ -1,6 +1,9 @@ class ImageFactory::BuildsController < ApplicationController before_filter [:require_user], :except => [:update_status]
+ def index + end + def create @tpl = Template.find(params[:template_id]) check_permission @@ -43,6 +46,39 @@ class ImageFactory::BuildsController < ApplicationController # FIXME: is @tpl defined here? do we need check_permission here? end
+ def retry + @tpl = Template.find(params[:template_id]) + check_permission + begin + if params[:image_id] + image = Image.find(params[:image_id]) + if image + errors = {} + warnings = [] + image.rebuild! + flash[:notice] = "Queued rebuild for image #{image.name}" + else + flash[:warning] = "could not find image #{image_id}" + end + elsif params[:provider_image_id] + provider_image = ProviderImage.find(params[:provider_image_id]) + if provider_image + provider_image.retry_upload! + flash[:notice] = "Queued upload for provider image #{provider_image.name}" + else + flash[:warning] = "could not find provider image #{provider_image_id}" + end + else + flash[:warning] = 'You need to specify either a provider or a provider image' + end + rescue + flash[:warning] = $!.message + logger.error $!.message + logger.error $!.backtrace.join("\n ") + end + redirect_to image_factory_template_path(@tpl, :details_tab => 'builds') + end + def update_status image = Image.find_by_uuid(params[:uuid]) image = ProviderImage.find_by_uuid(params[:uuid]) unless image diff --git a/src/app/models/image.rb b/src/app/models/image.rb index f15f2da..f0434b8 100644 --- a/src/app/models/image.rb +++ b/src/app/models/image.rb @@ -84,6 +84,17 @@ class Image < ActiveRecord::Base end end
+ def rebuild! + # TODO: remove this check when we have UI for pushing images + if self.provider_type.providers.empty? or not self.provider_type.providers.detect {|p| !p.provider_accounts.empty?} + raise "Error: A valid provider and provider account are required to create and build templates" + end + self.status = STATE_QUEUED + self.save! + self.provider_images.each {|pimg| pimg.destroy} + Delayed::Job.enqueue(BuildJob.new(self.id)) + end + def not_uploaded_providers uploaded = self.provider_images Provider.all(:conditions => {:provider_type_id => self.provider_type.id}).select do |p| diff --git a/src/app/models/provider_image.rb b/src/app/models/provider_image.rb index aee69a9..9cc0e23 100644 --- a/src/app/models/provider_image.rb +++ b/src/app/models/provider_image.rb @@ -37,6 +37,12 @@ class ProviderImage < ActiveRecord::Base end end
+ def retry_upload! + self.status = STATE_QUEUED + self.save! + Delayed::Job.enqueue(BuildJob.new(self.id)) + end + def warehouse_bucket 'provider_images' end diff --git a/src/app/views/image_factory/builds/index.haml b/src/app/views/image_factory/builds/index.haml new file mode 100644 index 0000000..e69de29 diff --git a/src/app/views/image_factory/templates/_builds.haml b/src/app/views/image_factory/templates/_builds.haml index 65cbc5a..446ceb2 100644 --- a/src/app/views/image_factory/templates/_builds.haml +++ b/src/app/views/image_factory/templates/_builds.haml @@ -19,12 +19,20 @@ Builds for - else - @images.each do |img| %li - = "#{img.name}-#{img.template.architecture}: #{img.status}" + - form_tag(retry_image_factory_builds_path, { :method => :post }) do + = "#{img.name}-#{img.template.architecture}: #{img.status}" + = hidden_field_tag :template_id, @tpl.id + = hidden_field_tag :image_id, img.id + = submit_tag "Retry build", :name => "build" - if img.status == Image::STATE_COMPLETED %ul - img.provider_images.each do |pimg| %li - = "#{pimg.provider.name}: #{pimg.status}" + - form_tag(retry_image_factory_builds_path, { :method => :post }) do + = "#{pimg.provider.name}: #{pimg.status}" + = hidden_field_tag :template_id, @tpl.id + = hidden_field_tag :provider_image_id, img.id + = submit_tag "Retry upload", :name => "build" - unless @tpl.imported - img.not_uploaded_providers.each do |provider| %li diff --git a/src/config/routes.rb b/src/config/routes.rb index 313dd25..54c8ee6 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -46,7 +46,7 @@ ActionController::Routing::Routes.draw do |map| r.resources :deployables, :collection => { :multi_destroy => :delete } r.resources :templates, :collection => {:collections => :get, :add_selected => :get, :metagroup_packages => :get, :remove_package => :get, :multi_destroy => :delete} r.connect "/builds/update_status.:format", :controller => :builds, :action => :update_status - r.resources :builds, :collection => { :delete => :delete, :upload => :get } + r.resources :builds, :collection => { :delete => :delete, :upload => :get, :retry => :post } end
map.namespace 'admin' do |r| diff --git a/src/spec/controllers/builds_controller_spec.rb b/src/spec/controllers/builds_controller_spec.rb index a3d7f4d..17fac9c 100644 --- a/src/spec/controllers/builds_controller_spec.rb +++ b/src/spec/controllers/builds_controller_spec.rb @@ -53,5 +53,19 @@ describe ImageFactory::BuildsController do post :create, :template_id => @template.id, :target => ProviderType.find_by_codename("mock").id end.should change(Image, :count).by(1) end + + it "retry build should update Image status" do + lambda do + post :create, :template_id => @template.id, :target => ProviderType.find_by_codename("mock").id + end.should change(Image, :count).by(1) + @template.images.size.should == 1 + image = @template.images[0] + image.reload.status.should == "queued" + post :update_status, :uuid => image.uuid, :status => 'failed' + image.reload.status.should == "failed" + post :retry, :image_id => image.id, :template_id => @template.id + image.reload.status.should == "queued" + + end end end
Hi, 2 notes inline.
On 03/31/2011 11:30 PM, Scott Seago wrote:
Signed-off-by: Scott Seagosseago@redhat.com
.../controllers/image_factory/builds_controller.rb | 36 ++++++++++++++++++++ src/app/models/image.rb | 11 ++++++ src/app/models/provider_image.rb | 6 +++ src/app/views/image_factory/templates/_builds.haml | 12 +++++- src/config/routes.rb | 2 +- src/spec/controllers/builds_controller_spec.rb | 14 ++++++++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/app/views/image_factory/builds/index.haml
diff --git a/src/app/controllers/image_factory/builds_controller.rb b/src/app/controllers/image_factory/builds_controller.rb index b6efe81..a9901e5 100644 --- a/src/app/controllers/image_factory/builds_controller.rb +++ b/src/app/controllers/image_factory/builds_controller.rb @@ -1,6 +1,9 @@ class ImageFactory::BuildsController< ApplicationController before_filter [:require_user], :except => [:update_status]
- def index
- end
- def create @tpl = Template.find(params[:template_id]) check_permission
@@ -43,6 +46,39 @@ class ImageFactory::BuildsController< ApplicationController # FIXME: is @tpl defined here? do we need check_permission here? end
- def retry
- @tpl = Template.find(params[:template_id])
- check_permission
- begin
if params[:image_id]
image = Image.find(params[:image_id])
if image
errors = {}
warnings = []
image.rebuild!
flash[:notice] = "Queued rebuild for image #{image.name}"
else
flash[:warning] = "could not find image #{image_id}"
end
elsif params[:provider_image_id]
provider_image = ProviderImage.find(params[:provider_image_id])
if provider_image
provider_image.retry_upload!
flash[:notice] = "Queued upload for provider image #{provider_image.name}"
else
flash[:warning] = "could not find provider image #{provider_image_id}"
end
else
flash[:warning] = 'You need to specify either a provider or a provider image'
end
- rescue
flash[:warning] = $!.message
logger.error $!.message
logger.error $!.backtrace.join("\n ")
- end
- redirect_to image_factory_template_path(@tpl, :details_tab => 'builds')
- end
- def update_status image = Image.find_by_uuid(params[:uuid]) image = ProviderImage.find_by_uuid(params[:uuid]) unless image
diff --git a/src/app/models/image.rb b/src/app/models/image.rb index f15f2da..f0434b8 100644 --- a/src/app/models/image.rb +++ b/src/app/models/image.rb @@ -84,6 +84,17 @@ class Image< ActiveRecord::Base end end
- def rebuild!
- # TODO: remove this check when we have UI for pushing images
- if self.provider_type.providers.empty? or not self.provider_type.providers.detect {|p| !p.provider_accounts.empty?}
raise "Error: A valid provider and provider account are required to create and build templates"
- end
- self.status = STATE_QUEUED
- self.save!
- self.provider_images.each {|pimg| pimg.destroy}
- Delayed::Job.enqueue(BuildJob.new(self.id))
- end
- def not_uploaded_providers uploaded = self.provider_images Provider.all(:conditions => {:provider_type_id => self.provider_type.id}).select do |p|
diff --git a/src/app/models/provider_image.rb b/src/app/models/provider_image.rb index aee69a9..9cc0e23 100644 --- a/src/app/models/provider_image.rb +++ b/src/app/models/provider_image.rb @@ -37,6 +37,12 @@ class ProviderImage< ActiveRecord::Base end end
- def retry_upload!
- self.status = STATE_QUEUED
- self.save!
- Delayed::Job.enqueue(BuildJob.new(self.id))
PushJob should be called here.
- end
- def warehouse_bucket 'provider_images' end
diff --git a/src/app/views/image_factory/builds/index.haml b/src/app/views/image_factory/builds/index.haml new file mode 100644 index 0000000..e69de29 diff --git a/src/app/views/image_factory/templates/_builds.haml b/src/app/views/image_factory/templates/_builds.haml index 65cbc5a..446ceb2 100644 --- a/src/app/views/image_factory/templates/_builds.haml +++ b/src/app/views/image_factory/templates/_builds.haml @@ -19,12 +19,20 @@ Builds for - else - @images.each do |img| %li
= "#{img.name}-#{img.template.architecture}: #{img.status}"
- form_tag(retry_image_factory_builds_path, { :method => :post }) do
= "#{img.name}-#{img.template.architecture}: #{img.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :image_id, img.id
= submit_tag "Retry build", :name => "build" - if img.status == Image::STATE_COMPLETED %ul - img.provider_images.each do |pimg| %li
= "#{pimg.provider.name}: #{pimg.status}"
- form_tag(retry_image_factory_builds_path, { :method => :post }) do
= "#{pimg.provider.name}: #{pimg.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :provider_image_id, img.id
= submit_tag "Retry upload", :name => "build" - unless @tpl.imported - img.not_uploaded_providers.each do |provider| %li
I think 'retry' action should be allowed only for failed jobs or at least for not completed images / provider images. Also not sure what happens if I run 'retry' on image which is building at the moment (but I would expect that it finishes current build and pushes to warehouse, then starts new build invoked by 'retry' action).
Both retry buttons can be done by links instead of forms, but if you prefer this way, OK.
diff --git a/src/config/routes.rb b/src/config/routes.rb index 313dd25..54c8ee6 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -46,7 +46,7 @@ ActionController::Routing::Routes.draw do |map| r.resources :deployables, :collection => { :multi_destroy => :delete } r.resources :templates, :collection => {:collections => :get, :add_selected => :get, :metagroup_packages => :get, :remove_package => :get, :multi_destroy => :delete} r.connect "/builds/update_status.:format", :controller => :builds, :action => :update_status
- r.resources :builds, :collection => { :delete => :delete, :upload => :get }
r.resources :builds, :collection => { :delete => :delete, :upload => :get, :retry => :post } end
map.namespace 'admin' do |r|
diff --git a/src/spec/controllers/builds_controller_spec.rb b/src/spec/controllers/builds_controller_spec.rb index a3d7f4d..17fac9c 100644 --- a/src/spec/controllers/builds_controller_spec.rb +++ b/src/spec/controllers/builds_controller_spec.rb @@ -53,5 +53,19 @@ describe ImageFactory::BuildsController do post :create, :template_id => @template.id, :target => ProviderType.find_by_codename("mock").id end.should change(Image, :count).by(1) end
- it "retry build should update Image status" do
lambda do
post :create, :template_id => @template.id, :target => ProviderType.find_by_codename("mock").id
end.should change(Image, :count).by(1)
@template.images.size.should == 1
image = @template.images[0]
image.reload.status.should == "queued"
post :update_status, :uuid => image.uuid, :status => 'failed'
image.reload.status.should == "failed"
post :retry, :image_id => image.id, :template_id => @template.id
image.reload.status.should == "queued"
- end end end
On 04/01/2011 07:17 AM, Jan Provaznik wrote:
Hi, 2 notes inline.
On 03/31/2011 11:30 PM, Scott Seago wrote:
Signed-off-by: Scott Seagosseago@redhat.com
.../controllers/image_factory/builds_controller.rb | 36 ++++++++++++++++++++ src/app/models/image.rb | 11 ++++++ src/app/models/provider_image.rb | 6 +++ src/app/views/image_factory/templates/_builds.haml | 12 +++++- src/config/routes.rb | 2 +- src/spec/controllers/builds_controller_spec.rb | 14 ++++++++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/app/views/image_factory/builds/index.haml
diff --git a/src/app/models/provider_image.rb b/src/app/models/provider_image.rb index aee69a9..9cc0e23 100644 --- a/src/app/models/provider_image.rb +++ b/src/app/models/provider_image.rb @@ -37,6 +37,12 @@ class ProviderImage< ActiveRecord::Base end end
- def retry_upload!
- self.status = STATE_QUEUED
- self.save!
- Delayed::Job.enqueue(BuildJob.new(self.id))
PushJob should be called here.
Good catch. Push retry is the one area that didn't get end-to-end testing since I was blocked on some (now fixed) connector problems.
- end
- def warehouse_bucket 'provider_images' end
diff --git a/src/app/views/image_factory/builds/index.haml b/src/app/views/image_factory/builds/index.haml new file mode 100644 index 0000000..e69de29 diff --git a/src/app/views/image_factory/templates/_builds.haml b/src/app/views/image_factory/templates/_builds.haml index 65cbc5a..446ceb2 100644 --- a/src/app/views/image_factory/templates/_builds.haml +++ b/src/app/views/image_factory/templates/_builds.haml @@ -19,12 +19,20 @@ Builds for - else - @images.each do |img| %li
= "#{img.name}-#{img.template.architecture}: #{img.status}"
- form_tag(retry_image_factory_builds_path, { :method =>
:post }) do
= "#{img.name}-#{img.template.architecture}: #{img.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :image_id, img.id
= submit_tag "Retry build", :name => "build" - if img.status == Image::STATE_COMPLETED %ul - img.provider_images.each do |pimg| %li
= "#{pimg.provider.name}: #{pimg.status}"
- form_tag(retry_image_factory_builds_path, {
:method => :post }) do
= "#{pimg.provider.name}: #{pimg.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :provider_image_id, img.id
= submit_tag "Retry upload", :name => "build" - unless @tpl.imported - img.not_uploaded_providers.each do |provider| %li
I think 'retry' action should be allowed only for failed jobs or at least for not completed images / provider images. Also not sure what happens if I run 'retry' on image which is building at the moment (but I would expect that it finishes current build and pushes to warehouse, then starts new build invoked by 'retry' action).
Yes, initially I wanted to do something more elaborate around failure detection and limit this, but there were a bunch of edge cases and Jay had suggested that we don't worry about them yet. In particular, if we prevent retry for an image which is building at the moment, we won't catch a case where factory died while building, since we'll never get the 'failed' status update. Eventually the factory interface needs to be able to report a crashed factory properly and then we could limit this to failed builds.
However, maybe we do want to not allow retry for completed builds -- I'll see if Jay has any input here as well.
Both retry buttons can be done by links instead of forms, but if you prefer this way, OK.
I made them buttons since they need to be POST actions and we're not doing a lot with javascript yet -- this whole view is due for a complete UI overhaul (to make a nice nested table with more metadata, etc), so I'd rather defer this sort of UI question to then.
Scott
On Fri, 2011-04-01 at 08:53 -0400, Scott Seago wrote:
On 04/01/2011 07:17 AM, Jan Provaznik wrote:
Hi, 2 notes inline.
On 03/31/2011 11:30 PM, Scott Seago wrote:
Signed-off-by: Scott Seagosseago@redhat.com
.../controllers/image_factory/builds_controller.rb | 36 ++++++++++++++++++++ src/app/models/image.rb | 11 ++++++ src/app/models/provider_image.rb | 6 +++ src/app/views/image_factory/templates/_builds.haml | 12 +++++- src/config/routes.rb | 2 +- src/spec/controllers/builds_controller_spec.rb | 14 ++++++++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/app/views/image_factory/builds/index.haml
diff --git a/src/app/models/provider_image.rb b/src/app/models/provider_image.rb index aee69a9..9cc0e23 100644 --- a/src/app/models/provider_image.rb +++ b/src/app/models/provider_image.rb @@ -37,6 +37,12 @@ class ProviderImage< ActiveRecord::Base end end
- def retry_upload!
- self.status = STATE_QUEUED
- self.save!
- Delayed::Job.enqueue(BuildJob.new(self.id))
PushJob should be called here.
Good catch. Push retry is the one area that didn't get end-to-end testing since I was blocked on some (now fixed) connector problems.
- end
- def warehouse_bucket 'provider_images' end
diff --git a/src/app/views/image_factory/builds/index.haml b/src/app/views/image_factory/builds/index.haml new file mode 100644 index 0000000..e69de29 diff --git a/src/app/views/image_factory/templates/_builds.haml b/src/app/views/image_factory/templates/_builds.haml index 65cbc5a..446ceb2 100644 --- a/src/app/views/image_factory/templates/_builds.haml +++ b/src/app/views/image_factory/templates/_builds.haml @@ -19,12 +19,20 @@ Builds for - else - @images.each do |img| %li
= "#{img.name}-#{img.template.architecture}: #{img.status}"
- form_tag(retry_image_factory_builds_path, { :method =>
:post }) do
= "#{img.name}-#{img.template.architecture}: #{img.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :image_id, img.id
= submit_tag "Retry build", :name => "build" - if img.status == Image::STATE_COMPLETED %ul - img.provider_images.each do |pimg| %li
= "#{pimg.provider.name}: #{pimg.status}"
- form_tag(retry_image_factory_builds_path, {
:method => :post }) do
= "#{pimg.provider.name}: #{pimg.status}"
= hidden_field_tag :template_id, @tpl.id
= hidden_field_tag :provider_image_id, img.id
= submit_tag "Retry upload", :name => "build" - unless @tpl.imported - img.not_uploaded_providers.each do |provider| %li
I think 'retry' action should be allowed only for failed jobs or at least for not completed images / provider images. Also not sure what happens if I run 'retry' on image which is building at the moment (but I would expect that it finishes current build and pushes to warehouse, then starts new build invoked by 'retry' action).
Yes, initially I wanted to do something more elaborate around failure detection and limit this, but there were a bunch of edge cases and Jay had suggested that we don't worry about them yet. In particular, if we prevent retry for an image which is building at the moment, we won't catch a case where factory died while building, since we'll never get the 'failed' status update. Eventually the factory interface needs to be able to report a crashed factory properly and then we could limit this to failed builds.
However, maybe we do want to not allow retry for completed builds -- I'll see if Jay has any input here as well.
Your call for right now. I think it is a good idea in general, but as you said, there will be a lot to add to this later anyway. My inclination would be to get the simplest possible thing out first, then enhance it over time, and make a new story for those enhancements.
Both retry buttons can be done by links instead of forms, but if you prefer this way, OK.
I made them buttons since they need to be POST actions and we're not doing a lot with javascript yet -- this whole view is due for a complete UI overhaul (to make a nice nested table with more metadata, etc), so I'd rather defer this sort of UI question to then.
I tend to agree with this thinking - they could be links if you could click them many times without side-effect, but that is not really the case (yet, anyway). Non-idempotent actions should always be forms.
-j
Scott _______________________________________________ aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
aeolus-devel@lists.fedorahosted.org