-
Notifications
You must be signed in to change notification settings - Fork 331
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
Support clear_env variable by PHP_CLEAR_ENV #406
Conversation
6546d6c
to
5c4aa5f
Compare
5c4aa5f
to
45c8545
Compare
[test] |
@pkubatrh @hhorak @remicollet Tests passed. Can you please have a look at it and made a review? Your review is more than appropriate. Thanks. The change is valid only for RHEL9 and only in case of |
It would be good to include more info in the commit messages, a summary from #403 might work to explain basic background why clearing environment is not desired sometimes. |
We should likely document this new variable in the README.md, shouldn't we? |
45c8545
to
82544b0
Compare
README.md files are updated. Thanks for the point. |
I have tried to summarize it somehow. If something is still unclear, please let me know. |
82544b0
to
70decf4
Compare
[test] |
@remicollet Hi Remi, once you have free or spare time. Can you please have a look at this pull request? Thank you. |
78c5d8e
to
6b7dcff
Compare
[test] |
@phracek @remicollet @hhorak I would love to see this merged... I think it got stuck somewhere in the nirvana... :-) |
I am gonna finish this next week, so also tests are passing. Please be patient. |
Actually I didn`t see that someone worked on it last week. My bad. |
6b7dcff
to
431f16e
Compare
[test-openshift] |
Looks good to me, but i can only test once it is merged. |
Let's re-run tests after fixing some issues which are fixed by 76d4cf1 [test] |
[test-openshift] |
@remicollet @pkubatrh @hhorak Please review it. So we can merge it. |
76d4cf1
to
65a578a
Compare
Pull request was rebased against master [test-all] |
[test] |
I don't understand the change
We already have in https://github.com/sclorg/s2i-php-container/blob/master/8.1/root/usr/share/container-scripts/php/common.sh#L47
This sed comment probably need to be fixed, as the default line in RPM provided file is
|
Thanks for the review, I will fix it. You are right. |
@remicollet Good comment. In case of PHP_CLEAR_ENV is set to
Do we want to handle all those states? The condition that will handled all those cases would be a bit complicated. I did not try it yet ;) But can try |
AFAIK IIUC the current code, is that we always expect Perhaps a simple fix of the sed is enough.
And If you really want a new config, so:
|
Closes #403 Setting variable PHP_CLEAR_ENV clears environment in FPM workers. Prevents arbitrary environment variables from reaching FPM worker processes by clearing the environment in workers before env vars specified in this pool configuration are added. Default value: no. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
65a578a
to
5951bd7
Compare
Rebased and added also to 8.2 version. |
Add sed command instead of echo. Thanks to @remicollet Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
5951bd7
to
0369b9d
Compare
[test] |
[test-openshift] |
Tests are passing. Completely. |
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
[test] |
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.
LGTM
Hi @phracek, sorry for my late reply, but I just noticed it. It looks like having this functionality only in run script should be sufficent for all use cases. No need for functionality container-setup (unless it is also prt of some run activity) and assemble, but maybe there are reasons for it. Just a comment. No need to fix. |
This pull request fixes issue #403.
By parameter
PHP_CLEAR_ENV=OFF
user can set 'clear_env = false' in /etc/pphp-fpm.d/www.conf file.