On 06/02/2011 10:00 PM, Matt Wagner wrote:
The feature 1605 has 2 subtasks: 1610 (parse deployable definition) and
1607 (replace the select deployable UI with a URL box for deployable).
Looks like this patch implements the latter but not the former. Neither
of the tasks are assigned in Redmine so I just want to make sure that's
what's going on.
There is 1 failing RSpec test and a 14 Cucumber features.
The RSpec one is obsolete now that we don't require
deployment.deployable_id to be set.
Deleting the test on spec/models/deployment_spec.rb line 19 takes care
of it.
However, does this mean that we really no longer require a deployment to
be attached to a deployable? Or is this just a temporary thing?
Most of the cuke failures seem to be related to the UI change but there
are a few nil references as well.
Additionally, there are some concerns about pushing this before all the
other pieces are in place as it would break launching deployments in the
meantime.
Other than that, the patch looks good. There are a few notes inline but
nothing big.
Thomas
--
No trees were killed to send this message, but a large number of
electrons were terribly inconvenienced.
---
src/app/controllers/deployments_controller.rb | 58 +++++++++-----------
src/app/models/deployment.rb | 2 +-
src/app/views/deployments/_edit.haml | 41 +++++++++++---
src/app/views/deployments/_launch_new.haml | 25 ++++++---
src/app/views/deployments/_new.haml | 50 +++++++++--------
src/app/views/pools/_header_show.haml | 2 +-
src/config/locales/en.yml | 1 +
src/config/routes.rb | 2 +-
..._remove_not_null_constraint_on_deployable_id.rb | 9 +++
9 files changed, 116 insertions(+), 74 deletions(-)
create mode 100644
src/db/migrate/20110602144940_remove_not_null_constraint_on_deployable_id.rb
diff --git a/src/app/controllers/deployments_controller.rb
b/src/app/controllers/deployments_controller.rb
index 53ec037..d198679 100644
--- a/src/app/controllers/deployments_controller.rb
+++ b/src/app/controllers/deployments_controller.rb
@@ -3,6 +3,8 @@ class DeploymentsController< ApplicationController
before_filter :load_deployments, :only => [:index, :show]
before_filter :load_deployment, :only => [:edit, :update]
+ layout 'application'
+
def index
save_breadcrumb(deployments_path(:viewstate => @viewstate ? @viewstate.id :
nil))
respond_to do |format|
@@ -26,22 +28,12 @@ class DeploymentsController< ApplicationController
def new
require_privilege(Privilege::CREATE, Deployment)
- @deployment = Deployment.new(:deployable_id => params[:deployable_id])
+ @pool = Pool.find_by_id(params[:pool_id]) or raise "Invalid pool"
+ @deployment = Deployment.new(:pool_id => params[:pool_id])
respond_to do |format|
- if @deployment.deployable.assemblies.empty?
- flash[:warning] = "Deployable must have at least one assembly"
- format.js do
- load_deployments
- render :partial => 'list'
- end
- format.html { redirect_to deployments_path }
- format.json { render :json => {:error => flash[:warning]}, :status =>
:unprocessable_entity }
- else
- init_new_deployment_attrs
- format.js { render :partial => 'new' }
- format.html
- format.json { render :json => @deployment }
- end
+ format.js { render :partial => 'new' }
+ format.html
+ format.json { render :json => @deployment}
end
end
@@ -49,25 +41,18 @@ class DeploymentsController< ApplicationController
require_privilege(Privilege::CREATE, Deployment)
@deployment = Deployment.new(params[:deployment])
@deployment.owner = current_user
+ @pool = @deployment.pool
+ # TODO - The save logic is TBD here...
+ @deployment.import_xml_from_url(params[:deployable]['url'])
respond_to do |format|
if @deployment.save
- flash[:notice] = "Deployment launched"
- errors = @deployment.launch(params[:hw_profiles] || {}, current_user)
- unless errors.empty?
- flash[:error] = {
- :summary => "Failed to launch following assemblies:",
- :failures => errors
- }
- end
- format.js { render :partial => 'properties' }
- format.html { redirect_to deployment_path(@deployment) }
- format.json { render :json => @deployment, :status => :created }
+ format.js { render :partial => 'edit' }
+ format.html { redirect_to edit_deployment_url(@deployment) }
+ format.json { render :json => @deployment }
else
- flash.now[:warning] = "Deployment launch failed"
- init_new_deployment_attrs
- format.js { launch_new }
+ format.js { render :partial => 'new' }
format.html { render :action => 'new' }
- format.json { render :json => @deployment.errors, :status =>
:unprocessable_entity }
+ format.json { render :json => @deployment }
end
end
end
@@ -92,7 +77,10 @@ class DeploymentsController< ApplicationController
end
def edit
+ # TODO - Need to pull in provider images
+ # TODO - Need to pull in quota information
require_privilege(Privilege::MODIFY, @deployment)
+ @pool = @deployment.pool
respond_to do |format|
format.js { render :partial => 'edit' }
format.html
@@ -103,9 +91,10 @@ class DeploymentsController< ApplicationController
# TODO - This should eventually support updating multiple objects
def update
attrs = {}
- params[:deployment].each_pair{|k,v| attrs[k] = v if
Deployment::USER_MUTABLE_ATTRS.include?(k)}
+ params[:deployment].each_pair{|k,v| attrs[k] = v if
Deployment::USER_MUTABLE_ATTRS.include?(k)} if params[:deployment]
respond_to do |format|
if check_privilege(Privilege::MODIFY, @deployment) and
@deployment.update_attributes(attrs)
+ @deployment.launch({}, current_user) if params[:deployment]['launch'] ==
"true"
flash[:success] = t('deployments.updated', :count => 1, :list =>
@deployment.name)
format.js { render :partial => 'properties' }
format.html { redirect_to @deployment }
@@ -178,6 +167,13 @@ class DeploymentsController< ApplicationController
end
end
+ # Quick way to check if a deployment name is taken or not
+ # Returns "true" if name is available; "false" if it is already
taken
+ def check_name
+ deployment = Deployment.find_by_name(params[:name])
+ render :text => deployment.nil?.to_s
+ end
+
private
def load_deployments
@url_params = params
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 99a97ff..11ff6cf 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -62,7 +62,7 @@ class Deployment< ActiveRecord::Base
has_one :provider, :through => :realm
validates_presence_of :pool_id
- validates_presence_of :deployable_id
+# validates_presence_of :deployable_id
validates_presence_of :name
validates_uniqueness_of :name, :scope => :pool_id
diff --git a/src/app/views/deployments/_edit.haml b/src/app/views/deployments/_edit.haml
index 72c8e60..7ae1f5a 100644
--- a/src/app/views/deployments/_edit.haml
+++ b/src/app/views/deployments/_edit.haml
@@ -1,11 +1,34 @@
= error_messages_for 'deployment'
-%h2 Edit deployment
+%header.page-header
+ %h1
+ Deployment
+ = @deployment.name
+%h2
+ Launch Summary
+
+#scoreboard
+ %dl.statistics
+ %ul.groups
+ %li
+ %dt= t("pools.index.pool")
+ %dd= @pool.name
+ %li
+ %dt= t("deployments.deployment")
+ %dd= @deployment.name
+ %li
+ = link_to('view deployable definition', '#')
+
+%h2
+ Images
+
+-# Cannot implement this with real data until we have deployments implemented...
+= filter_table(['Image'], []) do |image|
+ %tr
+ %td
+ = image.name
+
- form_for @deployment do
- = hidden_field :deployment, :deployable_id
- %ul
- %li
- = label :deployment, :name
- = text_field :deployment, :name
-
- = submit_tag 'Cancel', :name => 'cancel'
- = submit_tag 'Save', :name => 'save'
+ = hidden_field 'deployment', 'launch', {:value => true}
+ = submit_tag 'Launch'
+
+-# Quota checking to go here as well, once I understand how it works with new code
\ No newline at end of file
diff --git a/src/app/views/deployments/_launch_new.haml
b/src/app/views/deployments/_launch_new.haml
index 34126dd..5f66d5e 100644
--- a/src/app/views/deployments/_launch_new.haml
+++ b/src/app/views/deployments/_launch_new.haml
@@ -1,8 +1,17 @@
-= if request.xhr?
- = render :partial => '/layouts/notification'
-%h2 Launch new deployment via
-- form_tag new_deployment_path, :method => :get do
- = label_tag "Deployable:"
- = select_tag :deployable_id,
options_from_collection_for_select(@launchable_deployables, "id",
"name")
- = submit_tag "Launch"
- = link_to "Cancel", deployments_path, :class => "button"
+= error_messages_for 'deployment'
+%h1
+ New Deployment in
+ = @pool.name if @pool
+%h2
+ Deployment Details
+- form_for :deployment, :url => deployments_path do
+ = hidden_field :deployment, :pool_id
+ %p
+ What do you want to launch?
+ %p
+ Provide the URL of the Deployable definition
+ = text_field('deployable', 'url')
+ %p
+ Name your deployment
+ = text_field(:deployment, :name)
+ = submit_tag('Next')
\ No newline at end of file
It looks to me that the _launch_new.haml and _new.haml partials are
virtually identical.
Would it make sense to have both render a single partial view?
diff --git a/src/app/views/deployments/_new.haml
b/src/app/views/deployments/_new.haml
index 390840d..0ed55a8 100644
--- a/src/app/views/deployments/_new.haml
+++ b/src/app/views/deployments/_new.haml
@@ -1,25 +1,29 @@
= error_messages_for 'deployment'
-%h2 Launch deployable
-- form_for @deployment, :url => deployments_path do
- = hidden_field :deployment, :deployable_id
- %ul
- %li
- = label :deployment, :name
- = text_field :deployment, :name
- %li
- = label :deployment, :pool
- = select :deployment, :pool_id, @pools.map {|p| [ p.name, p.id ]}, {
:include_blank => true }
- %li
- = label :deployment, :frontend_realm
- = select :deployment, :frontend_realm_id, @realms.map {|r| [ r.name, r.id ]}, {
:include_blank => true }
- %p
- %h3 Assemblies hardware profiles
- %ul
- - @deployment.deployable.assemblies.each do |assembly|
- %li
- - hwprofiles = @hardware_profiles.select {|hwp| hwp.architecture.value ==
assembly.templates.first.architecture}
- = label_tag "hw_profiles[#{assembly.id}]", assembly.name
- = select_tag "hw_profiles[#{assembly.id}]",
options_from_collection_for_select(hwprofiles, 'id', 'name')
+%header.page-header
+ %h1
+ New Deployment in
+ = @pool.name if @pool
+%h2
+ Deployment Details
+- form_for :deployment, :url => deployments_path do
+ = hidden_field :deployment, :pool_id
+ %p
+ What do you want to launch?
+ %p
+ Provide the URL of the Deployable definition
+ = text_field('deployable', 'url')
+ %p
+ Name your deployment
+ = text_field(:deployment, :name)
+ %span.name_available
+ = submit_tag('Next')
- = submit_tag 'Cancel', :name => 'cancel'
- = submit_tag 'Launch', :name => 'launch'
+:javascript
+ $(document).ready(function () {
+ $('#deployment_name').keyup(function(e) {
+ e.preventDefault();
+ $.get('/conductor/deployments/check_name', {name:
$('#deployment_name').val() }, function(data) {
The path '/conductor/deployments/check_name' doesn't work when running
from a dev environment without the 'coductor' prefix.
Using "#{check_name_deployments_path}" would work no matter how it's set
up.
+ $('.name_available').html(data == "false"
? "That name is already in use" : "Name available");
+ });
+ });
+ });
There is a race condition in this check.
Since the $.get requests are asynchronous and their time of delivery
over the network is not guaranteed, the uniqueness check result may not
reflect the name entered in the text field.
I'm not sure how serious this actually is (may almost never happen) and
I realize that the correct way of implementing is more difficult.
I recommend sticking a TODO warning to the code and we'll see how bad
this actually gets (unless you want to fix it now).
diff --git a/src/app/views/pools/_header_show.haml
b/src/app/views/pools/_header_show.haml
index 2ea3cac..3b42782 100644
--- a/src/app/views/pools/_header_show.haml
+++ b/src/app/views/pools/_header_show.haml
@@ -4,7 +4,7 @@
Pool
-# Insert appropriate controls here, or ommit %ul.controls
#obj_actions.button-container
- = link_to 'New Deployment', launch_new_deployments_path, :class =>
'button primary', :id => 'new_deployment_button'
+ = link_to 'New Deployment', new_deployment_path(:pool_id => @pool.id),
:class => 'button primary', :id => 'new_deployment_button'
.button-group
= link_to 'Edit', edit_pool_path(@pool), :class => 'button
pill', :id => 'edit_pool_button'
- form_tag do
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 73ddb39..5cc6f1c 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -70,6 +70,7 @@ en:
updates: Udates
deployments:
deployments: Deployments
+ deployment: Deployment
deployment_name: Deployment Name
updated:
one: "The deployment %{list} was successfully updated."
diff --git a/src/config/routes.rb b/src/config/routes.rb
index 1a557cd..6695651 100644
--- a/src/config/routes.rb
+++ b/src/config/routes.rb
@@ -33,7 +33,7 @@ ActionController::Routing::Routes.draw do |map|
# -- just remember to delete public/index.html.
map.resources :pools, :collection => { :multi_destroy => :delete }
- map.resources :deployments, :collection => { :multi_stop => :get, :launch_new
=> :get }
+ map.resources :deployments, :collection => { :multi_stop => :get, :launch_new
=> :get, :check_name => :get }
map.resources :instances, :collection => {:start => :get, :multi_stop =>
:get, :select_template => :get, :remove_failed => :get, :can_start => :get,
:can_create => :get }, :member => {:key => :get}
map.can_start_instance
'/instances/:instance_id/can_start/:provider_account_id', :controller =>
'instances', :action => 'can_start', :conditions => { :method
=> :get }
diff --git a/src/db/migrate/20110602144940_remove_not_null_constraint_on_deployable_id.rb
b/src/db/migrate/20110602144940_remove_not_null_constraint_on_deployable_id.rb
new file mode 100644
index 0000000..d6a5a89
--- /dev/null
+++ b/src/db/migrate/20110602144940_remove_not_null_constraint_on_deployable_id.rb
@@ -0,0 +1,9 @@
+class RemoveNotNullConstraintOnDeployableId< ActiveRecord::Migration
+ def self.up
+ change_column :deployments, :deployable_id, :integer, :null => true
+ end
+
+ def self.down
+ change_column :deployments, :deployable_id, :integer, :null => false
+ end
+end