From c57d2dd5ddafe26bd17727ab184b00fb8a166a2a Mon Sep 17 00:00:00 2001 From: Daniele Sluijters Date: Mon, 2 Mar 2015 22:40:06 +0100 Subject: [PATCH] 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. --- manifests/params.pp | 23 +++++++++++++++++------ manifests/pin.pp | 12 +++++++++++- manifests/ppa.pp | 4 ++-- manifests/source.pp | 31 ++++++++++++++++--------------- spec/classes/apt_spec.rb | 2 +- spec/classes/apt_update_spec.rb | 20 ++++++++++---------- spec/classes/params_spec.rb | 2 +- spec/defines/conf_spec.rb | 2 +- spec/defines/key_spec.rb | 21 ++++++++++++++------- spec/defines/pin_spec.rb | 2 +- spec/defines/ppa_spec.rb | 13 +++++++------ spec/defines/setting_spec.rb | 2 +- templates/pin.pref.erb | 2 +- 13 files changed, 83 insertions(+), 53 deletions(-) diff --git a/manifests/params.pp b/manifests/params.pp index 14401c6..8edd2e7 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -1,9 +1,20 @@ 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 +24,6 @@ class apt::params { $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 +75,7 @@ class apt::params { case $::lsbdistid { 'ubuntu', 'debian': { $distid = $::lsbdistid - $distcodename = $::lsbdistcodename + $distcodename = $xfacts['lsbdistcodename'] } 'linuxmint': { if $::lsbdistcodename == 'debian' { @@ -113,5 +120,9 @@ class apt::params { } } } + '', default: { + $ppa_options = undef + $ppa_package = undef + } } } diff --git a/manifests/pin.pp b/manifests/pin.pp index b27ed8e..bcccf28 100644 --- 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 @@ define apt::pin( 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 index 33cd60d..5fc7f3c 100644 --- 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 index ea24cbf..9b34057 100644 --- 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 @@ define apt::source( $_key = merge($::apt::source_key_defaults, $key) } else { validate_string($key) + $_key = $key } } @@ -64,9 +65,9 @@ define apt::source( 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 index 9e05985..e668996 100644 --- 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 index 6ae9e8f..06f76de 100644 --- a/spec/classes/apt_update_spec.rb +++ b/spec/classes/apt_update_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' 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,7 +14,7 @@ describe 'apt::update', :type => :class do 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 @@ -22,7 +22,7 @@ describe 'apt::update', :type => :class do 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 @@ describe 'apt::update', :type => :class do 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 @@ describe 'apt::update', :type => :class do 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 @@ describe 'apt::update', :type => :class do 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 @@ describe 'apt::update', :type => :class do 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 @@ describe 'apt::update', :type => :class do 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,7 +88,7 @@ describe 'apt::update', :type => :class do 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) @@ -96,7 +96,7 @@ describe 'apt::update', :type => :class do 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 index 65759fa..f8ca89f 100644 --- 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 index 4e5b46b..a7db4e6 100644 --- a/spec/defines/conf_spec.rb +++ b/spec/defines/conf_spec.rb @@ -3,7 +3,7 @@ describe 'apt::conf', :type => :define do 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 index 31248cc..b9bcea8 100644 --- 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 @@ describe 'apt::key' do :id => title, :ensure => 'present', :source => nil, - :server => nil, + :server => 'keyserver.ubuntu.com', :content => nil, :options => nil, }) @@ -39,7 +44,7 @@ describe 'apt::key' do :id => GPG_KEY_ID, :ensure => 'present', :source => nil, - :server => nil, + :server => 'keyserver.ubuntu.com', :content => nil, :options => nil, }) @@ -59,7 +64,7 @@ describe 'apt::key' do :id => title, :ensure => 'absent', :source => nil, - :server => nil, + :server => 'keyserver.ubuntu.com', :content => nil, :keyserver => nil, }) @@ -276,7 +281,8 @@ describe 'apt::key' do 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 @@ describe 'apt::key' do :id => title, :ensure => 'present', :source => nil, - :server => nil, + :server => 'keyserver.ubuntu.com', :content => nil, :options => nil, }) @@ -305,7 +311,8 @@ describe 'apt::key' do 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 index e4d5d0a..a11c3b5 100644 --- a/spec/defines/pin_spec.rb +++ b/spec/defines/pin_spec.rb @@ -3,7 +3,7 @@ describe 'apt::pin', :type => :define do 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 index f29a8df..7c0d072 100644 --- 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 @@ describe 'apt::ppa' do :operatingsystem => 'Ubuntu', :lsbdistid => 'Ubuntu', :osfamily => 'Debian', + :lsbdistcodeanme => nil, } end let(:title) { 'ppa:foo' } @@ -203,10 +204,10 @@ describe 'apt::ppa' do 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 index e01fdbf..f397176 100644 --- a/spec/defines/setting_spec.rb +++ b/spec/defines/setting_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' 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 index 26b2516..76936d7 100644 --- 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 %> -- 2.32.3