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 Seago<sseago(a)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