Skip to content
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

Small Docker fixes #34

Merged
merged 6 commits into from
Nov 12, 2023
Merged

Small Docker fixes #34

merged 6 commits into from
Nov 12, 2023

Conversation

evanebb
Copy link
Contributor

@evanebb evanebb commented Nov 11, 2023

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
Copy link
Contributor

dqos commented Nov 11, 2023

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
Copy link
Contributor Author

evanebb commented Nov 12, 2023

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
Copy link
Contributor

dqos commented Nov 12, 2023

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

@dqos dqos merged commit 9cf9388 into hybula:main Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants