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

Only warn about servername logging if relevant #2154

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented May 26, 2021

The warning about $use_servername_for_filenames is long and may not affect users. If they use a normal $name, there would effectively be no difference. This compares $name to $normalized_servername. The warning is only relevant Only if they differ.

Technically there could be a space in $servername (since there is no data type to enforce validation) but Apache wouldn't accept that anyway.

It also fixes two typos in the warnings.

Please make sure my logic is correct here, I'm not 100% sure it is.

@ekohl ekohl requested a review from a team as a code owner May 26, 2021 10:33
@david22swan
Copy link
Member

@ekohl You seem to have a lot of test failures here, have you had a chance to look at them more closely?
Do you need ay assistance?

The warning about $use_servername_for_filenames is long and may not
affect users. If they use a normal $name, there would effectively be no
difference. This compares $name to $normalized_servername. The warning
is only relevant Only if they differ.

Technically there could be a space in $servername (since there is no
data type to enforce validation) but Apache wouldn't accept that anyway.

It also fixes two typos in the warnings.
@ekohl ekohl force-pushed the improve-warning branch from d509785 to 6a37b36 Compare June 15, 2021 12:49
@ekohl
Copy link
Collaborator Author

ekohl commented Jun 15, 2021

I think when I pushed this I was unable to run Puppet locally since the ecosystem was Ruby 3 incompatible. Right now rodjek/rspec-puppet#830 still makes it harder than it needs to be. Also that there's no puppet-module-posix-default-r3.0 is yet another reason I never use PDK. It's always late.

Looks like I had a syntax error which caused the whole tests to fail for obvious reasons.

What we do in Voxpupuli is a single test job that runs basic validations prior to running any other tests. It also determines the matrix. That saves a lot of CI resources on these trivial mistakes.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM

@david22swan
Copy link
Member

@ekohl Sorry for the trouble, but thank you for the work. This is the nice sort of change that only someone who actually work's with the module would think of, gonna go ahead and merge it.
Release containing it should be out later today.

Once again, thank you for putting in the effort :)

@david22swan david22swan merged commit e36de8d into puppetlabs:main Jun 21, 2021
@ekohl ekohl deleted the improve-warning branch June 21, 2021 10:28
@dhs-rec
Copy link

dhs-rec commented Jul 14, 2021

Hmm, I've just updated to 6.3.0, but I still see that warning although none of my vhosts have whitespace in their names.

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.

3 participants