On Wed, Apr 20, 2011 at 01:06:18PM +0200, Jan Provaznik wrote:
On 04/20/2011 12:53 PM, Tomas Sedovic wrote:
> On 04/20/2011 11:20 AM, jtomasek(a)redhat.com wrote:
>> From: Jiri Tomasek<jtomasek(a)redhat.com>
>>
>> ---
>> .../controllers/admin/pool_families_controller.rb | 29
++++++++++++++++++-
>> src/app/views/admin/pool_families/_list.haml | 2 +-
>> .../admin/pool_families/_provider_accounts.haml | 25 +++++++++++++++++
>> src/config/routes.rb | 2 +-
>> src/features/pool_family.feature | 22 +++++++++++++++
>> src/features/step_definitions/pool_family_steps.rb | 23 +++++++++++++++-
>> src/features/support/paths.rb | 3 ++
>> 7 files changed, 101 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/app/controllers/admin/pool_families_controller.rb
b/src/app/controllers/admin/pool_families_controller.rb
>> index af53aac..ec21c53 100644
>> --- a/src/app/controllers/admin/pool_families_controller.rb
>> +++ b/src/app/controllers/admin/pool_families_controller.rb
>> @@ -2,6 +2,7 @@ class Admin::PoolFamiliesController< ApplicationController
>> before_filter :require_user
>> before_filter :set_params_and_header, :only => [:index, :show]
>> before_filter :load_pool_families, :only =>[:show]
>> + before_filter :load_tab_captions_and_details_tab, :only => [:show]
>>
>> def index
>> @search_term = params[:q]
>> @@ -50,8 +51,7 @@ class Admin::PoolFamiliesController<
ApplicationController
>> def show
>> @pool_family = PoolFamily.find(params[:id])
>> @url_params = params.clone
>> - @tab_captions = ['Properties', 'History',
'Permissions', 'Provider Accounts', 'Pools']
>> - @details_tab = params[:details_tab].blank? ? 'properties' :
params[:details_tab]
>> +
>> respond_to do |format|
>> format.js do
>> if @url_params.delete :details_pane
>> @@ -63,6 +63,15 @@ class Admin::PoolFamiliesController<
ApplicationController
>> end
>> end
>>
>> + def add_provider_account
>> + @pool_family = PoolFamily.find(params[:id])
>> + @provider_account = ProviderAccount.find(params[:provider_account_id])
>> +
>> + @pool_family.provider_accounts<< @provider_account
>> + flash[:notice] = "Provider Account has been added"
>> + redirect_to admin_pool_family_path(@pool_family, :details_tab =>
'provider_accounts')
>> + end
>> +
>> def multi_destroy
>> deleted = []
>> not_deleted = []
>> @@ -82,8 +91,24 @@ class Admin::PoolFamiliesController<
ApplicationController
>> redirect_to admin_pool_families_path
>> end
>>
>> + def multi_destroy_provider_accounts
>> + @pool_family = PoolFamily.find(params[:pool_family_id])
>> +
>> + ProviderAccount.find(params[:provider_account_selected]).each do
|provider_account|
>> + @pool_family.provider_accounts.delete provider_account
>> + end
>> +
>> + redirect_to admin_pool_family_path(@pool_family, :details_tab =>
'provider_accounts')
>> + end
>> +
>> protected
>>
>> + def load_tab_captions_and_details_tab
>> + @tab_captions = ['Properties', 'History',
'Permissions', 'Provider Accounts', 'Pools']
>> + @details_tab = params[:details_tab].blank? ? 'properties' :
params[:details_tab]
>> + @provider_accounts_header = [{ :name => "Provider Account",
:sort_attr => :name}]
>> + end
>> +
>> def set_params_and_header
>> @url_params = params.clone
>> @header = [{ :name => "Name", :sort_attr => :name},
>> diff --git a/src/app/views/admin/pool_families/_list.haml
b/src/app/views/admin/pool_families/_list.haml
>> index a824676..6250c66 100644
>> --- a/src/app/views/admin/pool_families/_list.haml
>> +++ b/src/app/views/admin/pool_families/_list.haml
>> @@ -8,7 +8,7 @@
>> %span> ,
>> = link_to "None", @url_params.merge(:select =>
'none')
>>
>> - %table#pool_famlies_table
>> + %table#pool_families_table
>> = sortable_table_header @header
>> - unless @pool_families.blank?
>> - @pool_families.each do |pool_family|
>> diff --git a/src/app/views/admin/pool_families/_provider_accounts.haml
b/src/app/views/admin/pool_families/_provider_accounts.haml
>> index e69de29..358e3d2 100644
>> --- a/src/app/views/admin/pool_families/_provider_accounts.haml
>> +++ b/src/app/views/admin/pool_families/_provider_accounts.haml
>> @@ -0,0 +1,25 @@
>> +%h3
>> + Provider Accounts for
>> + = @pool_family.name
>> +
>> +- unless @pool_family.provider_accounts.blank?
>> + - form_tag do
>> + = restful_submit_tag "Remove selected", 'destroy',
multi_destroy_provider_accounts_admin_pool_families_path(:pool_family_id =>
@pool_family.id), 'DELETE', :id => 'delete_button'
>> + = #submit_tag "Remove selected"
>> +
>> + %table#provider_accounts_table
>> + = sortable_table_header @provider_accounts_header
>> + - @pool_family.provider_accounts.each do |provider_account|
>> + %tr
>> + %td
>> + - selected = @url_params[:select_provider_accounts] ==
'all'
>> + %input{:name => "provider_account_selected[]", :type
=> "checkbox", :value => provider_account.id, :id =>
"provider_account_checkbox_#{provider_account.id}", :checked => selected }
>> + = provider_account.name
>> +
>> +- unless @pool_family.provider_accounts.count == ProviderAccount.all.count
>> + - form_for :pool_family, :url =>
add_provider_account_admin_pool_families_path(:id => @pool_family.id) do |f|
>> + - unless @pool_family.provider_accounts.blank?
>> + = select_tag :provider_account_id,
options_for_select(ProviderAccount.find(:all, :conditions => [ "id NOT IN
(?)", @pool_family.provider_account_ids ]).collect{ |pa| [pa.name, pa.id] })
>> + - else
>> + = select_tag :provider_account_id,
options_for_select(ProviderAccount.all.collect{ |pa| [pa.name, pa.id] })
>> + = f.submit "Add to #{(a)pool_family.name}"
>> diff --git a/src/config/routes.rb b/src/config/routes.rb
>> index 162260c..6aa9c8c 100644
>> --- a/src/config/routes.rb
>> +++ b/src/config/routes.rb
>> @@ -58,7 +58,7 @@ ActionController::Routing::Routes.draw do |map|
>>
>> r.resources :roles, :collection => { :multi_destroy => :delete
}
>> r.resources :settings, :collection => { :self_service => :get,
:general_settings => :get }
>> - r.resources :pool_families, :collection => { :multi_destroy =>
:delete }
>> + r.resources :pool_families, :collection => { :multi_destroy =>
:delete, :add_provider_account => :post, :multi_destroy_provider_accounts =>
:delete }
>> r.resources :realms, :collection => { :multi_destroy => :delete
}
>> r.resources :realm_mappings, :collection => { :multi_destroy =>
:delete }
>> end
>> diff --git a/src/features/pool_family.feature
b/src/features/pool_family.feature
>> index 6795d42..fd4eb55 100644
>> --- a/src/features/pool_family.feature
>> +++ b/src/features/pool_family.feature
>> @@ -78,3 +78,25 @@ Feature: Pool Families
>> Then I should see "first_family"
>> And I should see "second_family"
>> And I should see "third_family"
>> +
>> + Scenario: Add provider account to pool family
>> + Given there is a pool family named "testpoolfamily"
>> + And there is a provider named "testprovider"
>> + And there is a provider account named "testaccount"
>> + And I am on the pool family provider accounts page
>> + Then I should see "Provider Accounts for"
>> + When I select "testaccount" from "provider_account_id"
>> + And I press "pool_family_submit"
>> + Then there should be 1 provider accounts assigned to
"testpoolfamily"
>> + And I should see "testaccount"
>> +
>> + Scenario: Remove provider account from pool family
>> + Given there is a pool family named "testpoolfamily"
>> + And there is a provider named "testprovider"
>> + And there is a provider account named "testaccount"
>> + And there is a provider account "testaccount" related to pool
family "testpoolfamily"
>> + And I am on the pool family provider accounts page
>> + Then I should see "testaccount"
>> + When I check "testaccount" provider account
>> + And I press "Remove selected"
>> + Then there should not exist a provider account assigned to
"testpoolfamily"
>> diff --git a/src/features/step_definitions/pool_family_steps.rb
b/src/features/step_definitions/pool_family_steps.rb
>> index 9585a21..716d722 100644
>> --- a/src/features/step_definitions/pool_family_steps.rb
>> +++ b/src/features/step_definitions/pool_family_steps.rb
>> @@ -5,7 +5,7 @@ Given /^there are these pool families:$/ do |table|
>> end
>>
>> Given /^there is a pool family named "([^\"]*)"$/ do |name|
>> - @provider = Factory(:pool_family, :name => name)
>> + @pool_family = Factory(:pool_family, :name => name)
>> end
>>
>> Given /^there is not a pool family named "([^"]*)"$/ do
|name|
>> @@ -25,3 +25,24 @@ end
>> Then /^there should not exist a pool family named "([^\"]*)"$/
do |name|
>> PoolFamily.find_by_name(name).should be_nil
>> end
>> +
>> +Then /^there should be (\d+) provider accounts assigned to
"([^\"]*)"$/ do |count,name|
>> + @pool_family = PoolFamily.find_by_name(name)
>> + @pool_family.provider_accounts.count == count
>> +end
>> +
>> +Given /^there is a provider account "([^"]*)" related to pool
family "([^"]*)"$/ do |provider_account, pool_family|
>> + @pool_family = PoolFamily.find_by_name(pool_family)
>> + @provider_account = ProviderAccount.find_by_label(provider_account)
>> + @pool_family.provider_accounts<< @provider_account
>> +end
>> +
>> +When /^I check "([^"]*)" provider account$/ do |label|
>> + provider_account = ProviderAccount.find_by_label(label)
>> + check("provider_account_checkbox_#{provider_account.id}")
>> +end
>> +
>> +Then /^there should not exist a provider account assigned to
"([^"]*)"$/ do |name|
>> + @pool_family = PoolFamily.find_by_name(name)
>> + @pool_family.provider_accounts.count == 0
>> +end
>> diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb
>> index 5af9b34..a313aa0 100644
>> --- a/src/features/support/paths.rb
>> +++ b/src/features/support/paths.rb
>> @@ -83,6 +83,9 @@ module NavigationHelpers
>> when /the template builds page/
>> url_for image_factory_template_path(@template, :details_tab =>
'builds')
>>
>> + when /the pool family provider accounts page/
>> + url_for admin_pool_family_path(@pool_family, :details_tab =>
'provider_accounts')
>> +
>> when /the templates page/
>> templates_path
>>
>
> Hey Jiri,
>
> It looks good, but I'm afraid there's been some misunderstanding
> regarding how pool families work.
>
> As far as I'm aware, a provider account must be associated with *exactly
> one* pool family at all times.
>
> That means:
>
> 1) User should not be able to remove an account from the default pool family
>
> 2) When user removes an account from a non-default pool family, it
> should be moved to the default one automatically
>
> 3) When user adds a provider account to a pool family, it must be
> removed from the family it was associated with previously
>
> Ideally, the patch should enforce these rules and have tests for them.
>
> However.
>
> It's entirely possible that my understanding of the pool families is
> incorrect. We should clarify this with Scott once he gets online. If
> your patch mimics the correct behaviour, I'll ack and push it immediatelly.
>
> Take care,
> Thomas
Hm, adding my understanding too:
- a provider account for a pool family is optional (if there is no
account, condor chooses from all accounts)
- there can be multiple accounts for the pool family (then condor
chooses from accounts defined for the pool family)
- account can be associated with multiple pool families
This is correct except for the first point. If there's no provider
account for pool family, then that pool family has 0 capacity -- in
other words, you can't launch any instances there.
The main use case for these things (pool families) is to segment
providers for different use cases. So for example the dev PF might be
backed only by cheap providers with low QOS, while the prod PF would
have the expensive providers.
Take care,
--H
--
== Hugh Brock, hbrock(a)redhat.com ==
== Engineering Manager, Cloud BU ==
== Aeolus Project: Manage virtual infrastructure across clouds. ==
==
http://aeolusproject.org ==
"I know that you believe you understand what you think I said, but I’m
not sure you realize that what you heard is not what I meant."
--Robert McCloskey