do not specify file modes unless relevant

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é <anarcat@debian.org>

diff --git a/manifests/init.pp b/manifests/init.pp
index 39955344007233af8a1117b89fb54621fdfd7752..d2aead96112f26cf31a8d11bb51c8d207317d367 100644 (file)
--- 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 4729295d0d3e4fe84bef7667d1caa2bbf3c0108b..16124c31f51a2901986722e5d2a2c484908e0e42 100644 (file)
--- 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 4421180ba459998f725b01ffa008efb18572125f..ca11d4a990ca0cba0c3080d32a0a2a4ab1820c5c 100644 (file)
--- 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 1c0ee162531d59077598b1bc64daea5d9a51d49e..5decab57e99b4c91dbec16825553cc17fc7dd31e 100644 (file)
--- 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 7e2a9087aa9d1528462402a51fc37722793c0027..16db75145ab8acc97b1a7c367e326974c4022322 100644 (file)
--- 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