-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Correct vhost parameter documentation and run vhost tests on all supported OSes #2257
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
Correct vhost parameter documentation and run vhost tests on all supported OSes #2257
Conversation
This makes it easier to see if a mod is present.
The previous location was testing it on a vhost that's absent. This is now testing it on the very complex test where it's present. This is important because in the future the whole directories concat fragment isn't rendered on absent vhosts.
This splits off the fastcgi tests to its own case with proper conditionals. This allows testing the entire define on all supported operating system versions. FastCGI is actually unavailable on all supported Debian versions, rather than just Ubuntu so those are skipped as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
end | ||
|
||
# this setup uses fastcgi wich isn't available on RHEL 7 / RHEL 8 / Debian / Ubuntu | ||
unless facts[:os]['family'] || (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'].to_i >= 7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyed that I missed this before I merged, but this doesn't look right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think there is a compare missing from the first part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, my bad. That should have been facts[:os]['family'] == 'Debian'
. I'm out today and tomorrow so if you haven't fixed it up before Monday I'll submit a patch then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: #2258
See invididual commits for details. This doesn't modify any code, it's just tests and docs.