thias / puppet-php

Puppet module to manage PHP
Other
49 stars 67 forks source link

Fixed bug with the process priority #26

Closed Nyholm closed 10 years ago

Nyholm commented 10 years ago

I've created this PR as a follow up after #25. There was a problem when PR #25 was manually merged. The setting $process_priority should be a setting for each PHP-FPM pool. Not the daemon.

Consider using Githubs merge function to avoid these kind of errors in the future.

thias commented 10 years ago

Now I'm confused... I followed the upstream configuration found here : https://raw.githubusercontent.com/php/php-src/master/sapi/fpm/php-fpm.conf.in

Indeed, I didn't merge your PR as-is because it was adding to the configuration file in an order different from the original file (I try to always keep templates as close as possible to the default provided files, it makes reviewing diffs easier).

And, from what I understood :

So now I really don't see how the change you propose here fixes anything. Is the example configuration from upstream wrong? Did I understand something wrong and/or get something mixed up?

Nyholm commented 10 years ago

Okey. We both made a misstake here. I did not see that you added priority on the php::fpm::conf. That's fine, sorry about that.

You have mistaken renamed process.priority to priority in the per pool config. Per Pool :

The current release 0.3.11 will generate invalid config files and php-fpm will not start.

I've modified this PR with this update.

thias commented 10 years ago

Aha! Fun! I just clicked on the link I posted, and there was no longer a "priority" in there!!! It's because this has been merged a few hours ago : https://github.com/php/php-src/commit/636adf251c80884c151cb233bbdae1497fdadc4a

I'm not crazy, and following the upstream file lead to the wrong name. It all makes sense now :-)

thias commented 10 years ago

Could you please update your PR to have the parameter called $process_priority in both cases? I could do it, but you seem keen on me including your PRs as-is ;-)

Nyholm commented 10 years ago

Okey. =)

Sure. The PR is updated. Thank you.

thias commented 10 years ago

0.3.12 released to the forge!