-
-
Notifications
You must be signed in to change notification settings - Fork 268
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 extensions if ensured "absent" #263
Conversation
Hi @mbrodala, thanks for the PR. Are you able to add tests as well? |
@bastelfreak I was not able to find a test for the "enable" path and I'm not that well with writing specs but I'll give it a try. |
a7daa55
to
c70c147
Compare
I cannot manage to make the test successful ATM so if someone else could have a look, that'd be great. |
@bastelfreak Could you have a look? |
c70c147
to
83c85bf
Compare
Could somebody have a look at this? |
I'd like this feature too 😄 |
manifests/extension.pp
Outdated
|
||
if $::osfamily == 'Debian' and $ext_tool_enabled { | ||
$cmd = "${ext_tool_enable} -s ${sapi} ${lowercase_title}" | ||
if $ensure == 'absent' { | ||
$cmd = "${ext_tool_disable} -s ${final_sapi} ${lowercase_title}" |
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 think we should use ${sapi}
instead of ${final_sapi}
here (otherwise the 'ALL' will never work)
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.
Hm, it's been a while. So you'd suggest to drop $final_sapi
?
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.
nope, final_sapi
should still be used with phpquery
(in the unless
or onlyif
statements) as phpquery doesnt support the ALL
option.
usage: phpenmod [ -v ALL|php_version ] [ -s ALL|sapi_name ] module_name [ module_name_2 ]
usage: phpdismod [ -v ALL|php_version ] [ -s ALL|sapi_name ] module_name [ module_name_2 ]
usage: phpquery [ -d ] [ -q ] -v version_name -s sapi_name [ -m module_name ] [ -M ] [ -S ] [ -V ]
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.
OK done.
manifests/extension.pp
Outdated
} | ||
} else { | ||
$cmd = "${ext_tool_enable} -s ${final_sapi} ${lowercase_title}" |
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.
Same here.
I think we should use ${sapi}
instead of ${final_sapi}
(otherwise the 'ALL' will never work)
@mbrodala , could you have a look at my comments and make the corresponding changes. That might fix some of the travis failures. Regards, |
manifests/init.pp
Outdated
@@ -130,6 +134,7 @@ | |||
$package_prefix = $::php::params::package_prefix, | |||
$config_root_ini = $::php::params::config_root_ini, | |||
$ext_tool_enable = $::php::params::ext_tool_enable, | |||
$ext_tool_disable = $::php::params::ext_tool_disable, |
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 fix the indendation and add a datatype
manifests/init.pp
Outdated
@@ -161,6 +166,9 @@ | |||
if $ext_tool_enable != undef { | |||
validate_absolute_path($ext_tool_enable) | |||
} | |||
if $ext_tool_disable != undef { | |||
validate_absolute_path($ext_tool_disable) |
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.
those three lines are not needed with the new datatype
Hi @mbrodala, can you take a look at the inline comments I did and also rebase against our current master branch? Feel free to join our IRC channel #voxpupuli on freenode if you need any help with puppet or git. |
13ccd36
to
7c3be0f
Compare
I've tried to update everything to the latest state as good as I can. 🤞 |
7c3be0f
to
1ff4205
Compare
What is left is fixing the tests which is a bit out of scope for me. |
besides the tests, this also needs a rebase. I'm going to close this PR due to inactivity. Please reopen it if you're still interested in it. |
Well, I've asked for help, that's all I can do ATM. And of course, this still needs to be fixed. |
Fixes #262