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

Docker image improvements #655

Closed
wants to merge 5 commits into from

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Feb 2, 2022

The commits are individually reviewable. This includes:

  • the docker image is now multi-arch capable thanks to BuildKit. It does not rely on QEMU emulation, rather using BuildKit's TARGETPLATFORM and BUILDPLATFORM build args for doing the bundling under the host arch
  • use an unprivileged nginx base image to run as non-root. This also allows running the whole thing with a read-only root filesystem. It still needs a writable /tmp, e.g. docker run --publish 80:8080 --read-only --volume tmp:/tmp hydrogen
  • have the CI build a multi-arch image (currently building for x86_64, ARM and ARM64)
  • some updates to the documentation
  • a config.json templated from environment variables

Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned elsewhere, I don't think we want to load the config file like this. Why did you need to change it?

Apart from that, I don't know much about Docker, so can't really review that. If you're confident that these changes to the docker setup are an improvement and won't break, I'm happy to merge them.

@sandhose
Copy link
Member Author

As mentioned elsewhere, I don't think we want to load the config file like this. Why did you need to change it?

This change is here to be able to change the config without rebuilding the app. Currently, the config lives in the bundled JS, which makes it impossible to change without rebuilding the app.

This allows to have on the filesystem a plain old config.js file that can be easily changed at runtime, and this is what is done in the Docker image: there is a config.js.tmpl which is templated by this script on the start of the container, which fills it with environement variable.

The end result, is that one can run the image with a custom default homeserver without having to build a custom one:

docker --rm -p 8080:8080 -e DEFAULT_HOMESERVER=https://my-hs.com ghcr.io/vector-im/hydrogen-web

If that is not desirable, I can move the config handling changes in another PR and merge the docker improvements first

@bwindels
Copy link
Contributor

If that is not desirable, I can move the config handling changes in another PR and merge the docker improvements first

Thanks, that would be good.

@sandhose sandhose changed the title Runtime configuration of the Docker image and other improvements Docker image improvements Feb 16, 2022
@bwindels bwindels self-requested a review March 21, 2022 13:48
@sandhose sandhose force-pushed the sandhose/runtime-config branch 2 times, most recently from f4d0ba3 to 66851ba Compare April 25, 2022 08:53
@hughns
Copy link
Member

hughns commented Jan 11, 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.

3 participants