search

geerlingguy / ansible-role-php

Ansible Role - PHP
https://galaxy.ansible.com/geerlingguy/php/
MIT License
496 stars 441 forks source link

Backwards compatibility broken #329

Closed tjoelsson closed 3 years ago

tjoelsson commented 3 years ago

The recently merged PR https://github.com/geerlingguy/ansible-role-php/pull/302 broke backwards compatibility. FPM configuration items for pool settings have been renamed. If you have these configured they no longer have any effect and default config will be used:

php_fpm_listen
php_fpm_listen_allowed_clients
php_fpm_pm_max_children
php_fpm_pm_start_servers
php_fpm_pm_min_spare_servers
php_fpm_pm_max_spare_servers
geerlingguy commented 3 years ago

@rsicart had submitted that PR. Right now I don't have time to work on fixing this myself, but I would be willing to merge in a fix that preserves the backwards compatibility. Otherwise I'd also be willing to revert that PR since it's the last thing that was added and then we can look at getting something like it added again at some point in the future...

rossmitchell commented 3 years ago

Just hit this today. It also changes the pool location if you had configured it by setting the php_fpm_pool_conf_path to use a file other than www.conf

geerlingguy commented 3 years ago

Can you see if https://github.com/geerlingguy/ansible-role-php/pull/330 would fix?

rossmitchell commented 3 years ago

Not as it is at the moment. The destination is built using the conf path and the pool name here https://github.com/rsicart/ansible-role-php/blob/c62db9bbc1eb6864c257e17b0a6e9898926e48b9/tasks/configure-fpm.yml#L39

I think it could be fixed by adding a new property for the file name and defaulting it to php_fpm_pool_conf_path | basename and then using that when creating the pools

geerlingguy commented 3 years ago

@shakalandy - Could you see if you can get it to work that way instead?

tjoelsson commented 3 years ago

330 Looks ok to me. It solves my issue.

shakalandy commented 3 years ago

will check that one tomorrow or later @geerlingguy @rossmitchell

tjoelsson commented 3 years ago

Any update? How about merging #330 first, then looking into additional problems in separate issues?

geerlingguy commented 3 years ago

Merged #330; we can solve any other issues as they crop up after.

shakalandy commented 3 years ago

@rossmitchell could you please open a new ticket? I'll try to build a fix for that.

rossmitchell commented 3 years ago

@shakalandy I've created a new ticket for the pool name. I can take a look at it over the weekend and see if I can get a quick fix pushed up unless you want to look at it first

tjoelsson commented 3 years ago

Thank you!