-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
(MODULES-4049) SLES support #1554
Conversation
@@ -1,11 +1,18 @@ | |||
class apache::mod::cgi { | |||
$_lib_path = $::apache::lib_path |
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.
Where is this being used?
} elsif defined(Class['::apache::mod::prefork']){ | ||
$suse_path = '/usr/lib64/apache2-prefork' | ||
} else { | ||
$suse_path = pick('/usr/lib64/apache2-prefork', '/usr/lib64/apache2-worker') |
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.
technically this will always pick the first one, since it's never undef.
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.
Also, since prefork would be the default case then you don't need the elsif
above.
apache::mod { 'info': } | ||
|
||
if $::osfamily == 'Suse' { | ||
if defined(Class['::apache::mod::worker']){ |
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.
Hmm. defined()
is parse-order dependent, but most likely this will work.
::apache::mod { $mod: | ||
package => $_package_name, | ||
package_ensure => $package_ensure, | ||
lib => 'mod_php5.so', | ||
id => "php${_php_major}_module", | ||
path => "${suse_lib_path}/mod_php5.so", | ||
path => "${$::apache::lib_path}/mod_php5.so", |
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.
wat? This isn't from a special path?
} elsif defined(Class['::apache::mod::prefork']){ | ||
$suse_path = '/usr/lib64/apache2-prefork' | ||
} else { | ||
$suse_path = pick('/usr/lib64/apache2-worker', '/usr/lib64/apache2-prefork') |
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.
ditto
} | ||
::apache::mod { 'ssl': | ||
package => $package_name, | ||
lib_path => $suse_path |
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.
technically this resource declaration doesn't need to be duplicated because lib_path => $suse_path
when undef would be as if it wasn't passed at all
@@ -27,23 +27,14 @@ | |||
default => "${secdefaultaction},log", | |||
} | |||
|
|||
if $::osfamily == 'FreeBSD' { | |||
if $::osfamily == 'FreeBSD' or ($::osfamily == 'Suse' and $::operatingsystemrelease < '11') { |
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.
This should be in a separate block with it's own unique failure msg.
@@ -118,11 +118,18 @@ | |||
} | |||
} | |||
} | |||
'debian', 'freebsd', 'Suse': { | |||
'debian', 'freebsd' : { |
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.
get rid of this space. /nit
::apache::mpm{ 'worker': | ||
apache_version => $apache_version, | ||
} | ||
} | ||
'Suse' : { |
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.
this space too. /nit
::apache::mpm{ 'worker': | ||
apache_version => $apache_version, | ||
} | ||
} | ||
'Suse' : { | ||
::apache::mpm{ 'worker': |
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.
add a space here. /nit
@@ -120,10 +137,8 @@ | |||
} | |||
} | |||
|
|||
if versioncmp($apache_version, '2.4') < 0 { |
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.
hmm.. this change wasn't SLES specific...are we changing debian behavior here on purpose?
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.
it's difficult to see, but it is in the SLES block
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.
haha. doh. stupid github code folding...
@@ -40,6 +40,12 @@ | |||
shell 'apt-get install -y ssl-cert' | |||
end | |||
|
|||
if fact('operatingsystem') == 'SLES' | |||
#generate certs for SLES | |||
shell 'zypper install -y apache2' |
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.
hmm... is this what we want to do here? Doesn't this circumvent the module installing apache using the platform provider?
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 wasn't sure what to do here. gensslcert is part of apache2 and seemed like the easiest route to uniform certificate creation across SLES versions. there is the whole circumventing the module thing. i could move gensslcert activity into only the tests that require it.
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.
👍 on making it isolated to test environments. I think installing/managing packages like that fall under the scope of the users' sys administration but in this context... we also don't want to muck up the rest of the tests but installing apache2 differently.
👍 with bmjen's comments |
238d301
to
a3f4d15
Compare
No description provided.