Allow a custom location for the php-fpm error log #427
Conversation
There was a problem hiding this comment.
error log config
the option to configure the fpm error log can be quite handy, as far as i can see the changes should be backwards compatible. looks nice to me
remove of the default fpm pool
from my experience with php-fpm i can say that the remove of the default pool leads to problems. without any pool the fpm service is not starting at all. this can be the case if puppet install and starts php-fpm first and adds the custom pools in a later run.
additional removing the default pool is a BC break. it can break existing installations which use this module. so this PR can only be introduced in a major version.
if parameters are removed, the old one should be mapped to the new one and marked deprecated. so there is no bc break and the user of the module gets a deprecation warning.
you can easily control your configs by inlcuding this module like this:
class { '::php':
fpm_pools => {},
}
or with hiera:
your-manifest.pp
class { '::php': }
common.yaml
php::fpm_pools: {}
general
the PR would be much easier to review if styling of existing code is applied in the last step. so many lines are "touched" without a real code change.
all in all i would vote to reject this PR because of the BC break with no real benefits.
manifests/globals.pp
Outdated
| @@ -15,6 +15,7 @@ | |||
| Optional[Pattern[/^[57].[0-9]/]] $php_version = undef, | |||
| Optional[Stdlib::Absolutepath] $config_root = undef, | |||
| Optional[Stdlib::Absolutepath] $fpm_pid_file = undef, | |||
| Optional[Stdlib::Absolutepath] $fpm_error_log = undef, | |||
There was a problem hiding this comment.
syslog must be a valid option here
There was a problem hiding this comment.
syslog is a valid option, see the $default_fpm_error_log in the Archlinux block.
There was a problem hiding this comment.
Thank you for your reply about the fpm pools, this has helped.
But i still need to handle the different location of the error log file when using an RHSCL php version.
I know there is already a PR for including RHSCL in php module, but that is taking already a while to get merged.
Will fix the coding style.
There was a problem hiding this comment.
i am not sure about the Stdlib::Absolutepath will work for syslog as far as i can remember, the absolute path must start with a / see
- https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/absolutepath.pp
- https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/unixpath.pp
- https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/windowspath.pp
if i set php::globals::fpm_error_log: syslog in my common.yaml in hiera, i will get an error. so it should be Optional or Optional[Enum['syslog'], Stdlib::Absolutepath] or something like that (the correct value has to be tested).
There was a problem hiding this comment.
as you reverted the pool change, please rename this PR removed default www fpm pool and added parameter for error log file -> added parameter for error log file
|
Hi @DJMuggs, thanks for the PR! |
DJMuggs
changed the title
DJMuggs
changed the title
|
Will squash the commits pretty soon |
use String instead of Stdlib::Absolutepath for fpm_error_log to allow use of syslog
|
Your test fail because of the manage_repos => true for step 9 in your build ci process. It has to do with a libdb5.1 that can't be found when using wheezy repos on jessie. he following packages have unmet dependencies: Can you please fix? After this my pull request should be without errors. |
|
any progress on this ticket, i am using the php packages from remi and run into this problem too |
|
@attachmentgenie see my comment above |
I like the possibility to choose a custom error log location, very usefull if you use something like RHSCL in combination with this module.