Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not specify file modes unless relevant #923

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

anarcat
Copy link

@anarcat anarcat commented Mar 11, 2020

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):

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 #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

@anarcat anarcat requested a review from a team as a code owner March 11, 2020 14:05
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>
@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #923 into master will decrease coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #923      +/-   ##
==========================================
- Coverage   73.07%   72.69%   -0.39%     
==========================================
  Files           5        5              
  Lines         260      260              
==========================================
- Hits          190      189       -1     
- Misses         70       71       +1     
Impacted Files Coverage Δ
lib/facter/apt_reboot_required.rb 75.00% <0.00%> (-25.00%) ⬇️
lib/facter/apt_updates.rb 89.65% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 718cf4f...4d8b3e1. Read the comment docs.

Copy link

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this solution @anarcat - one that seems to satisfy all parties involved, very much appreciated.

@sanfrancrisko sanfrancrisko changed the title do not specify file modes unless relevant Do not specify file modes unless relevant Mar 12, 2020
@sanfrancrisko sanfrancrisko merged commit e69426f into puppetlabs:master Mar 12, 2020
@anarcat anarcat deleted the no-mode branch March 12, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants