Skip to content

Conversation

@pstray
Copy link

@pstray pstray commented Sep 24, 2020

Since we already use $ssl to add "_ssl" to the log filenames, and the
examples for a mix of SSL and not typically would result in logfiles
with names like "mix.example.com non-ssl_access.log" and
"mix.example.com ssl_access_ssl.log", this patch fixes that by using
$servername instead and thus getting what it probably more expected:
"mix.example.com_access.log" and "mix.example.com_access_ssl.log"

Since we already use $ssl to add "_ssl" to the log filenames, and the
examples for a mix of SSL and not typically would result in logfiles
with names like "mix.example.com non-ssl_access.log" and
"mix.example.com ssl_access_ssl.log", this patch fixes that by using
$servername instead and thus getting what it probably more expected:
"mix.example.com_access.log" and "mix.example.com_access_ssl.log"
@pstray pstray requested a review from a team as a code owner September 24, 2020 16:23
@puppet-community-rangefinder
Copy link

apache::vhost is a type

Breaking changes to this file WILL impact these 125 modules (exact match):
Breaking changes to this file MAY impact these 32 modules (near match):

This module is declared in 174 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@5323b8d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2068   +/-   ##
=======================================
  Coverage        ?   58.49%           
=======================================
  Files           ?       12           
  Lines           ?      212           
  Branches        ?        0           
=======================================
  Hits            ?      124           
  Misses          ?       88           
  Partials        ?        0           

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 5323b8d...3b898f1. Read the comment docs.

@sanfrancrisko
Copy link
Contributor

Thanks for the suggested tweak @pstray .

There's another PR currently open that is proposing changes in the same area: #2064

I'll discuss with the IAC Team on how we want to proceed with both of these changes. Both very useful, however, we need to take in to consideration how we publicise and version this, as there could be assumptions in place about log file name formats, for other tools, etc...

@sanfrancrisko
Copy link
Contributor

Hi @pstray

Further to my comment above, we have a ticket raised to implement this work and #2064's in a manner that will allow us to do it in a minor version bump: https://tickets.puppetlabs.com/browse/IAC-1186

We'll raise a PR that closes this one and request your review on it too.

@sanfrancrisko sanfrancrisko self-assigned this Nov 2, 2020
sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.

This commit introduces
sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.

This commit introduces
sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.

This commit introduces
sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.
sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.
@sanfrancrisko
Copy link
Contributor

Hi @pstray

Would you be able to have a peek at #2086 and see if it would be a satisfactory combination of this PR and #2064 's work

sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 9, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.
@pstray
Copy link
Author

pstray commented Nov 10, 2020

Yes that seems like a good way to do it

sanfrancrisko pushed a commit to sanfrancrisko/puppetlabs-apache that referenced this pull request Nov 10, 2020
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.
@sanfrancrisko
Copy link
Contributor

Thanks @pstray , I'll close this PR given #2086 incorporates this work (I thought it would have been automatically closed from that PR, strangely).

Thanks again for your suggestion.

daveseff pushed a commit to daveseff/puppetlabs-apache that referenced this pull request Jul 19, 2022
Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As puppetlabs#2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is puppetlabs#2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both puppetlabs#2064
and puppetlabs#2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants