Signed-off-by: Scott Seago sseago@redhat.com --- src/app/controllers/instance_controller.rb | 18 ++--------- src/app/controllers/portal_pool_controller.rb | 13 +++++-- src/app/controllers/provider_controller.rb | 2 +- src/app/models/instance.rb | 25 +++++++++++++-- src/app/models/instance_task.rb | 7 +++- src/app/models/portal_pool.rb | 40 +++++++++++++++++++----- src/app/models/provider.rb | 7 ++++- src/app/views/instance/new.html.erb | 20 ++++++------ src/app/views/instance/show.html.erb | 2 +- src/app/views/portal_pool/show.html.erb | 6 ++-- 10 files changed, 92 insertions(+), 48 deletions(-)
diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb index 24127e1..8893286 100644 --- a/src/app/controllers/instance_controller.rb +++ b/src/app/controllers/instance_controller.rb @@ -32,12 +32,11 @@ class InstanceController < ApplicationController def show @instances = Instance.find(:all, :conditions => {:portal_pool_id => params[:id]}) @pool = PortalPool.find(params[:id]) - @provider = @pool.cloud_account.provider end
def new @instance = Instance.new({:portal_pool_id => params[:id]}) - _new(params[:provider]) + @pool = PortalPool.find(params[:id]) end
def create @@ -46,18 +45,18 @@ class InstanceController < ApplicationController #FIXME: This should probably be in a transaction if @instance.save
- @task = InstanceTask.new({:user => @instance.portal_pool.cloud_account.username, + @task = InstanceTask.new({:user => current_user, :task_target => @instance, :action => InstanceTask::ACTION_CREATE}) if @task.save flash[:notice] = "Instance added." redirect_to :controller => "portal_pool", :action => 'show', :id => @instance.portal_pool_id else - _new(params[:provider_id]) + @pool = @instance.portal_pool render :action => 'new' end else - _new(params[:provider_id]) + @pool = PortalPool.find(@instance.portal_pool_id) render :action => 'new' end end @@ -82,13 +81,4 @@ class InstanceController < ApplicationController def delete end
- private - - def _new(provider) - @flavors = Flavor.find(:all, :conditions => {:provider_id => provider}) - @images = Image.find(:all, :conditions => {:provider_id => provider}) - @realms = Realm.find(:all, :conditions => {:provider_id => provider}) - @provider_id = provider - end - end diff --git a/src/app/controllers/portal_pool_controller.rb b/src/app/controllers/portal_pool_controller.rb index b06041c..ae45d02 100644 --- a/src/app/controllers/portal_pool_controller.rb +++ b/src/app/controllers/portal_pool_controller.rb @@ -27,10 +27,9 @@ class PortalPoolController < ApplicationController end
def show - @instances = Instance.find(:all, :conditions => {:portal_pool_id => params[:id]}) #FIXME: clean this up, many error cases here @pool = PortalPool.find(params[:id]) - @provider = @pool.cloud_account.provider + @instances = @pool.instances end
def new @@ -41,10 +40,16 @@ class PortalPoolController < ApplicationController
def create @account = CloudAccount.find_or_create(params[:account]) + #FIXME: owner is set to current user for self-service account creation, + # but in the more general case we need a way for the admin to pick + # a user + # FIXME: automatically grant owner access rights to the pool + params[:portal_pool][:owner_id] = @current_user.id #FIXME: This should probably be in a transaction if @account.save - @portal_pool = @account.portal_pools.build(params[:portal_pool]) - if @portal_pool.save && @portal_pool.populate_realms_and_images + @portal_pool = PortalPool.new(params[:portal_pool]) + @portal_pool.cloud_accounts << @account + if @portal_pool.save && @portal_pool.populate_realms_and_images([@account]) flash[:notice] = "Pool added." redirect_to :action => 'show', :id => @portal_pool.id else diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 947c63c..c969797 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -32,7 +32,7 @@ class ProviderController < ApplicationController
def new @provider = Provider.new(params[:provider]) - if request.post? && @provider.save && @provider.populate_flavors + if request.post? && @provider.save && @provider.populate_hardware_profiles flash[:notice] = "Provider added." redirect_to :action => "show", :id => @provider end diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 83d463a..a528e4e 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -39,9 +39,9 @@ class Instance < ActiveRecord::Base validates_presence_of :name validates_uniqueness_of :name, :scope => :portal_pool_id
- # FIXME: for now, flavor is required, realm is optional, although for RHEV-M, - # flavor may be optional too - validates_presence_of :flavor_id + # FIXME: for now, hardware profile is required, realm is optional, although for RHEV-M, + # hardware profile may be optional too + validates_presence_of :hardware_profile_id validates_presence_of :image_id
STATE_NEW = "new" @@ -83,5 +83,24 @@ class Instance < ActiveRecord::Base return task end
+ def front_end_realm + if cloud_account.nil? + "" + else + cloud_account.provider.name + Realm::AGGREGATOR_REALM_PROVIDER_DELIMITER + + cloud_account.username + (realm.nil? ? "" : + (Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER + + realm.name)) + end + end + + def front_end_realm=(realm_name) + provider_name, tmpstr = realm_name.split(Realm::AGGREGATOR_REALM_PROVIDER_DELIMITER,2) + account_name, realm_name = tmpstr.split(Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER,2) + provider = Provider.find_by_name(provider_name) + self.cloud_account = provider.cloud_accounts.find_by_username(account_name) + self.realm = provider.realms.find_by_name(realm_name) unless realm_name.nil? + end +
end diff --git a/src/app/models/instance_task.rb b/src/app/models/instance_task.rb index 88b2e5f..9da110b 100644 --- a/src/app/models/instance_task.rb +++ b/src/app/models/instance_task.rb @@ -44,8 +44,11 @@ class InstanceTask < Task # automatic transitions from transient states. def self.valid_actions_for_instance_state(state, instance, user=nil) actions = [] - c_state = instance.portal_pool.cloud_account.connect.instance_state(state) - if c_state + # FIXME: cloud_account won't always be set here, but we're requiring + # front end realm for now. + cloud_account = instance.cloud_account + c_state = cloud_account.connect.instance_state(state) unless cloud_account.nil? + if !cloud_account.nil? and c_state transitions = c_state.transitions transitions.each do |transition| # FIXME if we allow actions based on the expected state after diff --git a/src/app/models/portal_pool.rb b/src/app/models/portal_pool.rb index c88e4df..1fd9dd4 100644 --- a/src/app/models/portal_pool.rb +++ b/src/app/models/portal_pool.rb @@ -29,12 +29,11 @@ class PortalPool < ActiveRecord::Base has_many :hardware_profiles, :dependent => :destroy
- validates_presence_of :cloud_account_id
validates_presence_of :owner_id validates_presence_of :name validates_uniqueness_of :name, :scope => :owner_id - validates_uniqueness_of :exported_as + validates_uniqueness_of :exported_as, :if => :exported_as
has_many :permissions, :as => :permission_object, :dependent => :destroy, :include => [:role], @@ -44,16 +43,20 @@ class PortalPool < ActiveRecord::Base realm_list = [] cloud_accounts.each do |cloud_account| prefix = cloud_account.provider.name + - AGGREGATOR_REALM_PROVIDER_DELIMITER + cloud_account.username + Realm::AGGREGATOR_REALM_PROVIDER_DELIMITER + + cloud_account.username + realm_list << prefix cloud_account.provider.realms.each do |realm| - realm_list << prefix + AGGREGATOR_REALM_ACCOUNT_DELIMITER + realm.name + realm_list << prefix + Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER + + realm.name end end + realm_list end
- - def populate_realms_and_images - cloud_accounts.each do |cloud_account| + # FIXME: for already-mapped accounts, update rather than add new + def populate_realms_and_images(accounts=cloud_accounts) + accounts.each do |cloud_account| client = cloud_account.connect realms = client.realms if client.driver_name == "ec2" @@ -77,10 +80,11 @@ class PortalPool < ActiveRecord::Base end images.each do |image| #ignore if it exists - #FIXME: we need to handle keeping in sync forupdates as well as + #FIXME: we need to handle keeping in sync for updates as well as # account permissions - unless Image.find_by_external_key_and_provider_id(image.id, + ar_image = Image.find_by_external_key_and_provider_id(image.id, cloud_account.provider.id) + unless ar_image ar_image = Image.new(:external_key => image.id, :name => image.name ? image.name : (image.description ? image.description : @@ -89,6 +93,24 @@ class PortalPool < ActiveRecord::Base :provider_id => cloud_account.provider.id) ar_image.save! end + # FIXME: what do we ue for external_key values for front end images? + # FIXME: this will break if multiple imported accounts (from different + # providers) use the same external key + front_end_image = Image.new(:external_key => ar_image.external_key, + :name => ar_image.name, + :architecture => ar_image.architecture, + :portal_pool_id => id) + front_end_image.save! + end + cloud_account.provider.hardware_profiles.each do |hardware_profile| + front_hardware_profile = HardwareProfile.new(:external_key => + hardware_profile.external_key, + :name => hardware_profile.name, + :memory => hardware_profile.memory, + :storage => hardware_profile.storage, + :architecture => hardware_profile.architecture, + :portal_pool_id => id) + front_hardware_profile.save! end end end diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb index 14ffd80..650cc4d 100644 --- a/src/app/models/provider.rb +++ b/src/app/models/provider.rb @@ -48,7 +48,8 @@ class Provider < ActiveRecord::Base end
def populate_hardware_profiles - hardware_profiles = connect.hardware_profiles + # FIXME: once API has hw profiles, change the below + hardware_profiles = connect.flavors # FIXME: this should probably be in the same transaction as provider.save self.transaction do hardware_profiles.each do |hardware_profile| @@ -63,6 +64,10 @@ class Provider < ActiveRecord::Base end end
+ def portal_pools + cloud_accounts.collect {|account| account.portal_pools}.flatten + end + # TODO: implement or remove - this is meant to contain a hash of # supported cloud_types to use in populating form, though if we # infer that field, we don't need this. diff --git a/src/app/views/instance/new.html.erb b/src/app/views/instance/new.html.erb index 7acb09e..1ca2024 100644 --- a/src/app/views/instance/new.html.erb +++ b/src/app/views/instance/new.html.erb @@ -5,25 +5,25 @@ <% form_tag :action => 'create' do-%> <ul> <li><label>Name<span>Name for this new Instance</span></label><%= text_field :instance, :name, :class => "txtfield" %></li> - <% if @flavors.size > 0 %> - <li><label>Flavor<span>Pick your flavor</span></label> - <%= select("instance", "flavor_id", @flavors.collect {|p| [ p.name, p.id ] }, { :include_blank => true }) %> + <% if @pool.hardware_profiles.size > 0 %> + <li><label>Hardware Profile<span>Pick your hardware profile</span></label> + <%= select("instance", "hardware_profile_id", + @pool.hardware_profiles.collect {|p| [ p.name, p.id ] }, + { :include_blank => true }) %> </li> <% end %> - <% if @images.size > 0 %> + <% if @pool.images.size > 0 %> <li><label>Image<span>Choose a bundled image to use</span></label> - <%= select("instance", "image_id", @images.collect {|i| [ truncate(i.name, 29, '...'), i.id ] }, { :include_blank => true }) %> + <%= select("instance", "image_id", @pool.images.collect {|i| [ truncate(i.name, 29, '...'), i.id ] }, { :include_blank => true }) %> </li> <% end %> - <% if @realms.size > 0 %> + <% if @pool.realms.size > 0 %> <li><label>Realm<span>Choose a realm</span></label> - <%= select("instance", "realm_id", @realms.collect {|r| [ r.name, r.id ] }, { :include_blank => true }) %> + <%= select("instance", "front_end_realm", @pool.realms, { :include_blank => false }) %> </li> <% end %> </ul> - <input id="instance_provider_id" type="hidden" value="<%=@provider_id%>" name="provider_id"/> - <%#= hidden_field :provider, :provider_id %> <%= hidden_field :instance, :portal_pool_id %> <%= submit_tag "Save", :class => "submit" %> <% end %> -</div> \ No newline at end of file +</div> diff --git a/src/app/views/instance/show.html.erb b/src/app/views/instance/show.html.erb index 2a550d8..a2bfe2f 100644 --- a/src/app/views/instance/show.html.erb +++ b/src/app/views/instance/show.html.erb @@ -26,4 +26,4 @@ </tbody> </table> <% end %> -<%= link_to "Add a new instance", :controller => "instance", :action => "new", :id => @pool, :provider => @provider%> +<%= link_to "Add a new instance", :controller => "instance", :action => "new", :id => @pool %> diff --git a/src/app/views/portal_pool/show.html.erb b/src/app/views/portal_pool/show.html.erb index 998b27f..d649199 100644 --- a/src/app/views/portal_pool/show.html.erb +++ b/src/app/views/portal_pool/show.html.erb @@ -7,7 +7,7 @@ <th scope="col">Actions</th> <th scope="col">Name</th> <th scope="col">State</th> - <th scope="col">Flavor</th> + <th scope="col">Hardware Profile</th> <th scope="col">Image</th> </tr> </thead> @@ -28,11 +28,11 @@ </td> <td><%= instance.name %></td> <td><%= instance.state %></td> - <td><%= instance.flavor.name %></td> + <td><%= instance.hardware_profile.name %></td> <td><%= instance.image.name %></td> </tr> <% } %> </tbody> </table> <% end %> -<%= link_to "Add a new instance", {:controller => "instance", :action => "new", :id => @pool, :provider => @provider}, :class=>"actionlink"%> +<%= link_to "Add a new instance", {:controller => "instance", :action => "new", :id => @pool}, :class=>"actionlink"%>
One minor suggestion/question below. Other than that and other minor points in previous responses, ACK to the whole series, everything seems to be working as it should.
On Wed, Feb 17, 2010 at 4:16 PM, Scott Seago sseago@redhat.com wrote:
Signed-off-by: Scott Seago sseago@redhat.com
<snip>
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 83d463a..a528e4e 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -39,9 +39,9 @@ class Instance < ActiveRecord::Base validates_presence_of :name validates_uniqueness_of :name, :scope => :portal_pool_id
- # FIXME: for now, flavor is required, realm is optional, although for RHEV-M,
- # flavor may be optional too
- validates_presence_of :flavor_id
- # FIXME: for now, hardware profile is required, realm is optional, although for RHEV-M,
- # hardware profile may be optional too
- validates_presence_of :hardware_profile_id
validates_presence_of :image_id
STATE_NEW = "new" @@ -83,5 +83,24 @@ class Instance < ActiveRecord::Base return task end
- def front_end_realm
Two questions here: 1. Do we need to return ""? Could we just not set anything? 2a. If we do, why not phrase this as: return "" if cloud_account.nil? <build string stuff> 2b. If it is not needed (which is my thought), perhaps phrase as: unless cloud_account.nil? <build string stuff>
- if cloud_account.nil?
- ""
- else
- cloud_account.provider.name + Realm::AGGREGATOR_REALM_PROVIDER_DELIMITER +
- cloud_account.username + (realm.nil? ? "" :
- (Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER +
- realm.name))
- end
- end
<snip>
deltacloud-devel@lists.fedorahosted.org