Hi,
I'm attaching a patch that implements #1605, "As a user, I want to launch a deployment." This happens within the context of a pool, as opposed to a dedicated Deployments tab. (View a pool and press "New Deployment".)
There are two things you should be aware of:
1.) It doesn't actually work. We need to parse the deployable definition (as XML loaded from the provided URL) and then find or build matching images based on the enclosed data. But since that isn't implemented just yet, we just store the contents of the URL in the database and move you to the next page, from which you could launch an image, if only we had any of the needed data.
2.) It breaks existing deployments, which are going away at some point. I get a slew of failing tests as a result. (I'm happy to remove the no-longer-applicable tests, but I first wanted someone to confirm that I'm not going crazy and removing functionality that we actually want to keep.)
So why send it? Because it implements a feature that we're going to need (and soon!), and because I'd rather have eyes on it now than wait until next week when it might have something to hook into to be fully-functional.
-- Matt
Implements https://www.aeolusproject.org/redmine/issues/1605 --- 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 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) { + $('.name_available').html(data == "false" ? "That name is already in use" : "Name available"); + }); + }); + }); 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
On 06/02/2011 10:00 PM, Matt Wagner wrote:
Implements https://www.aeolusproject.org/redmine/issues/1605
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
On Fri, Jun 03, 2011 at 02:30:18PM +0200, Tomas Sedovic 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.
Ah, sorry about the confusion. Without really intending to do so, I ignored the subtasks and just worked on the parent task as a whole. 1607 is implemented here, as are a bunch of things that don't have tasks.
1610 is not done; my understanding is that this is still being ironed out, and is one of the reasons that this interface doesn't actually work.
There is 1 failing RSpec test and a 14 Cucumber features.
Yes, I was hoping for a sanity check here before going through and cleaning up. Much of the old deployment code stops working; I didn't want to remove all that code and the tests without a little reassurance that I wasn't totally off base. I'll pull out some of the now-inapplicable code and fix up the tests.
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?
This is correct. The deployable_xml column in deployments will store a chunk of XML that describes the deployable, as loaded from a URL, instead of storing a separate Deployable object. (Actually parsing this is something Scott's working on as we speak.)
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.
Yes, I had these concerns as well, and am not sure how to proceed. I was encouraged to get this rolling, but I'm not sure how to proceed with actually pushing it yet, as it does a lot of damage short-term. Does anyone have any feedback here? I don't want to push this before it's ready, but I also fear that if we wait until everything is ready, we'll end up hopelessly deadlocked.
It looks to me that the _launch_new.haml and _new.haml partials are virtually identical.
D'oh! They are, but _launch_new should just be deleted. I copied _launch_new into _new and made that the place you go to, well, create a new deployment.
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.
Oh yes, good catch. Thanks.
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 inclined to go with your note about sticking a TODO disclaimer here. It is a potential race condition, but I think the impact is very low -- worst case, someone steals your pool name before you can click "Next", and when you do post the form, you get an error that the name is already taken and must come up with another one.
-- Matt
aeolus-devel@lists.fedorahosted.org