apt: Fix all strict variable cases.

A few of these fixes are absolutely horrendous but we have no choice as we need to stay current- and future-parser compatible for now. Once we can go Puppet 4 only we can use the `$facts` hash lookup instead which will return undef/nil for things that aren't set instead of them not being defined at all.
puppetlabs · Mar 3, 2015 · 6e36831 · 6e36831
1 parent 3960368
commit 6e36831
Show file tree

Hide file tree

Showing 13 changed files with 84 additions and 53 deletions.
diff --git a/manifests/params.pp b/manifests/params.pp
@@ -1,9 +1,17 @@
 class apt::params {
 
-  if $caller_module_name and $caller_module_name != $module_name {
+  if defined('$caller_module_name') and $caller_module_name and $caller_module_name != $module_name {
     fail('apt::params is a private class and cannot be accessed directly')
   }
 
+  if $::osfamily != 'Debian' {
+    fail('This module only works on Debian or derivatives like Ubuntu')
+  }
+
+  $xfacts = {
+    'lsbdistcodename' => defined('$lsbdistcodename') ? { true => $::lsbdistcodename, default => undef },
+  }
+
   $root           = '/etc/apt'
   $provider       = '/usr/bin/apt-get'
   $sources_list   = "${root}/sources.list"
@@ -13,10 +21,6 @@
   $preferences_d  = "${root}/preferences.d"
   $keyserver      = 'keyserver.ubuntu.com'
 
-  if $::osfamily != 'Debian' {
-    fail('This module only works on Debian or derivatives like Ubuntu')
-  }
-
   $config_files = {
     'conf'   => {
       'path' => $conf_d,
@@ -68,7 +72,11 @@
   case $::lsbdistid {
     'ubuntu', 'debian': {
       $distid = $::lsbdistid
-      $distcodename = $::lsbdistcodename
+      if defined('$lsbdistcodename') {
+        $distcodename = $::lsbdistcodename
+      } else {
+        $distcodename = undef
+      }
     }
     'linuxmint': {
       if $::lsbdistcodename == 'debian' {
@@ -113,5 +121,9 @@
         }
       }
     }
+    '', default: {
+      $ppa_options = undef
+      $ppa_package = undef
+    }
   }
 }
diff --git a/manifests/pin.pp b/manifests/pin.pp
@@ -3,7 +3,7 @@
 
 define apt::pin(
   $ensure          = present,
-  $explanation     = "${caller_module_name}: ${name}",
+  $explanation     = undef,
   $order           = undef,
   $packages        = '*',
   $priority        = 0,
@@ -20,6 +20,16 @@
     fail('Only integers are allowed in the apt::pin order param')
   }
 
+  if $explanation {
+    $_explanation = $explanation
+  } else {
+    if defined('$caller_module_name') { # strict vars check
+      $_explanation = "${caller_module_name}: ${name}"
+    } else {
+      $_explanation = ": ${name}"
+    }
+  }
+
   $pin_release_array = [
     $release,
     $codename,

diff --git a/manifests/ppa.pp b/manifests/ppa.pp
@@ -1,12 +1,12 @@
 # ppa.pp
 define apt::ppa(
   $ensure         = 'present',
-  $release        = $::lsbdistcodename,
   $options        = $::apt::ppa_options,
+  $release        = $::apt::xfacts['lsbdistcodename'],
   $package_name   = $::apt::ppa_package,
   $package_manage = false,
 ) {
-  if ! $release {
+  unless $release {
     fail('lsbdistcodename fact not available: release parameter required')
   }
 

diff --git a/manifests/source.pp b/manifests/source.pp
@@ -1,22 +1,22 @@
 # source.pp
 # add an apt source
 define apt::source(
-  $comment           = $name,
-  $ensure            = present,
-  $location          = '',
-  $release           = $::lsbdistcodename,
-  $repos             = 'main',
-  $include_src       = false,
-  $include_deb       = true,
-  $key               = undef,
-  $pin               = false,
-  $architecture      = undef,
-  $trusted_source    = false,
+  $comment        = $name,
+  $ensure         = present,
+  $location       = '',
+  $release        = $::apt::xfacts['lsbdistcodename'],
+  $repos          = 'main',
+  $include_src    = false,
+  $include_deb    = true,
+  $key            = undef,
+  $pin            = false,
+  $architecture   = undef,
+  $trusted_source = false,
 ) {
-  validate_string($architecture, $comment, $location, $release, $repos)
+  validate_string($architecture, $comment, $location, $repos)
   validate_bool($trusted_source, $include_src, $include_deb)
 
-  if ! $release {
+  unless $release {
     fail('lsbdistcodename fact not available: release parameter required')
   }
 
@@ -30,6 +30,7 @@
       $_key = merge($::apt::source_key_defaults, $key)
     } else {
       validate_string($key)
+      $_key = $key
     }
   }
 
@@ -64,9 +65,9 @@
         before  => $_before,
       }
     } else {
-      apt::key { "Add key: ${key} from Apt::Source ${title}":
+      apt::key { "Add key: ${_key} from Apt::Source ${title}":
         ensure => present,
-        id     => $key,
+        id     => $_key,
         before => $_before,
       }
     }

diff --git a/spec/classes/apt_spec.rb b/spec/classes/apt_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 describe 'apt' do
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy'} }
 
   context 'defaults' do
     it { is_expected.to contain_file('sources.list').that_notifies('Exec[apt_update]').only_with({

diff --git a/spec/classes/apt_update_spec.rb b/spec/classes/apt_update_spec.rb
@@ -4,7 +4,7 @@
 describe 'apt::update', :type => :class do
   context "when update['always']=true" do
     #This should completely disable all of this logic. These tests are to guarantee that we don't somehow magically change the behavior.
-    let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+    let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
     let (:pre_condition) { "class{'::apt': update => {'always' => true},}" }
     it 'should trigger an apt-get update run' do
       #set the apt_update exec's refreshonly attribute to false
@@ -14,15 +14,15 @@
       context "when apt::update['frequency'] has the value of #{update_frequency}" do
         { 'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
           context "and $::apt_update_last_success indicates #{desc}" do
-            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval, :lsbdistcodename => 'wheezy' } }
             let (:pre_condition) { "class{'::apt': update => {'always' => true, 'frequency' => '#{update_frequency}'}, }" }
             it 'should trigger an apt-get update run' do
               # set the apt_update exec's refreshonly attribute to false
               is_expected.to contain_exec('apt_update').with({'refreshonly' => false})
             end
           end
           context 'when $::apt_update_last_success is nil' do
-            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
             let (:pre_condition) { "class{'::apt': update => {'always' => true, 'frequency' => '#{update_frequency}'}, }" }
             it 'should trigger an apt-get update run' do
               #set the apt_update exec\'s refreshonly attribute to false
@@ -38,7 +38,7 @@
     context "and apt::update['frequency']='always'" do
       { 'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
         context "and $::apt_update_last_success indicates #{desc}" do
-          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval, :lsbdistcodename => 'wheezy' } }
           let (:pre_condition) { "class{'::apt': update => {'always' => false, 'frequency' => 'always' },}" }
           it 'should trigger an apt-get update run' do
             #set the apt_update exec's refreshonly attribute to false
@@ -47,7 +47,7 @@
         end
       end
       context 'when $::apt_update_last_success is nil' do
-        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
         let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => 'always' },}" }
         it 'should trigger an apt-get update run' do
           #set the apt_update exec\'s refreshonly attribute to false
@@ -58,7 +58,7 @@
     context "and apt::update['frequency']='reluctantly'" do
       {'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
         context "and $::apt_update_last_success indicates #{desc}" do
-          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval} }
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval, :lsbdistcodename => 'wheezy'} }
           let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => 'reluctantly' },}" }
           it 'should not trigger an apt-get update run' do
             #don't change the apt_update exec's refreshonly attribute. (it should be true)
@@ -67,7 +67,7 @@
         end
       end
       context 'when $::apt_update_last_success is nil' do
-        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
         let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => 'reluctantly' },}" }
         it 'should not trigger an apt-get update run' do
           #don't change the apt_update exec's refreshonly attribute. (it should be true)
@@ -79,7 +79,7 @@
       context "and apt::update['frequency'] has the value of #{update_frequency}" do
         { 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
           context "and $::apt_update_last_success indicates #{desc}" do
-            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval, :lsbdistcodename => 'wheezy' } }
             let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => '#{update_frequency}',} }" }
             it 'should trigger an apt-get update run' do
               #set the apt_update exec\'s refreshonly attribute to false
@@ -88,15 +88,15 @@
           end
         end
         context 'when the $::apt_update_last_success fact has a recent value' do
-          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => Time.now.to_i } }
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy', :apt_update_last_success => Time.now.to_i } }
             let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => '#{update_frequency}',} }" }
           it 'should not trigger an apt-get update run' do
             #don't change the apt_update exec\'s refreshonly attribute. (it should be true)
             is_expected.to contain_exec('apt_update').with({'refreshonly' => true})
           end
         end
         context 'when $::apt_update_last_success is nil' do
-          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy', :apt_update_last_success => nil } }
             let (:pre_condition) { "class{ '::apt': update => {'always' => false, 'frequency' => '#{update_frequency}',} }" }
           it 'should trigger an apt-get update run' do
             #set the apt_update exec\'s refreshonly attribute to false

diff --git a/spec/classes/params_spec.rb b/spec/classes/params_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 describe 'apt::params', :type => :class do
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
   let (:title) { 'my_package' }
 
   it { is_expected.to contain_apt__params }

diff --git a/spec/defines/conf_spec.rb b/spec/defines/conf_spec.rb
@@ -3,7 +3,7 @@
   let :pre_condition do
     'class { "apt": }'
   end
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
   let :title do
     'norecommends'
   end

diff --git a/spec/defines/key_spec.rb b/spec/defines/key_spec.rb
@@ -1,7 +1,12 @@
 require 'spec_helper'
 
 describe 'apt::key' do
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let :pre_condition do
+    'class { "apt": }'
+  end
+
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
+
   GPG_KEY_ID = '47B320EB4C7C375AA9DAE1A01054B7A24BD6EC30'
 
   let :title do
@@ -15,7 +20,7 @@
           :id      => title,
           :ensure  => 'present',
           :source  => nil,
-          :server  => nil,
+          :server  => 'keyserver.ubuntu.com',
           :content => nil,
           :options => nil,
         })
@@ -39,7 +44,7 @@
           :id      => GPG_KEY_ID,
           :ensure  => 'present',
           :source  => nil,
-          :server  => nil,
+          :server  => 'keyserver.ubuntu.com',
           :content => nil,
           :options => nil,
         })
@@ -59,7 +64,7 @@
           :id        => title,
           :ensure    => 'absent',
           :source    => nil,
-          :server    => nil,
+          :server    => 'keyserver.ubuntu.com',
           :content   => nil,
           :keyserver => nil,
         })
@@ -276,7 +281,8 @@
     describe 'duplication' do
       context 'two apt::key resources for same key, different titles' do
         let :pre_condition do
-          "apt::key { 'duplicate': id => '#{title}', }"
+          "class { 'apt': }
+          apt::key { 'duplicate': id => '#{title}', }"
         end
 
         it 'contains two apt::key resources' do
@@ -295,7 +301,7 @@
             :id      => title,
             :ensure  => 'present',
             :source  => nil,
-            :server  => nil,
+            :server  => 'keyserver.ubuntu.com',
             :content => nil,
             :options => nil,
           })
@@ -305,7 +311,8 @@
 
       context 'two apt::key resources, different ensure' do
         let :pre_condition do
-          "apt::key { 'duplicate': id => '#{title}', ensure => 'absent', }"
+          "class { 'apt': }
+          apt::key { 'duplicate': id => '#{title}', ensure => 'absent', }"
         end
         it 'informs the user of the impossibility' do
           expect { subject }.to raise_error(/already ensured as absent/)

diff --git a/spec/defines/pin_spec.rb b/spec/defines/pin_spec.rb
@@ -3,7 +3,7 @@
   let :pre_condition do
     'class { "apt": }'
   end
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
   let(:title) { 'my_pin' }
 
   context 'defaults' do

diff --git a/spec/defines/ppa_spec.rb b/spec/defines/ppa_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 describe 'apt::ppa' do
+  let :pre_condition do
+    'class { "apt": }'
+  end
 
   describe 'defaults' do
-    let :pre_condition do
-      'class { "apt": }'
-    end
     let :facts do
       {
         :lsbdistrelease  => '11.04',
@@ -190,6 +190,7 @@
           :operatingsystem => 'Ubuntu',
           :lsbdistid       => 'Ubuntu',
           :osfamily        => 'Debian',
+          :lsbdistcodeanme => nil,
         }
       end
       let(:title) { 'ppa:foo' }
@@ -203,10 +204,10 @@
     describe 'not ubuntu' do
       let :facts do
         {
-          :lsbdistrelease  => '14.04',
-          :lsbdistcodename => 'trusty',
+          :lsbdistrelease  => '6.0.7',
+          :lsbdistcodename => 'wheezy',
           :operatingsystem => 'Debian',
-          :lsbdistid       => 'Ubuntu',
+          :lsbdistid       => 'debian',
           :osfamily        => 'Debian',
         }
       end

diff --git a/spec/defines/setting_spec.rb b/spec/defines/setting_spec.rb
@@ -2,7 +2,7 @@
 
 describe 'apt::setting' do
   let(:pre_condition) { 'class { "apt": }' }
-  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :lsbdistcodename => 'wheezy' } }
   let(:title) { 'conf-teddybear' }
 
   let(:default_params) { { :content => 'di' } }

diff --git a/templates/pin.pref.erb b/templates/pin.pref.erb
@@ -15,7 +15,7 @@ elsif @origin.length > 0
   @pin = "origin #{@origin}"
 end
 -%>
-Explanation: <%= @explanation %>
+Explanation: <%= @_explanation %>
 Package: <%= @packages_string %>
 Pin: <%= @pin %>
 Pin-Priority: <%= @priority %>