Small Docker fixes #34

2023-11-12T05:29:55+08:00

evanebb commented

2023-11-12 05:29:55 +08:00

(Migrated from github.com)

This PR contains some small fixes for the Docker container(s) that I noticed:

Prevent requests for /favicon.ico from being sent to PHP-FPM, which fixes an error regarding an invalid CSRF token.
Currently, the optional Terms of Use feature thingy is always turned on, even if the corresponding LG_TERMS environment variable is not defined. This has been fixed, so that if this variable is not defined, it is not shown.
I have added the available environment variables with examples to the docker-compose.yml, and commented out the optional ones that probably shouldn't be turned on by default.
Fixes the paths to the files currently listed in the PHP-FPM Docker image's .dockerignore file; the build context of this image is set to the root of the repository, which means that any paths in the .dockerignore file should use this directory as the base as well.

This PR contains some small fixes for the Docker container(s) that I noticed: - Prevent requests for `/favicon.ico` from being sent to PHP-FPM, which fixes an error regarding an invalid CSRF token. - Currently, the optional Terms of Use feature thingy is always turned on, even if the corresponding `LG_TERMS` environment variable is not defined. This has been fixed, so that if this variable is not defined, it is not shown. - I have added the available environment variables with examples to the `docker-compose.yml`, and commented out the optional ones that probably shouldn't be turned on by default. - Fixes the paths to the files currently listed in the PHP-FPM Docker image's `.dockerignore` file; the build context of this image is set to the root of the repository, which means that any paths in the `.dockerignore` file should use this directory as the base as well.

dqos commented

2023-11-12 06:03:48 +08:00

(Migrated from github.com)

Thank you very much for your PR. Could you also set LG_TERMS in config.dist.php to false, so it matches the Docker config.php 👍🏽

Thank you very much for your PR. Could you also set `LG_TERMS` in config.dist.php to false, so it matches the Docker config.php 👍🏽

evanebb commented

2023-11-12 18:37:00 +08:00

(Migrated from github.com)

Done :)
I have also added a few installation steps for Docker to the README; these are very simple and don't cover things like enabling HTTPS and other things that you would want to do for production use.

Additionally, there are a few other things that could be done to make the Docker deployment method more of a first-class citizen, like:

Pushing pre-built images (tagged with their respective release) to a container registry like Docker Hub or GHCR, instead of having to build them locally.
Moving more configuration directives/toggles to environment variables, to avoid having to manually modify the configuration file and re-building the images.

But whether that is worth doing depends on whether the Docker images are meant for production use, or really just for testing purposes.

Done :) I have also added a few installation steps for Docker to the README; these are very simple and don't cover things like enabling HTTPS and other things that you would want to do for production use. Additionally, there are a few other things that could be done to make the Docker deployment method more of a first-class citizen, like: - Pushing pre-built images (tagged with their respective release) to a container registry like Docker Hub or GHCR, instead of having to build them locally. - Moving more configuration directives/toggles to environment variables, to avoid having to manually modify the configuration file and re-building the images. But whether that is worth doing depends on whether the Docker images are meant for production use, or really just for testing purposes.

dqos commented

2023-11-12 19:04:56 +08:00

(Migrated from github.com)

@evanebb thank you for your effort. Merging this for a new release!

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: malhuda/lookingglass#34