From 16871326818235a90996fff711c61e9a57a14aef Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Thu, 6 Oct 2016 21:29:38 -0700 Subject: [PATCH 1/3] (MODULES-3951) Add explicit check for stringify_facts Without this check, you get incomprehensible results. The is_pe fact is evaluated as true, and so the class bails out. In some cases the info() function isn't loud enough and so it appears to do nothing. This just fails hard and makes it obvious why. --- lib/facter/settings.rb | 6 ++++++ manifests/params.pp | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/lib/facter/settings.rb b/lib/facter/settings.rb index fb002f342..93cd85979 100644 --- a/lib/facter/settings.rb +++ b/lib/facter/settings.rb @@ -12,6 +12,12 @@ end end +Facter.add('puppet_stringify_facts') do + setcode do + Puppet.settings['stringify_facts'] || false + end +end + Facter.add('puppet_sslpaths') do setcode do result = {} diff --git a/manifests/params.pp b/manifests/params.pp index 7cd14996e..344a526e1 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -5,6 +5,10 @@ # class puppet_agent::params { + if $::puppet_stringify_facts { + fail('The puppet_agent class requires stringify_facts to be disabled') + } + # The `is_pe` fact currently works by echoing out the puppet version # and greping for "puppet enterprise". With Puppet 4 and PE 2015.2, there # is no longer a "PE Puppet", and so that fact will no longer work. From 251e8ef8be51eb20fe7c5d94f4eb8b92acf0a263 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Mon, 10 Oct 2016 11:13:39 -0700 Subject: [PATCH 2/3] (MODULES-3951) Add unit test for stringify_facts fact Previously there were no tests for the stringify_facts fact. This commit adds unit tests for this fact by mocking the Puppet.settings values. This commit also adds a test to ensure that catalog compilation fails if stringify_facts evaluates as true. --- spec/classes/puppet_agent_spec.rb | 17 +++++++++++++ spec/unit/facter/settings_spec.rb | 42 +++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/spec/classes/puppet_agent_spec.rb b/spec/classes/puppet_agent_spec.rb index f2056e46a..3cac4dd4d 100644 --- a/spec/classes/puppet_agent_spec.rb +++ b/spec/classes/puppet_agent_spec.rb @@ -172,4 +172,21 @@ def global_facts(facts, os) it { is_expected.to raise_error(Puppet::Error, /Nexenta not supported/) } end end + + context 'stringify_facts is set to true' do + describe 'when puppet_stringify_facts evaluates as true ' do + # Mock a supported agent but with puppet_stringify_facts set to true + let(:facts) {{ + :osfamily => 'windows', + :operatingsystem => '', + :puppet_ssldir => '/dev/null/ssl', + :puppet_config => '/dev/null/puppet.conf', + :architecture => 'i386', + :puppet_stringify_facts => true, + }} + let(:params) { global_params } + + it { is_expected.to raise_error(Puppet::Error, /requires stringify_facts to be disabled/) } + end + end end diff --git a/spec/unit/facter/settings_spec.rb b/spec/unit/facter/settings_spec.rb index ea1830b6f..66da98603 100644 --- a/spec/unit/facter/settings_spec.rb +++ b/spec/unit/facter/settings_spec.rb @@ -18,6 +18,48 @@ end end +describe "puppet_stringify_facts fact for Puppet 4.x+", :unless => /3\./ =~ Puppet.version do + subject { Facter.fact("puppet_stringify_facts".to_sym).value } + after(:each) { Facter.clear } + + describe 'should always be false' do + it { is_expected.to eq(false) } + end +end + +describe "puppet_stringify_facts fact for Puppet 3.x", :if => /3\./ =~ Puppet.version do + subject { Facter.fact("puppet_stringify_facts".to_sym).value } + after(:each) { Facter.clear } + + describe 'when not set in Puppet' do + before(:each) { + mocked_settings = Puppet::Settings.new + Puppet.stubs(:settings).returns(mocked_settings) + } + it { is_expected.to eq(false) } + end + + describe 'when set false in Puppet' do + before(:each) { + mocked_settings = Puppet::Settings.new + mocked_settings.define_settings :main, :stringify_facts => { :default => false, :type => :boolean, :desc => 'mocked stringify_facts setting' } + Puppet.stubs(:settings).returns(mocked_settings) + } + + it { is_expected.to eq(false) } + end + + describe 'when set true in Puppet' do + before(:each) { + mocked_settings = Puppet::Settings.new + mocked_settings.define_settings :main, :stringify_facts => { :default => true, :type => :boolean, :desc => 'mocked stringify_facts setting' } + Puppet.stubs(:settings).returns(mocked_settings) + } + + it { is_expected.to eq(true) } + end +end + describe "puppet_sslpaths fact" do subject { Facter.fact("puppet_sslpaths".to_sym).value } after(:each) { Facter.clear } From 11fae83ac9e664e5109a88abe9c8c3c2b67a167d Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Mon, 10 Oct 2016 14:46:49 -0700 Subject: [PATCH 3/3] Revert "(MODULES-3591) Test if stringify_facts = true on agent" Commit 6fb63a0b6ea was added to detect when stringify_facts is true however this check is now superseded by the `fail` in MODULES-3951 This reverts commit 6fb63a0b6eaba445a69d62fbe9c4fd0870324504. --- manifests/prepare/ssl.pp | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/manifests/prepare/ssl.pp b/manifests/prepare/ssl.pp index 54a2c7b8f..639c0185f 100644 --- a/manifests/prepare/ssl.pp +++ b/manifests/prepare/ssl.pp @@ -23,30 +23,23 @@ 'requestdir' => 'certificate_requests', } - case $::puppet_sslpaths { - Hash: { - $sslpaths.each |String $setting, String $subdir| { - if $::puppet_sslpaths[$setting]['path_exists'] { - file { "${ssl_dir}/${subdir}": - ensure => directory, - source => $::puppet_sslpaths[$setting]['path'], - backup => false, - recurse => true, - } - } - } - - # The only one that's a file, not a directory. - if $::puppet_sslpaths['hostcrl']['path_exists'] { - file { "${ssl_dir}/crl.pem": - ensure => file, - source => $::puppet_sslpaths['hostcrl']['path'], - backup => false - } + $sslpaths.each |String $setting, String $subdir| { + if $::puppet_sslpaths[$setting]['path_exists'] { + file { "${ssl_dir}/${subdir}": + ensure => directory, + source => $::puppet_sslpaths[$setting]['path'], + backup => false, + recurse => true, } } - default: { - fail('$::puppet-sslpaths is not a Hash. Is stringify_facts not set to false in the main section of the agent puppet.conf?') + } + + # The only one that's a file, not a directory. + if $::puppet_sslpaths['hostcrl']['path_exists'] { + file { "${ssl_dir}/crl.pem": + ensure => file, + source => $::puppet_sslpaths['hostcrl']['path'], + backup => false } } }