search

linuxserver / docker-grocy

A container for grocy - the ERP application for your kitchen https://grocy.info
GNU General Public License v3.0
340 stars 43 forks source link

Enable configuration through environment variables by disabling php-fpm/clear_env #85

Closed benedicteb closed 6 months ago

benedicteb commented 6 months ago

linuxserver.io

Description:

By default php-fpm has clear_env option activated.

This means that environment variables never will be passed on from the system environment to the actual worker process for a given request.

And because of that it's not possible to configure Grocy with environment variables even though it's advocated for in the default configuration file. It seems other users already have struggled with this, with this image, for example in #12.

We can fix this by disabling the clear_env in a .d file for php-fpm. This PR introduces that file, which will be copied automatically by the ADD at the bottom of Dockerfile.

Benefits of this PR and context:

The clear benefit is that you'll be able to run Grocy completely declarative without editing any files.

There are some reasons as to why clear_env = yes is default in php-fpm. For example a note on performance. That forwarding the environment variables has certain performance implications in high traffic situations. Yet I believe Grocy won't be an app usually run in such situations and that the performance impact isn't so big that you'll notice it very soon.

There also might be some security implications. For example on multi-tenant setups. Yet that seems unlikely to apply here since we use containers for that kind of isolation.

It is also worth noting that in the official php docker image clear_env is actually disabled. See this thread for info.

How Has This Been Tested?

I have tested this by building the image locally, running it and verifying it now passes on configuration via environment variables.

You can test it yourself by for example changing the currency via an environment variable:

docker run -e GROCY_CURRENCY=NOK -p 8080:80 linuxserver/grocy:latest

Then opening:

http://localhost:8080/stockoverview

And verifying that the app actually has NOK as the currency. There's more info on how Grocy handles configuration, here:

https://github.com/grocy/grocy/blob/master/config-dist.php#L3

Source / References:

LinuxServer-CI commented 6 months ago
I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-df5e89f7d0e4c60092e2195e68a2e30d49613c26-pr-85/index.html https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-df5e89f7d0e4c60092e2195e68a2e30d49613c26-pr-85/shellcheck-result.xml Tag Passed
amd64-v4.2.0-pkg-4a9529da-dev-df5e89f7d0e4c60092e2195e68a2e30d49613c26-pr-85 ✅
arm64v8-v4.2.0-pkg-4a9529da-dev-df5e89f7d0e4c60092e2195e68a2e30d49613c26-pr-85 ✅
LinuxServer-CI commented 6 months ago
I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-bdbabe2b04b1726be4243aa8fbdbe95fbcd5dabc-pr-85/index.html https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-bdbabe2b04b1726be4243aa8fbdbe95fbcd5dabc-pr-85/shellcheck-result.xml Tag Passed
amd64-v4.2.0-pkg-4a9529da-dev-bdbabe2b04b1726be4243aa8fbdbe95fbcd5dabc-pr-85 ✅
arm64v8-v4.2.0-pkg-4a9529da-dev-bdbabe2b04b1726be4243aa8fbdbe95fbcd5dabc-pr-85 ✅
aptalca commented 6 months ago

Thanks for the PR. Sure, since upstream advertises the use of ENV vars, we can/should enable php to read them.

Can you please change the method to match what we do for Nextcloud? https://github.com/linuxserver/docker-nextcloud/blob/master/Dockerfile#L55-L56

benedicteb commented 6 months ago

Thanks for the review! 😊

I copied the sed/grep lines from the Nextcloud repo into both the Dockerfiles and performed the same tests again. It works well!

Also squashed/rebased the commits to make a bit more sense. Let me know if there's anything else y'all need to get this merged 😊

LinuxServer-CI commented 6 months ago
I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-b0f407bab5605fdcd67ea2f44378663acdfca8f8-pr-85/index.html https://ci-tests.linuxserver.io/lspipepr/grocy/v4.2.0-pkg-4a9529da-dev-b0f407bab5605fdcd67ea2f44378663acdfca8f8-pr-85/shellcheck-result.xml Tag Passed
amd64-v4.2.0-pkg-4a9529da-dev-b0f407bab5605fdcd67ea2f44378663acdfca8f8-pr-85 ✅
arm64v8-v4.2.0-pkg-4a9529da-dev-b0f407bab5605fdcd67ea2f44378663acdfca8f8-pr-85 ✅