Skip to content

Commit

Permalink
do not specify file modes unless relevant
Browse files Browse the repository at this point in the history
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 puppetlabs#906):

ab2e06b 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 puppetlabs#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 316fd8f.

Signed-off-by: Antoine Beaupré <anarcat@debian.org>
  • Loading branch information
anarcat committed Mar 11, 2020
1 parent 718cf4f commit 2dd273c
Show file tree
Hide file tree
Showing 5 changed files with 2 additions and 17 deletions.
5 changes: 0 additions & 5 deletions manifests/init.pp
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,6 @@
path => $::apt::sources_list,
owner => root,
group => root,
mode => '0644',
notify => Class['apt::update'],
}

Expand All @@ -239,7 +238,6 @@
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'],
Expand All @@ -250,7 +248,6 @@
path => $::apt::preferences,
owner => root,
group => root,
mode => '0644',
notify => Class['apt::update'],
}

Expand All @@ -259,7 +256,6 @@
path => $::apt::preferences_d,
owner => root,
group => root,
mode => '0644',
purge => $_purge['preferences.d'],
recurse => $_purge['preferences.d'],
notify => Class['apt::update'],
Expand All @@ -270,7 +266,6 @@
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'],
Expand Down
1 change: 0 additions & 1 deletion manifests/setting.pp
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@
ensure => $ensure,
owner => 'root',
group => 'root',
mode => '0644',
content => $content,
source => $source,
notify => $_notify,
Expand Down
5 changes: 0 additions & 5 deletions spec/classes/apt_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@
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]' }
Expand All @@ -20,14 +18,12 @@
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]' }
Expand Down Expand Up @@ -76,7 +72,6 @@

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";};},
)
Expand Down
6 changes: 2 additions & 4 deletions spec/defines/conf_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@
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
Expand Down Expand Up @@ -82,8 +81,7 @@
it {
is_expected.to contain_file(filename).with('ensure' => 'absent',
'owner' => 'root',
'group' => 'root',
'mode' => '0644')
'group' => 'root')
}
end
end
2 changes: 0 additions & 2 deletions spec/defines/setting_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
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
Expand All @@ -62,7 +61,6 @@
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
Expand Down

0 comments on commit 2dd273c

Please sign in to comment.