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.
Tags
None
Referenced Files
F80132290: D17089.id50516.diff
Thu, Mar 28, 8:25 AM
Unknown Object (File)
Sat, Mar 2, 10:06 AM
Unknown Object (File)
Feb 10 2024, 9:28 AM
Unknown Object (File)
Feb 6 2024, 8:28 AM
Unknown Object (File)
Jan 29 2024, 4:14 AM
Unknown Object (File)
Jan 20 2024, 8:29 AM
Unknown Object (File)
Jan 13 2024, 9:53 AM
Unknown Object (File)
Jan 8 2024, 7:05 AM
Subscribers
None

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
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 19473
Build 19066: arc lint + arc unit

Event Timeline

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.

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.

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

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"

Abandon this change - relocating existing conf files violates POLA.

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

Add entries to UPDATING and pkg-message

This revision is now accepted and ready to land.Nov 12 2018, 11:24 AM

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

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?

Like I said, delete the old file.

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.

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.

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

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.

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.

  • Add -f to mv to override file without prompting
  • Remove extra blank lines
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.