From b53ea1b90c8e050b1ce3008995ebfcb0ca0baf62 Mon Sep 17 00:00:00 2001 From: Daniele Sluijters Date: Fri, 27 Feb 2015 12:07:56 +0100 Subject: [PATCH 1/2] spec/(apt|ppa): Enough with all the strings. Most options can and should be named through symbols, makes it much easier to read too with syntax highlighting. --- spec/classes/apt_spec.rb | 82 ++++++++++++++++++++-------------------- spec/defines/ppa_spec.rb | 56 +++++++++++++-------------- 2 files changed, 69 insertions(+), 69 deletions(-) diff --git a/spec/classes/apt_spec.rb b/spec/classes/apt_spec.rb index b8ff37ea31..b07e0d6a76 100644 --- a/spec/classes/apt_spec.rb +++ b/spec/classes/apt_spec.rb @@ -1,46 +1,46 @@ require 'spec_helper' -describe 'apt', :type => :class do +describe 'apt' do let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } } context 'defaults' do it { is_expected.to contain_file('sources.list').that_notifies('Exec[apt_update]').only_with({ - 'ensure' => 'present', - 'path' => '/etc/apt/sources.list', - 'owner' => 'root', - 'group' => 'root', - 'mode' => '0644', - 'notify' => 'Exec[apt_update]', + :ensure => 'present', + :path => '/etc/apt/sources.list', + :owner => 'root', + :group => 'root', + :mode => '0644', + :notify => 'Exec[apt_update]', })} it { is_expected.to contain_file('sources.list.d').that_notifies('Exec[apt_update]').only_with({ - 'ensure' => 'directory', - 'path' => '/etc/apt/sources.list.d', - 'owner' => 'root', - 'group' => 'root', - 'purge' => false, - 'recurse' => false, - 'notify' => 'Exec[apt_update]', + :ensure => 'directory', + :path => '/etc/apt/sources.list.d', + :owner => 'root', + :group => 'root', + :purge => false, + :recurse => false, + :notify => 'Exec[apt_update]', })} it { is_expected.to contain_file('preferences.d').only_with({ - 'ensure' => 'directory', - 'path' => '/etc/apt/preferences.d', - 'owner' => 'root', - 'group' => 'root', - 'purge' => false, - 'recurse' => false, + :ensure => 'directory', + :path => '/etc/apt/preferences.d', + :owner => 'root', + :group => 'root', + :purge => false, + :recurse => false, })} it 'should lay down /etc/apt/apt.conf.d/15update-stamp' do is_expected.to contain_file('/etc/apt/apt.conf.d/15update-stamp').with({ - 'group' => 'root', - 'mode' => '0644', - 'owner' => 'root', + :group => 'root', + :mode => '0644', + :owner => 'root', }).with_content(/APT::Update::Post-Invoke-Success \{"touch \/var\/lib\/apt\/periodic\/update-success-stamp 2>\/dev\/null \|\| true";\};/) end it { is_expected.to contain_exec('apt_update').with({ - 'refreshonly' => 'true', + :refreshonly => 'true', })} end @@ -58,28 +58,28 @@ end it { is_expected.to contain_file('sources.list').with({ - 'content' => "# Repos managed by puppet.\n" + :content => "# Repos managed by puppet.\n" })} it { is_expected.to contain_file('sources.list.d').with({ - 'purge' => 'true', - 'recurse' => 'true', + :purge => 'true', + :recurse => 'true', })} it { is_expected.to contain_file('apt-preferences').only_with({ - 'ensure' => 'absent', - 'path' => '/etc/apt/preferences', + :ensure => 'absent', + :path => '/etc/apt/preferences', })} it { is_expected.to contain_file('preferences.d').with({ - 'purge' => 'true', - 'recurse' => 'true', + :purge => 'true', + :recurse => 'true', })} it { is_expected.to contain_exec('apt_update').with({ - 'refreshonly' => 'false', - 'timeout' => '1', - 'tries' => '3', + :refreshonly => 'false', + :timeout => '1', + :tries => '3', })} end @@ -99,7 +99,7 @@ 'key' => '55BE302B', 'key_server' => 'subkeys.pgp.net', 'pin' => '-10', - 'include_src' => true + 'include_src' => true, }, 'puppetlabs' => { 'location' => 'http://apt.puppetlabs.com', @@ -111,7 +111,7 @@ it { is_expected.to contain_apt__setting('list-debian_unstable').with({ - 'ensure' => 'present', + :ensure => 'present', }) } @@ -120,7 +120,7 @@ it { is_expected.to contain_apt__setting('list-puppetlabs').with({ - 'ensure' => 'present', + :ensure => 'present', }) } @@ -131,7 +131,7 @@ context 'bad purge_sources_list' do let :params do { - 'purge_sources_list' => 'foo' + :purge_sources_list => 'foo' } end it do @@ -144,7 +144,7 @@ context 'bad purge_sources_list_d' do let :params do { - 'purge_sources_list_d' => 'foo' + :purge_sources_list_d => 'foo' } end it do @@ -157,7 +157,7 @@ context 'bad purge_preferences' do let :params do { - 'purge_preferences' => 'foo' + :purge_preferences => 'foo' } end it do @@ -170,7 +170,7 @@ context 'bad purge_preferences_d' do let :params do { - 'purge_preferences_d' => 'foo' + :purge_preferences_d => 'foo' } end it do diff --git a/spec/defines/ppa_spec.rb b/spec/defines/ppa_spec.rb index 6ca008b4ba..dde015abad 100644 --- a/spec/defines/ppa_spec.rb +++ b/spec/defines/ppa_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -describe 'apt::ppa', :type => :define do +describe 'apt::ppa' do describe 'defaults' do let :pre_condition do @@ -18,11 +18,11 @@ let(:title) { 'ppa:needs/such.substitution/wow' } it { is_expected.to_not contain_package('python-software-properties') } it { is_expected.to contain_exec('add-apt-repository-ppa:needs/such.substitution/wow').that_notifies('Exec[apt_update]').with({ - 'environment' => [], - 'command' => '/usr/bin/add-apt-repository -y ppa:needs/such.substitution/wow', - 'unless' => '/usr/bin/test -s /etc/apt/sources.list.d/needs-such_substitution-wow-natty.list', - 'user' => 'root', - 'logoutput' => 'on_failure', + :environment => [], + :command => '/usr/bin/add-apt-repository -y ppa:needs/such.substitution/wow', + :unless => '/usr/bin/test -s /etc/apt/sources.list.d/needs-such_substitution-wow-natty.list', + :user => 'root', + :logoutput => 'on_failure', }) } end @@ -42,18 +42,18 @@ end let :params do { - 'options' => '', - 'package_manage' => true, + :options => '', + :package_manage => true, } end let(:title) { 'ppa:foo' } it { is_expected.to contain_package('software-properties-common') } it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ - 'environment' => [], - 'command' => '/usr/bin/add-apt-repository ppa:foo', - 'unless' => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', - 'user' => 'root', - 'logoutput' => 'on_failure', + :environment => [], + :command => '/usr/bin/add-apt-repository ppa:foo', + :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', + :user => 'root', + :logoutput => 'on_failure', }) } end @@ -81,11 +81,11 @@ let(:title) { 'ppa:foo' } it { is_expected.to contain_package('software-properties-common') } it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ - 'environment' => ['http_proxy=http://localhost:8080', 'https_proxy=http://localhost:8080'], - 'command' => '/usr/bin/add-apt-repository ppa:foo', - 'unless' => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', - 'user' => 'root', - 'logoutput' => 'on_failure', + :environment => ['http_proxy=http://localhost:8080', 'https_proxy=http://localhost:8080'], + :command => '/usr/bin/add-apt-repository ppa:foo', + :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', + :user => 'root', + :logoutput => 'on_failure', }) } end @@ -105,19 +105,19 @@ end let :params do { - 'options' => '', - 'package_manage' => true, - 'proxy' => { 'host' => 'localhost', 'port' => 8180, } + :options => '', + :package_manage => true, + :proxy => { 'host' => 'localhost', 'port' => 8180, } } end let(:title) { 'ppa:foo' } it { is_expected.to contain_package('software-properties-common') } it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ - 'environment' => ['http_proxy=http://localhost:8180', 'https_proxy=http://localhost:8180'], - 'command' => '/usr/bin/add-apt-repository ppa:foo', - 'unless' => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', - 'user' => 'root', - 'logoutput' => 'on_failure', + :environment => ['http_proxy=http://localhost:8180', 'https_proxy=http://localhost:8180'], + :command => '/usr/bin/add-apt-repository ppa:foo', + :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', + :user => 'root', + :logoutput => 'on_failure', }) } end @@ -138,11 +138,11 @@ let(:title) { 'ppa:foo' } let :params do { - 'ensure' => 'absent' + :ensure => 'absent' } end it { is_expected.to contain_file('/etc/apt/sources.list.d/foo-trusty.list').that_notifies('Exec[apt_update]').with({ - 'ensure' => 'absent', + :ensure => 'absent', }) } end From d81c3d9476b14892882620723a02617c344f703c Mon Sep 17 00:00:00 2001 From: Daniele Sluijters Date: Fri, 27 Feb 2015 12:34:05 +0100 Subject: [PATCH 2/2] apt: Add proxy support on the class. Re-introduce proxy support at the class level. Needing to configure a proxy is such a common scenario that having it on the class is a reasonable thing. It also affects `apt::ppa`. Change `apt::ppa` to no longer have its own `proxy` parameter but use the proxy as configured on the main `apt` class. --- manifests/init.pp | 23 ++++++++++++++++++++ manifests/params.pp | 7 +++--- manifests/ppa.pp | 17 +++++++-------- spec/classes/apt_spec.rb | 36 ++++++++++++++++++++++++++++++ spec/defines/ppa_spec.rb | 47 +++++++++++++++++++++++++++++++++++----- templates/proxy.erb | 4 ++++ 6 files changed, 116 insertions(+), 18 deletions(-) create mode 100644 templates/proxy.erb diff --git a/manifests/init.pp b/manifests/init.pp index 0b4a0bc626..86e49c8f4e 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -1,5 +1,6 @@ # class apt( + $proxy = {}, $always_apt_update = false, $apt_update_frequency = 'reluctantly', $purge_sources_list = false, @@ -19,6 +20,28 @@ validate_bool($purge_sources_list, $purge_sources_list_d, $purge_preferences, $purge_preferences_d) + validate_hash($proxy) + if $proxy['host'] { + validate_string($proxy['host']) + } + if $proxy['port'] { + unless is_integer($proxy['port']) { + fail('$proxy port must be an integer') + } + } + if $proxy['https'] { + validate_bool($proxy['https']) + } + + $_proxy = merge($apt::proxy_defaults, $proxy) + + if $proxy['host'] { + apt::setting { 'conf-proxy': + priority => '01', + content => template('apt/_header.erb', 'apt/proxy.erb'), + } + } + $sources_list_content = $purge_sources_list ? { false => undef, true => "# Repos managed by puppet.\n", diff --git a/manifests/params.pp b/manifests/params.pp index 3c169e3fcf..eda6adc93b 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -31,9 +31,10 @@ } } - $proxy = { - 'host' => undef, - 'port' => 8080, + $proxy_defaults = { + 'host' => undef, + 'port' => 8080, + 'https' => false, } $file_defaults = { diff --git a/manifests/ppa.pp b/manifests/ppa.pp index 1dc8e26fce..4a08dce6fd 100644 --- a/manifests/ppa.pp +++ b/manifests/ppa.pp @@ -5,7 +5,6 @@ $options = $::apt::ppa_options, $package_name = $::apt::ppa_package, $package_manage = false, - $proxy = {}, ) { if ! $release { fail('lsbdistcodename fact not available: release parameter required') @@ -20,8 +19,6 @@ $filename_without_ppa = regsubst($filename_without_dots, '^ppa:', '', 'G') $sources_list_d_filename = "${filename_without_ppa}-${release}.list" - $_proxy = merge($apt::proxy, $proxy) - if $ensure == 'present' { if $package_manage { package { $package_name: } @@ -31,13 +28,15 @@ $_require = File['sources.list.d'] } - case $_proxy['host'] { - false, '', undef: { - $_proxy_env = [] - } - default: { - $_proxy_env = ["http_proxy=http://${_proxy['host']}:${_proxy['port']}", "https_proxy=http://${_proxy['host']}:${_proxy['port']}"] + $_proxy = $::apt::_proxy + if $_proxy['host'] { + if $_proxy['https'] { + $_proxy_env = ["http_proxy=http://${_proxy['host']}:${_proxy['port']}", "https_proxy=https://${_proxy['host']}:${_proxy['port']}"] + } else { + $_proxy_env = ["http_proxy=http://${_proxy['host']}:${_proxy['port']}"] } + } else { + $_proxy_env = [] } exec { "add-apt-repository-${name}": diff --git a/spec/classes/apt_spec.rb b/spec/classes/apt_spec.rb index b07e0d6a76..c53e2a7dc6 100644 --- a/spec/classes/apt_spec.rb +++ b/spec/classes/apt_spec.rb @@ -42,8 +42,44 @@ it { is_expected.to contain_exec('apt_update').with({ :refreshonly => 'true', })} + + it { is_expected.not_to contain_apt__setting('conf-proxy') } end + describe 'proxy=' do + context 'host=localhost' do + let(:params) { { :proxy => { 'host' => 'localhost'} } } + it { is_expected.to contain_apt__setting('conf-proxy').with({ + :priority => '01', + }).with_content( + /Acquire::http::proxy "http:\/\/localhost:8080\/";/ + ).without_content( + /Acquire::https::proxy/ + )} + end + + context 'host=localhost and port=8180' do + let(:params) { { :proxy => { 'host' => 'localhost', 'port' => 8180} } } + it { is_expected.to contain_apt__setting('conf-proxy').with({ + :priority => '01', + }).with_content( + /Acquire::http::proxy "http:\/\/localhost:8180\/";/ + ).without_content( + /Acquire::https::proxy/ + )} + end + + context 'host=localhost and https=true' do + let(:params) { { :proxy => { 'host' => 'localhost', 'https' => true} } } + it { is_expected.to contain_apt__setting('conf-proxy').with({ + :priority => '01', + }).with_content( + /Acquire::http::proxy "http:\/\/localhost:8080\/";/ + ).with_content( + /Acquire::https::proxy "https:\/\/localhost:8080\/";/ + )} + end + end context 'lots of non-defaults' do let :params do { diff --git a/spec/defines/ppa_spec.rb b/spec/defines/ppa_spec.rb index dde015abad..14a5139c3b 100644 --- a/spec/defines/ppa_spec.rb +++ b/spec/defines/ppa_spec.rb @@ -60,7 +60,9 @@ describe 'apt included, proxy host' do let :pre_condition do - 'class { "apt": }' + 'class { "apt": + proxy => { "host" => "localhost" }, + }' end let :facts do { @@ -75,13 +77,12 @@ { 'options' => '', 'package_manage' => true, - 'proxy' => { 'host' => 'localhost', } } end let(:title) { 'ppa:foo' } it { is_expected.to contain_package('software-properties-common') } it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ - :environment => ['http_proxy=http://localhost:8080', 'https_proxy=http://localhost:8080'], + :environment => ['http_proxy=http://localhost:8080'], :command => '/usr/bin/add-apt-repository ppa:foo', :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', :user => 'root', @@ -92,7 +93,42 @@ describe 'apt included, proxy host and port' do let :pre_condition do - 'class { "apt": }' + 'class { "apt": + proxy => { "host" => "localhost", "port" => 8180 }, + }' + end + let :facts do + { + :lsbdistrelease => '14.04', + :lsbdistcodename => 'trusty', + :operatingsystem => 'Ubuntu', + :lsbdistid => 'Ubuntu', + :osfamily => 'Debian', + } + end + let :params do + { + :options => '', + :package_manage => true, + } + end + let(:title) { 'ppa:foo' } + it { is_expected.to contain_package('software-properties-common') } + it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ + :environment => ['http_proxy=http://localhost:8180'], + :command => '/usr/bin/add-apt-repository ppa:foo', + :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', + :user => 'root', + :logoutput => 'on_failure', + }) + } + end + + describe 'apt included, proxy host and port and https' do + let :pre_condition do + 'class { "apt": + proxy => { "host" => "localhost", "port" => 8180, "https" => true }, + }' end let :facts do { @@ -107,13 +143,12 @@ { :options => '', :package_manage => true, - :proxy => { 'host' => 'localhost', 'port' => 8180, } } end let(:title) { 'ppa:foo' } it { is_expected.to contain_package('software-properties-common') } it { is_expected.to contain_exec('add-apt-repository-ppa:foo').that_notifies('Exec[apt_update]').with({ - :environment => ['http_proxy=http://localhost:8180', 'https_proxy=http://localhost:8180'], + :environment => ['http_proxy=http://localhost:8180', 'https_proxy=https://localhost:8180'], :command => '/usr/bin/add-apt-repository ppa:foo', :unless => '/usr/bin/test -s /etc/apt/sources.list.d/foo-trusty.list', :user => 'root', diff --git a/templates/proxy.erb b/templates/proxy.erb new file mode 100644 index 0000000000..670e3a7e87 --- /dev/null +++ b/templates/proxy.erb @@ -0,0 +1,4 @@ +Acquire::http::proxy "http://<%= @_proxy['host'] %>:<%= @_proxy['port'] %>/"; +<%- if @_proxy['https'] %> +Acquire::https::proxy "https://<%= @_proxy['host'] %>:<%= @_proxy['port'] %>/"; +<%- end -%>