From 4424e3c5a23cac46066d3439d1da34c1d9a798e8 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Thu, 21 May 2015 10:54:54 +0200 Subject: [PATCH] Fix discovery and an inheritance issue --- .../foreman_salt/minions_controller.rb | 2 +- .../concerns/host_managed_extensions.rb | 7 +++--- .../concerns/hostgroup_extensions.rb | 12 ++-------- test/factories/foreman_salt_factories.rb | 9 ++++---- test/unit/hostgroup_extensions_test.rb | 22 ++++++++++++++++--- 5 files changed, 30 insertions(+), 22 deletions(-) diff --git a/app/controllers/foreman_salt/minions_controller.rb b/app/controllers/foreman_salt/minions_controller.rb index a9c6f529..f4f0ec67 100644 --- a/app/controllers/foreman_salt/minions_controller.rb +++ b/app/controllers/foreman_salt/minions_controller.rb @@ -69,7 +69,7 @@ def load_ajax_vars @minion = Host::Base.authorized(:view_hosts, Host).find_by_id(params[:host_id]) if @minion unless @minion.is_a?(Host::Managed) - @minion = @host.becomes(Host::Managed) + @minion = @minion.becomes(Host::Managed) @minion.type = 'Host::Managed' end @minion.attributes = params[:host] diff --git a/app/models/foreman_salt/concerns/host_managed_extensions.rb b/app/models/foreman_salt/concerns/host_managed_extensions.rb index cd9b6311..39d9676e 100644 --- a/app/models/foreman_salt/concerns/host_managed_extensions.rb +++ b/app/models/foreman_salt/concerns/host_managed_extensions.rb @@ -33,10 +33,9 @@ def params_with_salt_proxy end def salt_modules_for_enc - return [] unless self.salt_environment - - hostgroup_modules = self.hostgroup ? self.hostgroup.all_salt_modules : [] - self.salt_environment.salt_modules.where(:id => self.salt_modules + hostgroup_modules).pluck(:name) + return [] unless salt_environment + modules = salt_modules + (hostgroup ? hostgroup.all_salt_modules : []) + ForemanSalt::SaltModule.in_environment(salt_environment).where(:id => modules).pluck(:name).uniq end def salt_master diff --git a/app/models/foreman_salt/concerns/hostgroup_extensions.rb b/app/models/foreman_salt/concerns/hostgroup_extensions.rb index 29fd775f..5abd0c7e 100644 --- a/app/models/foreman_salt/concerns/hostgroup_extensions.rb +++ b/app/models/foreman_salt/concerns/hostgroup_extensions.rb @@ -16,11 +16,7 @@ module HostgroupExtensions end def all_salt_modules - if ancestry.present? - (self.salt_modules + self.inherited_salt_modules).uniq - else - self.salt_modules - end + ForemanSalt::SaltModule.in_environment(salt_environment).where(:id => salt_module_ids + inherited_salt_module_ids) end def inherited_salt_modules @@ -28,11 +24,7 @@ def inherited_salt_modules end def inherited_salt_module_ids - if ancestry.present? - self.class.sort_by_ancestry(ancestors.reject { |ancestor| ancestor.salt_module_ids.empty? }).map(&:salt_module_ids).inject(&:+).uniq - else - [] - end + ancestors.map(&:salt_module_ids).flatten.uniq end def salt_proxy diff --git a/test/factories/foreman_salt_factories.rb b/test/factories/foreman_salt_factories.rb index 9826a857..5be7b135 100644 --- a/test/factories/foreman_salt_factories.rb +++ b/test/factories/foreman_salt_factories.rb @@ -4,24 +4,25 @@ end factory :salt_environment, :class => 'ForemanSalt::SaltEnvironment' do - sequence(:name) { |n| "module#{n}" } + sequence(:name) { |n| "environment#{n}" } end end FactoryGirl.modify do factory :host do trait :with_salt_proxy do - salt_proxy { FactoryGirl.create :smart_proxy, :with_salt_feature } + salt_proxy { FactoryGirl.build :smart_proxy, :with_salt_feature } end end factory :hostgroup do trait :with_salt_proxy do - salt_proxy { FactoryGirl.create :smart_proxy, :with_salt_feature } + salt_proxy { FactoryGirl.build :smart_proxy, :with_salt_feature } end trait :with_salt_modules do - salt_modules { FactoryGirl.create_list :salt_module, 10 } + salt_environment { FactoryGirl.build :salt_environment } + salt_modules { FactoryGirl.create_list :salt_module, 10, :salt_environments => [self.salt_environment] } end end diff --git a/test/unit/hostgroup_extensions_test.rb b/test/unit/hostgroup_extensions_test.rb index 126fb8be..58c7632a 100644 --- a/test/unit/hostgroup_extensions_test.rb +++ b/test/unit/hostgroup_extensions_test.rb @@ -25,16 +25,32 @@ class HostgroupExtensionsTest < ActiveSupport::TestCase end test 'child and parent salt modules are combined' do + environment = FactoryGirl.create :salt_environment + parent = FactoryGirl.create :hostgroup, :with_salt_modules, :salt_environment => environment + child = FactoryGirl.create :hostgroup, :with_salt_modules, :salt_environment => environment, :parent => parent + + total = parent.salt_modules.count + child.salt_modules.count + assert_equal total, child.all_salt_modules.count + end + + test 'child doesnt get modules from outside its environment' do parent = FactoryGirl.create :hostgroup, :with_salt_modules child = FactoryGirl.create :hostgroup, :with_salt_modules, :parent => parent - assert_equal 10, (child.salt_modules - parent.salt_modules).length + assert_equal child.salt_modules.count, child.all_salt_modules.count end - test 'second child inherits from parent' do + test 'inheritance when only parent has modules' do parent = FactoryGirl.create :hostgroup, :with_salt_modules child_one = FactoryGirl.create :hostgroup, :parent => parent child_two = FactoryGirl.create :hostgroup, :parent => child_one - assert_equal [], parent.all_salt_modules - child_two.all_salt_modules + assert_blank parent.all_salt_modules - child_two.all_salt_modules + end + + test 'inheritance when no parents have modules' do + parent = FactoryGirl.create :hostgroup + child_one = FactoryGirl.create :hostgroup, :parent => parent + child_two = FactoryGirl.create :hostgroup, :with_salt_modules, :parent => child_one + assert child_two.all_salt_modules.any? end end end