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

split fpm config to two parts #903

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@glensc
Contributor

glensc commented Nov 18, 2014

main config = global options
secondary config = pool options

makes easier to add new pools:
duplicate pool config in fpm.d dir

https://bugs.php.net/bug.php?id=67106

as i understand from bug report comments, currently "lowest version" is master, so this PR is against master branch

@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Nov 20, 2014

Contributor

This is already used in some downstream distribution (like Fedora / RHEL / CentOS)
I will prefer /etc/php-fpm.d
And at least a new unit test should be added (lot of have be recently added because lack of covereage for this SAPI)

Contributor

remicollet commented Nov 20, 2014

This is already used in some downstream distribution (like Fedora / RHEL / CentOS)
I will prefer /etc/php-fpm.d
And at least a new unit test should be added (lot of have be recently added because lack of covereage for this SAPI)

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Nov 20, 2014

Contributor

the path can be made configurable with @fpm_conf_d_dir@ or similar, it's not a problem. thanks for taking interest!

Contributor

glensc commented Nov 20, 2014

the path can be made configurable with @fpm_conf_d_dir@ or similar, it's not a problem. thanks for taking interest!

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Nov 20, 2014

Contributor

but what kind of test would you expect from config split? tests don't use installed configs, but their own.

Contributor

glensc commented Nov 20, 2014

but what kind of test would you expect from config split? tests don't use installed configs, but their own.

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Nov 20, 2014

Contributor

looks like it can be set like:

./php-fpm.conf.in:125:include=@php_fpm_sysconfdir@/php-fpm.d/*.conf

that should expand to: /etc/php-fpm.d on your system. is that ok @remicollet?

Contributor

glensc commented Nov 20, 2014

looks like it can be set like:

./php-fpm.conf.in:125:include=@php_fpm_sysconfdir@/php-fpm.d/*.conf

that should expand to: /etc/php-fpm.d on your system. is that ok @remicollet?

@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Nov 21, 2014

Contributor

that should expand to: /etc/php-fpm.d on your system. is that ok @remicollet?

IMHO opinion, yes (but I'm not the only one which can say something)

but what kind of test would you expect from config split? tests don't use installed configs, but their own.

Forget, I already add a test for splitted configuration (to check load order after PR 906 , bug 68391)

Contributor

remicollet commented Nov 21, 2014

that should expand to: /etc/php-fpm.d on your system. is that ok @remicollet?

IMHO opinion, yes (but I'm not the only one which can say something)

but what kind of test would you expect from config split? tests don't use installed configs, but their own.

Forget, I already add a test for splitted configuration (to check load order after PR 906 , bug 68391)

@smalyshev

This comment has been minimized.

Show comment
Hide comment
@smalyshev

smalyshev Nov 23, 2014

Contributor

Needs rebase

Contributor

smalyshev commented Nov 23, 2014

Needs rebase

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Nov 26, 2014

Contributor

@smalyshev of course it does, this patch is against master branch!

do you approve this change and intend to merge? because rebasing such PR each time someone makes a change is very annoying, because i would need to manually redo whole file splitting again.

i would rebase it just before final merge.

Contributor

glensc commented Nov 26, 2014

@smalyshev of course it does, this patch is against master branch!

do you approve this change and intend to merge? because rebasing such PR each time someone makes a change is very annoying, because i would need to manually redo whole file splitting again.

i would rebase it just before final merge.

@smalyshev

This comment has been minimized.

Show comment
Hide comment
@smalyshev

smalyshev Dec 1, 2014

Contributor

@glensc I'm fine with it but I'd like one of FPM maintainers to sign off also, since I don't know enough FPM.

Contributor

smalyshev commented Dec 1, 2014

@glensc I'm fine with it but I'd like one of FPM maintainers to sign off also, since I don't know enough FPM.

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 12, 2014

Contributor

@smalyshev can you ping them?

Contributor

glensc commented Dec 12, 2014

@smalyshev can you ping them?

@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Dec 13, 2014

Contributor

Please rebase, I think there is no other configuration change pending.

Contributor

remicollet commented Dec 13, 2014

Please rebase, I think there is no other configuration change pending.

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 13, 2014

Contributor

rebased once again.

Contributor

glensc commented Dec 13, 2014

rebased once again.

@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Dec 14, 2014

Contributor

Patch looks fine, and works.

2 minor cosmetic hanges

  • please rename php-fpm.conf-d.in (ugly name imho) to www.conf.in
  • as you add the "glob / include" at the end (under pool definition), I think it will make sense to drop it from the beginning of php-fpm.conf.in, to keep only one (and only 1 include is currently allowed, see 68022)
Contributor

remicollet commented Dec 14, 2014

Patch looks fine, and works.

2 minor cosmetic hanges

  • please rename php-fpm.conf-d.in (ugly name imho) to www.conf.in
  • as you add the "glob / include" at the end (under pool definition), I think it will make sense to drop it from the beginning of php-fpm.conf.in, to keep only one (and only 1 include is currently allowed, see 68022)
@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Dec 14, 2014

Contributor

Good catch in 4357e32
But this one have to go in 5.6
But this will mean another rebase... sorry...

Contributor

remicollet commented Dec 14, 2014

Good catch in 4357e32
But this one have to go in 5.6
But this will mean another rebase... sorry...

@remicollet

This comment has been minimized.

Show comment
Hide comment
@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 14, 2014

Contributor

aside, current the configs are installed with .default extension, and none installed at all with base extension, meaning by default there's no working config, end user has to rename two files

so, imho
(in another pr after this gets merged), should copy foo.conf.default to foo.conf if foo.conf does not exist. or make the config install more rpm-like: install foo.conf if no such exist, otherwise use foo.conf.default

Contributor

glensc commented Dec 14, 2014

aside, current the configs are installed with .default extension, and none installed at all with base extension, meaning by default there's no working config, end user has to rename two files

so, imho
(in another pr after this gets merged), should copy foo.conf.default to foo.conf if foo.conf does not exist. or make the config install more rpm-like: install foo.conf if no such exist, otherwise use foo.conf.default

split fpm config to two parts. PR#903
main config = global options
secondary config = pool options

makes easier to add new pools:
duplicate pool config in fpm.d dir

https://bugs.php.net/bug.php?id=67106
@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 14, 2014

Contributor

i had the "include" directive once in first version patch (moved to the bottom), but with the flood of rebases, seems it got lost or i thought differently.

Contributor

glensc commented Dec 14, 2014

i had the "include" directive once in first version patch (moved to the bottom), but with the flood of rebases, seems it got lost or i thought differently.

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 14, 2014

Contributor

rebased again

Contributor

glensc commented Dec 14, 2014

rebased again

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Dec 14, 2014

Comment on behalf of remi at php.net:

merged

php-pulls commented Dec 14, 2014

Comment on behalf of remi at php.net:

merged

@php-pulls php-pulls closed this Dec 14, 2014

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Dec 15, 2014

Contributor

wohoo!

Contributor

glensc commented Dec 15, 2014

wohoo!

glensc added a commit to pld-linux/php that referenced this pull request Jul 10, 2017

glensc added a commit to pld-linux/php that referenced this pull request Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment