From 4d8b3e11f85758ef7dc754a3051e1d36e76aaebc Mon Sep 17 00:00:00 2001 From: =?utf8?q?Antoine=20Beaupr=C3=A9?= Date: Wed, 11 Mar 2020 09:58:15 -0400 Subject: [PATCH] do not specify file modes unless relevant MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit MODULES-10583 makes a good point: "why are you messing with my file permissions"? In my case, the entire reason I made the following change (in PR #906): ab2e06b72f2be8dc38d6e3ecec68dc2cdacbce4e MODULES-10548: make files readonly ... is exactly *because* Puppet was changing the file modes from under me. I was migrating from our own in-house APT module to the forge one, and our module did *not* intervene in those file modes: it left the file resources alone. Which means we could have a directive like this: File { owner => root, group => root, mode => '444', ensure => file, } ... which made all files readonly by default. So when I migrated to the Puppetlabs APT module, modes were changed to be writable, which I did not want. As I reasoned in MODULES-10548, having files readonly provides an excellent indicator that a file is managed by Puppet, even if some module does not add a warning header - either because it forgot or because it's impossible. But I also understand if people do not like that policy. I think the proper way of doing this is not specifying a mode at all, and let local site-specific policies apply. I specifically proppose this as an alternative to #921 because I believe adding more parameters to the resources will needlessly complicate the script, when we have a native, Puppet-DSL supported way of changing those modes according to the right scope and context. In a similar way, we might want to reconsider user and group ownership of the files, but that can be done in a later time. This reverts commit 316fd8f4dd1eb6595b3be495ccc5f4924da4de1b. Signed-off-by: Antoine Beaupré --- manifests/init.pp | 5 ----- manifests/setting.pp | 1 - spec/classes/apt_spec.rb | 6 ------ spec/defines/conf_spec.rb | 6 ++---- spec/defines/setting_spec.rb | 2 -- 5 files changed, 2 insertions(+), 18 deletions(-) diff --git a/manifests/init.pp b/manifests/init.pp index 3995534..d2aead9 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -230,7 +230,6 @@ class apt ( path => $::apt::sources_list, owner => root, group => root, - mode => '0644', notify => Class['apt::update'], } @@ -239,7 +238,6 @@ class apt ( path => $::apt::sources_list_d, owner => root, group => root, - mode => '0644', purge => $_purge['sources.list.d'], recurse => $_purge['sources.list.d'], notify => Class['apt::update'], @@ -250,7 +248,6 @@ class apt ( path => $::apt::preferences, owner => root, group => root, - mode => '0644', notify => Class['apt::update'], } @@ -259,7 +256,6 @@ class apt ( path => $::apt::preferences_d, owner => root, group => root, - mode => '0644', purge => $_purge['preferences.d'], recurse => $_purge['preferences.d'], notify => Class['apt::update'], @@ -270,7 +266,6 @@ class apt ( path => $::apt::apt_conf_d, owner => root, group => root, - mode => '0644', purge => $_purge['apt.conf.d'], recurse => $_purge['apt.conf.d'], notify => Class['apt::update'], diff --git a/manifests/setting.pp b/manifests/setting.pp index 4729295..16124c3 100644 --- a/manifests/setting.pp +++ b/manifests/setting.pp @@ -69,7 +69,6 @@ define apt::setting ( ensure => $ensure, owner => 'root', group => 'root', - mode => '0644', content => $content, source => $source, notify => $_notify, diff --git a/spec/classes/apt_spec.rb b/spec/classes/apt_spec.rb index 4421180..ca11d4a 100644 --- a/spec/classes/apt_spec.rb +++ b/spec/classes/apt_spec.rb @@ -4,14 +4,12 @@ sources_list = { ensure: 'file', path: '/etc/apt/sources.list', owner: 'root', group: 'root', - mode: '0644', notify: 'Class[Apt::Update]' } sources_list_d = { ensure: 'directory', path: '/etc/apt/sources.list.d', owner: 'root', group: 'root', - mode: '0644', purge: false, recurse: false, notify: 'Class[Apt::Update]' } @@ -20,14 +18,12 @@ preferences = { ensure: 'file', path: '/etc/apt/preferences', owner: 'root', group: 'root', - mode: '0644', notify: 'Class[Apt::Update]' } preferences_d = { ensure: 'directory', path: '/etc/apt/preferences.d', owner: 'root', group: 'root', - mode: '0644', purge: false, recurse: false, notify: 'Class[Apt::Update]' } @@ -36,7 +32,6 @@ apt_conf_d = { ensure: 'directory', path: '/etc/apt/apt.conf.d', owner: 'root', group: 'root', - mode: '0644', purge: false, recurse: false, notify: 'Class[Apt::Update]' } @@ -76,7 +71,6 @@ describe 'apt' do it 'lays 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').with_content( %r{APT::Update::Post-Invoke-Success {"touch /var/lib/apt/periodic/update-success-stamp 2>/dev/null || true";};}, ) diff --git a/spec/defines/conf_spec.rb b/spec/defines/conf_spec.rb index 1c0ee16..5decab5 100644 --- a/spec/defines/conf_spec.rb +++ b/spec/defines/conf_spec.rb @@ -34,8 +34,7 @@ describe 'apt::conf', type: :define do is_expected.to contain_file(filename).with('ensure' => 'present', 'content' => %r{Apt::Install-Recommends 0;\nApt::AutoRemove::InstallRecommends 1;}, 'owner' => 'root', - 'group' => 'root', - 'mode' => '0644') + 'group' => 'root') } context 'with notify_update = true (default)' do @@ -82,8 +81,7 @@ describe 'apt::conf', type: :define do it { is_expected.to contain_file(filename).with('ensure' => 'absent', 'owner' => 'root', - 'group' => 'root', - 'mode' => '0644') + 'group' => 'root') } end end diff --git a/spec/defines/setting_spec.rb b/spec/defines/setting_spec.rb index 7e2a908..16db751 100644 --- a/spec/defines/setting_spec.rb +++ b/spec/defines/setting_spec.rb @@ -50,7 +50,6 @@ describe 'apt::setting' do is_expected.to contain_file('/etc/apt/apt.conf.d/50teddybear').that_notifies('Class[Apt::Update]').with(ensure: 'file', owner: 'root', group: 'root', - mode: '0644', source: params[:source].to_s) } end @@ -62,7 +61,6 @@ describe 'apt::setting' do is_expected.to contain_file('/etc/apt/apt.conf.d/50teddybear').that_notifies('Class[Apt::Update]').with(ensure: 'file', owner: 'root', group: 'root', - mode: '0644', content: params[:content].to_s) } end -- 2.32.3