-
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
Disable mod_php on EL9 #2277
Disable mod_php on EL9 #2277
Conversation
d358efa
to
0cc48f5
Compare
manifests/mod/php.pp
Outdated
@@ -34,7 +35,7 @@ | |||
String $template = 'apache/mod/php.conf.erb', | |||
Optional[String] $source = undef, | |||
Optional[String] $root_group = $apache::params::root_group, | |||
Optional[String] $php_version = $apache::params::php_version, | |||
String[1] $php_version = $apache::params::php_version, |
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.
Rather than this would it not be better to put a check in to detect when its running on RHEL 9 and fail with a message stating that php-fpm
should be used instead?
i.e.:
if $facts['os']['name'] == 'Redhat' and $facts['os']['release']['major'] == '9' {
fail("RHEL 9 does not support 'mod_php' and so 'php-fpm' must be used instead.")
}
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.
Make the reasoning and cause explicit?
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.
How about this solution: keeping it Optional, but treat undef
as unsupported?
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.
Sound's good to me
0cc48f5
to
330bcd5
Compare
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
Soon as the tests pass will merge in and restart release
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.
Ah, look's like you need to update ./spec/classes/mod/php_spec.rb:98
to expect a failure when run on Redhat 9.
Which is weird since it shouldn't ben running against it anymore?
330bcd5
to
1177959
Compare
Unit tests run on all supported OSes (according to metadata.json) |
1177959
to
3fcaec7
Compare
Ah right, the unsupported comment works on the acceptance test |
EL 9 doesn't ship mod_php, so this is uninstallable. By making it a required parameter (which effectively was already the case) and setting the version to undef the catalog fails to compile by default. This still allows users to set it if they build a mod_php somehow. EL 9 users should use php-fpm instead. Fixes: edbdb65
3fcaec7
to
51bc232
Compare
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
:) |
EL 9 doesn't ship mod_php, so this is uninstallable. By making it a required parameter (which effectively was already the case) and setting the version to undef the catalog fails to compile by default. This still allows users to set it if they build a mod_php somehow.
EL 9 users should use php-fpm instead.
A draft right now to see which tests fail.
Fixes: edbdb65