Page MenuHomeFreeBSD

sysutils/munin-*: Use .conf suffix for /usr/local/etc/newsyslog.conf.d/
ClosedPublic

Authored by woodsb02 on Sep 9 2018, 7:25 AM.

Details

Summary

sysutils/munin-*: Use .conf suffix for /usr/local/etc/newsyslog.conf.d/

Rename the files installed to /usr/local/etc/newsyslog.conf.d/ to end
with a '.conf' suffix.

Proposed changes to /etc/newsyslog.conf will only include files from the
/usr/local/etc/newsyslog.conf.d/ directory which end with '.conf' and do
not beginning with a '.' character. https://reviews.freebsd.org/D17086

Approved by: mat (maintainer)

Test Plan

poudriere testport -j 12amd64: ok

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

woodsb02 created this revision.Sep 9 2018, 7:25 AM
mat added a comment.Sep 10 2018, 9:41 AM

I disagree with the change as is. It is going to BREAK in a big way for people who have edited the actual munin-node file. It goes the same for all the other changes you did to other ports.

woodsb02 updated this revision to Diff 47865.Sep 10 2018, 1:54 PM

Incorporate pkg-install script to move old newsyslog.conf.d script to new location, addressing concerns raised by mat.
This will both prevent local modifications being reverted to default, and also clean up the old script file.

mat added a comment.Sep 10 2018, 3:01 PM

I really, really, do not like this.

What about people using deployment tools to set this file, how do they know it was renamed for some dubious reason?

I really think this is a very bad idea.

Ok.

Can I please check that you agree with the value of only including .conf files (e.g. if the "include" was new functionality), but just disagree with the necessary changes as they violate POLA?

If that is the case, perhaps cem's idea to only include files if they do not start with a "." or finish with ".pkgnew", ".sample" or ".bak" (any others)?

Also, what are your thoughts on the way forward for the ports below which already have this change committed:

  • net/ntpa
  • mail/mailman
  • security/acme.sh

Possible options:

  • Rollback change (commit was approx 30 hours ago)
  • Add pkg-install feature to move old newsyslog.d file to new location
mat added a comment.Sep 10 2018, 3:51 PM

I wanted first to apologize if I sounded harsh, the work you're doing is great, I just do not think it is the correct way to go.

Can I please check that you agree with the value of only including .conf files (e.g. if the "include" was new functionality), but just disagree with the necessary changes as they violate POLA?

It would have been great if those "base system people" πŸ˜‰ had thought a bit more and added it as *.conf, but it has been * for 4 years, and in all releases since 10.1, it is a long time.

If that is the case, perhaps cem's idea to only include files if they do not start with a "." or finish with ".pkgnew", ".sample" or ".bak" (any others)?

I'd rather it be this way, yes, if possible.

Also, what are your thoughts on the way forward for the ports below which already have this change committed:

  • net/ntpa
  • mail/mailman
  • security/acme.sh

Possible options:

  • Rollback change (commit was approx 30 hours ago)

I have not checked the other, only ports using @sample would need to be rolled back.

  • Add pkg-install feature to move old newsyslog.d file to new location

The pkg-install thing is a bad idea, because it's the kind of fix that will need to stay for the unforseable future, it may need to stay in there for approximately a long time too. And like I said, people do use provisionning tools to set those files to values they need in their own companies, they'd have to handle two names depending on which version of the port, it would be sort of a nightmare.

Ok. I have reverted the changes to net/ntpa and mail/mailman - these were the only ones which install the conf file for you.

Can I ask for some globbing assistance please? I can't seem to find a way to match all filenames which do not end with ".sample", ".pkgnew" or ".bak"

woodsb02 abandoned this revision.Sep 10 2018, 4:18 PM

Abandon this change - relocating existing conf files violates POLA.

woodsb02 reclaimed this revision.Nov 10 2018, 10:59 AM

D17086 has now been committed to 13-CURRENT.
Re-opening this review.
New proposal to minimise the possibility that people are caught out unaware of this:

  1. Modify port to install file to newsyslog/*.conf
  2. Add move_newsyslog_conf() to pkg-install to automatically relocate any existing copy that is missing the .conf file
  3. Add note to UPDATING and pkg-message to notify the user of this change (important if provisioning tools such as ansible, puppet, salt, etc are in use).
woodsb02 updated this revision to Diff 50256.Nov 10 2018, 4:35 PM

Add entries to UPDATING and pkg-message

mat accepted this revision.Nov 12 2018, 11:24 AM

Looks ok.

This revision is now accepted and ready to land.Nov 12 2018, 11:24 AM
woodsb02 updated this revision to Diff 50317.Nov 12 2018, 2:57 PM

Hi mat,
Thanks for the approval.
I got some other comments from mandree in D17088, which I have incorporated here:

  • Take a backup of the new newsyslog file before clobbering it with the old one
  • Fix tense in UPDATING

Can you please let me know if you are ok with these latest changes?
Thanks,
Ben

This revision now requires review to proceed.Nov 12 2018, 2:57 PM
mat added a comment.Nov 12 2018, 6:12 PM

Mmmm, I think if the new file is already there and different from the sample file, the old file should simply be removed. And please, don't create backups in the newsyslog.d directory, the files will get parsed and used, and I am not sure what newsyslog will end up doing, but I don't think it will be right.

If the old file exists, and the new file already exists and is different from the sample file, our options (that I can think of):

  1. Clobber the new file by moving the old file into place (print warning)
  2. Backup the new file to another directory (e.g. /var/backups/), then clobber is with the old file (print warning)
  3. Delete the old file (print warning)
  4. Do nothing, leaving old file and new file in place (print warning)
  5. Provide interactive prompt to user during pkg-install (not unheard of) to chose between these options

As long as #5 isn’t too unusual, I think it would be my preference, but open to ideas/recommendations.

Would you still like me to proceed with #3?

mat added a comment.Nov 13 2018, 9:44 AM

Like I said, delete the old file.

woodsb02 updated this revision to Diff 50357.Nov 13 2018, 10:54 AM

Modify pkg-install script - when moving the configuration file from the old location to the new location, if the file in the new location has been modified from the sample file, then delete the file from the old location instead.
The logic behind this is that if the file is the new location varies from the sample file, then the sysadmin must have modified it (knowing about the new location), and therefore the file in the old location is obsolete.

mat added inline comments.Nov 13 2018, 11:01 AM
sysutils/munin-master/pkg-install
69–71 β†—(On Diff #50357)
if cmp -s ${samplefile} ${newfile} > /dev/null
sysutils/munin-node/pkg-install
21–23 β†—(On Diff #50357)

same.

woodsb02 updated this revision to Diff 50358.Nov 13 2018, 11:38 AM

Change check of new configuration file status to: if cmp -s ${samplefile} ${newfile} > /dev/null
Note that this introduces a functional change: if either the newfile or samplefile do not exist, it will be seen as a deliberate change by the sysadmin and therefore delete the oldfile

woodsb02 marked 2 inline comments as done.Nov 13 2018, 11:38 AM
mat added inline comments.Nov 13 2018, 2:14 PM
sysutils/munin-master/pkg-install
72 β†—(On Diff #50358)

Don't you need to mv -f or something so that the existing file is overwritten?

81 β†—(On Diff #50358)

Remove extra blank line.

sysutils/munin-node/pkg-install
33 β†—(On Diff #50358)

Remove extra blank line.

mat added a comment.Nov 13 2018, 3:16 PM

As a side note, we're going to have the same problem with cron. The it reads all files in /etc/cron.d and /usr/local/etc/cron.d.

woodsb02 updated this revision to Diff 50516.Nov 17 2018, 6:59 AM
  • Add -f to mv to override file without prompting
  • Remove extra blank lines
woodsb02 marked 3 inline comments as done.Nov 17 2018, 7:00 AM
mat accepted this revision.Nov 23 2018, 10:36 AM
This revision is now accepted and ready to land.Nov 23 2018, 10:36 AM
This revision was automatically updated to reflect the committed changes.