Hey Richard,
ACK. The patch looks great, but I've noticed a few things that need fixing:
There are instances of trailing whitespace in the new code and that is
against our coding standards. I've highlighted them inline.
I recommend that you enable the git hooks as shown in this wiki:
http://aeolusproject.org/page/Git_Configuration
That will configure git to reject your commits if they have any trailing
WS in them.
It's also useful to set up your editor so that it highlights the
trailing whitespace.
In addition, there are a few notes about the "Upload" and "Delete"
buttons -- they're inline in the _builds.haml area.
After you fix these things, feel free to push the code.
Thanks,
Thomas
On 03/04/2011 02:57 AM, Richard Su wrote:
Renamed "Images" tab to "Builds"
Table columns now matches cloud-engine-templates-details-images.png mockup.
Table shows builds that have been completed and uploaded, and additional
providers that a completed build can be uploaded to. The additional provider's
provider type would have to match the build's target.
Note the Arch column may be redundant because the Architecture is specified
per template and not per build.
Added an additional column to select providers to upload an existing image.
The hook to upload the image will need to be done at a future sprint when the
new image factory is ready.
It also contains a new form element to initiate a build. The select drop down
will only show targets that haven't been built. Try creating a mock provider
and you should see it appear in the select.
---
.../controllers/image_factory/builds_controller.rb | 9 ++++-
.../image_factory/templates_controller.rb | 42 ++++++++++++++++---
src/app/models/image.rb | 4 +-
src/app/views/image_factory/templates/_builds.haml | 42 ++++++++++++++++++++
src/app/views/image_factory/templates/_images.haml | 20 ---------
src/config/routes.rb | 2 +-
src/features/step_definitions/template_steps.rb | 6 ++-
src/features/support/paths.rb | 2 +-
src/features/template.feature | 13 ++++++
src/spec/factories/provider.rb | 8 ++++
10 files changed, 115 insertions(+), 33 deletions(-)
create mode 100644 src/app/views/image_factory/templates/_builds.haml
delete mode 100644 src/app/views/image_factory/templates/_images.haml
diff --git a/src/app/controllers/image_factory/builds_controller.rb
b/src/app/controllers/image_factory/builds_controller.rb
index ac0a3e2..6d335df 100644
--- a/src/app/controllers/image_factory/builds_controller.rb
+++ b/src/app/controllers/image_factory/builds_controller.rb
@@ -38,12 +38,19 @@ class ImageFactory::BuildsController< ApplicationController
end
flash[:warning] = 'Warning: ' + warnings.join unless warnings.empty?
if errors.empty?
- redirect_to image_factory_template_path(@tpl, :details_tab =>
'images')
+ redirect_to image_factory_template_path(@tpl, :details_tab =>
'builds')
else
flash_error('Error while trying to build image', errors)
render :action => 'new'
end
end
+
Two empty spaces here ^
+ def upload_or_delete
+ @tpl = Template.find(params[:template_id])
+ # FIXME: add logic to upload image
Empty space at the end of this line ^
+ # or delete image depending on which button was used
+ redirect_to image_factory_template_path(@tpl, :details_tab => 'builds')
+ end
def edit
# FIXME: is @tpl defined here? do we need check_permission here?
diff --git a/src/app/controllers/image_factory/templates_controller.rb
b/src/app/controllers/image_factory/templates_controller.rb
index 5f905fa..096a9c5 100644
--- a/src/app/controllers/image_factory/templates_controller.rb
+++ b/src/app/controllers/image_factory/templates_controller.rb
@@ -25,9 +25,9 @@ class ImageFactory::TemplatesController< ApplicationController
@tpl = Template.find(params[:id])
require_privilege(Privilege::VIEW, @tpl)
@url_params = params.clone
- @tab_captions = ['Properties', 'Images']
+ @tab_captions = ['Properties', 'Builds']
@details_tab = params[:details_tab].blank? ? 'properties' :
params[:details_tab]
- load_images(@tpl) if @details_tab == 'images'
+ load_images(@tpl) if @details_tab == 'builds'
respond_to do |format|
format.js do
if @url_params.delete :details_pane
@@ -196,12 +196,41 @@ class ImageFactory::TemplatesController< ApplicationController
def load_images(tpl)
@images_header = [
- {:name => 'NAME', :sort_attr => 'templates.name'},
- {:name => 'OS', :sort_attr => 'templates.platform'},
- {:name => 'VERSION', :sort_attr =>
'templates.platform_version'},
+ {:name => '', :sort_attr => 'select'},
{:name => 'ARCH', :sort_attr =>
'templates.architecture'},
+ {:name => 'PROVIDER', :sort_attr => 'provider'},
{:name => 'STATUS', :sort_attr => 'status'},
- ]
+ {:name => 'UPLOADED?', :sort_attr => 'images.uploaded'},
+ ]
+ @imaged_provider_types = {}
+ @targets_not_built = {}
+ @target_build_status = {}
+
+ Provider.all.each do |p|
+ @targets_not_built[p.provider_type.id] = p.provider_type.name
+ end
+
+ tpl.images.each do |img|
+ @target_build_status[img.provider_type.name] = img.status
+ @targets_not_built.delete(img.provider_type_id)
+ img.provider_images.each do |pimg|
+ @imaged_provider_types[pimg.provider_id] = img.provider_type.id
+ end
+ end
+ @targets_not_built = @targets_not_built.invert
+
+ @providers_to_delete = {}
+ @providers_not_uploaded =
Provider.find_all_by_provider_type_id((a)imaged_provider_types.values.uniq)
+ @providers_not_uploaded.each do |provider|
+ if @imaged_provider_types.has_key?(provider.id)
+ @providers_to_delete[provider] = provider
+ end
+ end
+
+ @providers_to_delete.keys.each do |p|
+ @providers_not_uploaded.delete(p)
+ end
+
Four empty spaces here ^
@images = tpl.images
end
@@ -283,4 +312,3 @@ class ImageFactory::TemplatesController< ApplicationController
end
end
-
diff --git a/src/app/models/image.rb b/src/app/models/image.rb
index cfa15e4..dad94f3 100644
--- a/src/app/models/image.rb
+++ b/src/app/models/image.rb
@@ -83,7 +83,7 @@ class Image< ActiveRecord::Base
unless provider = Provider.find_by_target_with_account(target.id)
raise "There is no provider for '#{target.name}' type with valid
account."
end
-
+
Another four here ^
img = nil
Image.transaction do
img = Image.create!(
@@ -94,7 +94,7 @@ class Image< ActiveRecord::Base
)
ProviderImage.create!(
:image_id => img.id,
- :provider_id => provider
+ :provider_id => provider.id
)
end
return img
diff --git a/src/app/views/image_factory/templates/_builds.haml
b/src/app/views/image_factory/templates/_builds.haml
new file mode 100644
index 0000000..f1a98c8
--- /dev/null
+++ b/src/app/views/image_factory/templates/_builds.haml
@@ -0,0 +1,42 @@
+Builds for
Whitespace at EOL ^
+= @tpl.platform
+= @tpl.platform_version
+, template "#{(a)tpl.name}"
+
+- form_tag(image_factory_builds_path, { :action => 'create', :method =>
:post }) do
+ = hidden_field_tag :template_id, @tpl.id
+ New Build for
+ = select( "targets", "", options_for_select(@targets_not_built))
+ = submit_tag "Go", :name => "build"
+
2 spaces here ^
+- form_tag(upload_or_delete_image_factory_builds_path, { :action
=> "destroy", :method => :post }) do
+ = hidden_field_tag :template_id, @tpl.id
+ /= restful_submit_tag "Cancel Job", :name => "job_details",
:disabled => true, :class => "icon"
+ /= restful_submit_tag "Show Job Details", :name =>
"job_details", :disabled => true, :class => "icon"
+ /= restful_submit_tag "Show Job Log", :name => "job_log",
:disabled => true, :class => "icon"
+ /= restful_submit_tag "Clear Job History", :name =>
"job_history", :disabled => true, :class => "icon"
I know it wasnt't you who commented the lines here out, but could you
please remove them? Code that is commented out without any explanation
only places a mental burden on anyone trying to read it.
And judging by the mockups, it's unclear that we'll ever use these buttons.
+ = submit_tag "Upload", :name => "upload"
+ = submit_tag "Delete", :name => "delete"
The "Upload" and "Delete" buttons don't do anything at the moment.
Judging by the FIXME in the controller I think that's intentional, but I
wanted to make sure it wasn't just forgotten.
If they're intentionally not functional yet, maybe you could make them
disabled so that nobody's going to click on them?
And one last thing: notice the restful_submit_tag helper above? That's a
neat method that will allow you to map different submit buttons in a
single form to different controller actions so that you don't have to
write dispatch controller code like the upload_or_delete action that you
have now.
If you use this instead:
= restful_submit_tag "Upload", 'upload',
upload_image_factory_builds_path, 'POST'
= restful_submit_tag "Delete", 'delete',
destroy_image_factory_builds_path, 'DELETE'
It will do the right thing and call the "upload" or "destroy" action
in
your controller with the specified HTTP method.
You can look at other views to see how we use this. So please change
that (and the corresponding controller actions) even if it doesn't do
anything.
It'll make it more consistent with the rest of the code and when someone
picks the implementation up, they won't be dragged into this manual
dispatch route.
+
2 spaces here ^
> + %table
> + = sortable_table_header(@images_header)
> + %tbody
> + - if @images.empty?
> + %tr
> + %td{:colspan => 5} No Builds
> + - else
> + - @images.each do |img|
> + - img.provider_images.each do |pimg|
> + %tr
> + %td= check_box_tag 'selected[]', pimg.provider_id,
@url_params[:select] == 'all', :id =>
"selected_#{pimg.provider_id}"
> + %td= img.template.architecture
> + %td= pimg.provider.nil? ? '' : pimg.provider.name
> + %td= img.status
> + %td= pimg.uploaded? ? 'yes' : 'no'
> + - @providers_not_uploaded.each do |provider|
> + %tr
> + %td= check_box_tag 'selected[]', provider.id, @url_params[:select]
== 'all', :id => "selected_#{provider.id}"
> + %td= @tpl.architecture
> + %td= provider.name
> + %td= @target_build_status[provider.provider_type]
> + %td no
> \ No newline at end of file
> diff --git a/src/app/views/image_factory/templates/_images.haml
b/src/app/views/image_factory/templates/_images.haml
> deleted file mode 100644
> index 3fb1046..0000000
> --- a/src/app/views/image_factory/templates/_images.haml
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -- form_tag :action => "" do
> - /= restful_submit_tag "Cancel Job", :name =>
"job_details", :disabled => true, :class => "icon"
> - /= restful_submit_tag "Show Job Details", :name =>
"job_details", :disabled => true, :class => "icon"
> - /= restful_submit_tag "Show Job Log", :name => "job_log",
:disabled => true, :class => "icon"
> - /= restful_submit_tag "Clear Job History", :name =>
"job_history", :disabled => true, :class => "icon"
> -
> - %table
> - = sortable_table_header(@images_header)
> - %tbody
> - - if @images.empty?
> - %tr
> - %td{:colspan => 5} No Images
> - - else
> - - @images.each do |img|
> - %tr
> - %td= img.template.name
> - %td= img.template.platform
> - %td= img.template.platform_version
> - %td= img.template.architecture
> - %td= img.status
> diff --git a/src/config/routes.rb b/src/config/routes.rb
> index c936726..b64713f 100644
> --- a/src/config/routes.rb
> +++ b/src/config/routes.rb
> @@ -45,7 +45,7 @@ ActionController::Routing::Routes.draw do |map|
> r.resources :assemblies
> 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.resources :builds
> + r.resources :builds, :collection => { :upload_or_delete => :delete }
> end
>
> map.namespace 'admin' do |r|
> diff --git a/src/features/step_definitions/template_steps.rb
b/src/features/step_definitions/template_steps.rb
> index 3491eb4..69a6928 100644
> --- a/src/features/step_definitions/template_steps.rb
> +++ b/src/features/step_definitions/template_steps.rb
> @@ -91,7 +91,6 @@ end
> When /^I choose this template$/ do
> click_link((a)template.name)
> end
> -
> Given /^there is Amazon AWS provider account$/ do
> account = Factory.build(:ec2_provider_account)
> account.save!
> @@ -101,6 +100,11 @@ Given /^there is Amazon AWS build for this template$/ do
> Image.build(@template, ProviderType.find_by_codename("ec2"))
> end
>
> +Given /^there is Amazon AWS provider with no builds$/ do
> + provider = Factory.build(:ec2_provider_no_builds)
> + provider.save!
> +end
+
> Given /^there is an imported template$/ do
> @template = Factory.build :template, :name => name, :imported => true
> @template.save!
> diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb
> index c123cba..5af9b34 100644
> --- a/src/features/support/paths.rb
> +++ b/src/features/support/paths.rb
> @@ -81,7 +81,7 @@ module NavigationHelpers
> url_for new_build_path
>
> when /the template builds page/
> - url_for image_factory_template_path(@template, :details_tab =>
'images')
> + url_for image_factory_template_path(@template, :details_tab =>
'builds')
>
> when /the templates page/
> templates_path
> diff --git a/src/features/template.feature b/src/features/template.feature
> index 336b194..4fe8e9a 100644
> --- a/src/features/template.feature
> +++ b/src/features/template.feature
> @@ -105,6 +105,19 @@ Feature: Manage Templates
> # test that we see a collection (collections are loaded by default)
> And I should see an input "conductor"
>
> + Scenario: See upload status
> + Given there is a "mock1" template
> + And there is Amazon AWS provider account
> + And there is Amazon AWS build for this template
> + And there is Amazon AWS provider with no builds
> + And I am on the image factory templates page
> + When I choose this template
> + And I follow "Builds"
> + Then I should see the following:
> + | ARCH | PROVIDERS | STATUS | UPLOADED? |
> + | x86_64 | amazon-ec2 | queued | no |
> + | x86_64 | amazon-ec2-no-builds | queued | no |
+
> Scenario: Build template
> Given there is a "mock1" template
> And there is Amazon AWS provider account
> diff --git a/src/spec/factories/provider.rb b/src/spec/factories/provider.rb
> index 03f8773..34f3011 100644
> --- a/src/spec/factories/provider.rb
> +++ b/src/spec/factories/provider.rb
> @@ -25,3 +25,11 @@ Factory.define :ec2_provider, :parent => :provider do |p|
> p.hardware_profiles { |hp| [hp.association(:ec2_hwp1)] }
> p.after_create { |p| p.realms<< Factory(:realm4, :provider => p) }
> end
+
> +Factory.define :ec2_provider_no_builds, :parent =>
:provider do |p|
> + p.name 'amazon-ec2-no-builds'
> + p.provider_type { ProviderType.find_by_codename("ec2") }
> + p.url 'http://localhost:3002/api'
> + p.hardware_profiles { |hp| [hp.association(:ec2_hwp1)] }
> + p.after_create { |p| p.realms<< Factory(:realm4, :provider => p) }
> +end