Thanks for the review team.
For the record, the old filename is printed out in the warning message if the user needs to do the cleanup - the echo command before the "if cmp" check.

As per the existing code, if the contents of ${newfile} are the same as ${samplefile} then there is no destruction to move ${oldfile} to ${newfile}. If the user wants to recover the content of the clobbered newfile - it is fully available in samplefile.

• Only warn the user if the old configuration file exists and the new file has been modified from the default (do not delete it)

Makes sense to me.

@mat preferred to delete the old file if the new file already exists and has been modified from the default. See further description below.

• Align with D17089 which has now been approved by portmgr and committed
• Incorporate logic if both old and new newsyslog file are present
• Note change to UPDATING was committed in rS485721 (wording changes proposed by mandree were incorporated)

Ok, I will abandon this change.
One thought I recently had, is we could have an adaptation of this change through logging in 12.0 release that simply alerted the user their system had incompatibilities with the upcoming change, and then 12.1 release could bring the change.
Any value in that?

Fair enough. That was my initial feel also, but I have been cautioned by others that there will be someone that doesn’t see the notices in UPDATING or the release notes, and after an hour of investigating why their log files have filled their disk they will find the change and curse my name. I do agree, there will always be at least one that doesn’t read the notes, and so thought I would propose the belt and braces approach to see what people thought.

• Add -f to mv to override file without prompting
• Remove extra blank lines

Note I am not a src committer (I have my ports commit bit only), so I would like this review to serve 2 purposes:

1. Is the idea and format of this warning good practice
2. Approval for me to commit to head (only after it has been on review for > 3 days)

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

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.

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

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

• Add pkg-install script to automatically move old newsyslog file to new filename
• Add entries to UPDATING and pkg-message

• Add pkg-install script to automatically move old newsyslog file to new filename
• Add entries to UPDATING and pkg-message

Add entries to UPDATING and pkg-message

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

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

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

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

No automation script unfortunately - just did it manually... which took ages! If you know of one please let me know :)

Hi @mat,
As portmgr, can you please approve the changes to Mk/bsd.gstreamer.mk?
Thanks,
Ben

This change has been reverted pending POLA dicusssions. Refer rS479438

In D17086#364678, @bapt wrote:

I think there is absolutely no POLA here as this is a configuration, there should be some warning in the release Release note about it and that is all imho.
Considering the option to only blacklist certain patterns: dotfiles, .pkgnew etc the problem is one would need to do more than that:
remove patch leftovers: .orig, .rej, .bak
editors leftovers file~ file.bak, etc

There are too many patterns to consider

I think I like cem's idea to only include files if they do not start with a "." or finish with ".pkgnew", ".sample" or ".bak" (any others?).

Abandon this change - relocating existing conf files violates POLA.

Abandon this change - relocating existing conf files violates POLA.

Abandon this change - relocating existing conf files violates POLA.

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

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

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?

Move all explicit_bzero commands to end of the for loop for each provider, as per asomers comment.