Avoid using global variables

Global variables are unspecified and unreliable in where they come from.
Instead, built in variables like $facts, $trusted and $server_facts are
better sources.

Author: ekohl

manifests/config.pp

@@ -43,7 +43,7 @@
   }
   if $use_srv_records {
     unless $srv_domain {
-      fail('$::domain fact found to be undefined and $srv_domain is undefined')
+      fail('domain fact found to be undefined and $srv_domain is undefined')
     }
     puppet::config::main{
       'use_srv_records': value => true;

manifests/params.pp

@@ -34,19 +34,15 @@
   $dns_alt_names       = []
   $use_srv_records     = false
 
-  if defined('$::domain') {
-    $srv_domain = $facts['networking']['domain']
-  } else {
-    $srv_domain = undef
-  }
+  $srv_domain = fact('networking.domain')
 
   # lint:ignore:puppet_url_without_modules
   $pluginsource        = 'puppet:///plugins'
   $pluginfactsource    = 'puppet:///pluginfacts'
   # lint:endignore
   $classfile           = '$statedir/classes.txt'
   $syslogfacility      = undef
-  $environment         = $::environment
+  $environment         = $server_facts['environment']
 
   # aio_agent_version is a core fact that's empty on non-AIO
   $aio_package      = fact('aio_agent_version') =~ String[1]
@@ -199,13 +195,10 @@
 
   # Will this host be a puppet agent ?
   $agent                      = true
-  $client_certname            = $::clientcert
+  $client_certname            = $trusted['certname']
 
-  if defined('$::puppetmaster') {
-    $puppetmaster             = $::puppetmaster
-  } else {
-    $puppetmaster             = undef
-  }
+  # Set by the Foreman ENC
+  $puppetmaster               = getvar('puppetmaster')
 
   # Hashes containing additional settings
   $additional_settings        = {}
@@ -220,7 +213,7 @@
   $server_external_nodes           = "${dir}/node.rb"
   $server_trusted_external_command = undef
   $server_request_timeout          = 60
-  $server_certname                 = $::clientcert
+  $server_certname                 = $trusted['certname']
   $server_strict_variables         = false
   $server_http                     = false
   $server_http_port                = 8139
@@ -262,7 +255,7 @@
 
   $server_storeconfigs = false
 
-  $puppet_major = regsubst($::puppetversion, '^(\d+)\..*$', '\1')
+  $puppet_major = regsubst($facts['puppetversion'], '^(\d+)\..*$', '\1')
 
   if ($facts['os']['family'] =~ /(FreeBSD|DragonFly)/) {
     $server_package = "puppetserver${puppet_major}"

manifests/server/config.pp

@@ -173,19 +173,19 @@
         command => "${puppet::puppetserver_cmd} ca migrate",
         creates => $puppet::server::cadir,
         onlyif  => "test -d '${puppet::server::ssl_dir}/ca' && ! test -L '${puppet::server::ssl_dir}'",
-        path    => $::path,
+        path    => $facts['path'],
         before  => Exec['puppet_server_config-generate_ca_cert'],
       }
     }
   } elsif $puppet::server::ca_crl_sync {
     # If not a ca AND sync the crl from the ca master
-    if defined('$::servername') {
+    if $server_facts['servername'] {
       file { $puppet::server::ssl_ca_crl:
         ensure  => file,
         owner   => $puppet::server::user,
         group   => $puppet::server::group,
         mode    => '0644',
-        content => file($::settings::cacrl, $::settings::hostcrl, '/dev/null'),
+        content => file($settings::cacrl, $settings::hostcrl, '/dev/null'),
       }
     }
   }

spec/classes/puppet_config_spec.rb

@@ -128,7 +128,7 @@
         context 'domain fact is unset' do
           let(:facts) { override_facts(super(), networking: {domain: nil}) }
 
-          it { is_expected.to raise_error(Puppet::Error, /\$::domain fact found to be undefined and \$srv_domain is undefined/) }
+          it { is_expected.to raise_error(Puppet::Error, /domain fact found to be undefined and \$srv_domain is undefined/) }
         end
 
         context 'is overriden via param' do
@@ -142,12 +142,9 @@
       end
 
       describe 'client_certname' do
-        context 'with client_certname => $::clientcert' do
-          let :facts do
-            # rspec-puppet(-facts) doesn't mock this
-            super().merge(clientcert: 'client.example.com')
-          end
+        let(:node) { 'client.example.com' }
 
+        context 'with client_certname => trusted certname' do
           it { is_expected.to contain_puppet__config__main('certname').with_value('client.example.com') }
         end
 

spec/classes/puppet_server_spec.rb

@@ -170,18 +170,11 @@
         let(:facts) do
           override_facts(super(),
             networking: {fqdn: 'PUPPETMASTER.example.com'},
-            # clientcert is always lowercase by Puppet design
-            clientcert: 'puppetmaster.example.com'
           )
         end
 
         it { should compile.with_all_deps }
-
-        it 'should use lowercase certificates' do
-          should contain_class('puppet::server::puppetserver')
-            .with_server_ssl_cert("#{ssldir}/certs/puppetmaster.example.com.pem")
-            .with_server_ssl_cert_key("#{ssldir}/private_keys/puppetmaster.example.com.pem")
-        end
+        it { should contain_class('puppet').with_server_foreman_url('https://puppetmaster.example.com') }
       end
 
       describe 'with ip parameter' do
@@ -503,6 +496,8 @@
           end
 
           it 'should not sync the crl' do
+            # https://github.com/puppetlabs/rspec-puppet/issues/37
+            pending("rspec-puppet always sets $server_facts['servername']")
             should_not contain_file('/etc/custom/puppetlabs/puppet/ssl/crl.pem')
           end
         end

spec/support/trusted.rb

@@ -0,0 +1,4 @@
+RSpec.configure do |c|
+  c.trusted_node_data = true
+  c.trusted_server_facts = true
+end