-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Update to support Apache 2.4 - rework of #136 #189
Conversation
manifests/params.pp
Outdated
case $::osfamily { | ||
'Debian': { | ||
if $::operatingsystem == ubuntu { | ||
$apache_confd = '/etc/apache2/conf-enabled' | ||
if ($::operatingsystem == 'ubuntu') { |
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.
You probably should use upper case naming to be consistent. Unless the Puppet logic for $::osfamily
cares about case and the logic for $::operatingsystem
does not.
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.
can you please switch to the new facts hash? $facts['os']['family']
manifests/params.pp
Outdated
# puppetlabs/apache names the apache service | ||
# resource 'httpd', nevermind the actual os | ||
$apache_service = 'httpd' | ||
|
||
case $::osfamily { |
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.
please use the facts hash here. $facts['os']['family']
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.
I did see those, and had left them the same as the original PR. I added an additional commit that should switch to structured facts.
Acceptance tests are another story; they were out of date, so I'm working on another pull to at least somewhat fix them.
manifests/params.pp
Outdated
if $::operatingsystem == ubuntu { | ||
$apache_confd = '/etc/apache2/conf-enabled' | ||
if ($::operatingsystem == 'ubuntu') { | ||
if (versioncmp($::operatingsystemrelease,'14.04')) { |
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.
please use the facts hash here. Probably $facts['os']['release']['full']
This reworks #136 by @steveames
Still probably could use some review, though it does get the tests passing and is rebased against current master.