docker
docker copied to clipboard
Proposal: Dockerfile rewrite
This PR is a proposal to rewrite the whole Dockerfile used for wallabag.
I made this version while moving my own instance to Docker, I wanted to simplify the management of the containers while removing some abstractions from within wallabag's image (like ansible, the process management and nginx).
Here are some important considerations compared to the former Dockerfile:
- It does not embed a webserver anymore, nginx (or any other webserver the user may want) will need to be handled outside of this container
- Ansible playbook tasks are either:
- replaced by envsubst for configuration files (parameter.yml and php.ini)
- removed and considered as tasks handled by other means (e.g. database provisioning is handled by the official postgres or mysql containers, if you use them)
- Base image moves from alpine to the official php-fpm image (still based on alpine)
Here are some considerations that may require discussion and/or improvement:
- I heavily rely on patches on my own instance, thus I added a apply-patches.sh script helper to let users easily create their own image by just adding files to the
patches/
folder - schema provisioning and database migrations are currently handled by hand, this part may need improvement
Some changes are "hardcoded" for my own need (like the use of redis in the session_handler php config), please consider them as a way to start a discussion about evolutions and improvements we can make.
Feel free to comment and ask any questions you may have about that.
A new, better-written Docker image for Wallabag would be great! Thank you for working on this.
I am not an expert in Docker, but I do have a couple questions/comments:
- For the env variables, is keeping the
SYMFONY__ENV__
prefix necessary? I think it would make the more sense for the variable names and default values to match those in the Wallabag documentation. This would also ease documentation for this image. - For the patches feature, would it make sense to run the
apply-patches.sh
script on container startup instead of in the Dockerfile so patches can be added as a volume mount instead of requiring building a new image? I think this might be a more elegant solution. - Again, excuse my lack of knowledge here, but what is the reason for removing Nginx from the image? Is the plan to create a
docker-compose
file with a seperate nginx container andnginx.conf
instead? - Could we also implement PGID and PUID selection (see #233)? For example, in the linuxserver/docker-nextcloud container image (using their linuxserver/docker-baseimage-alpine-nginx base image), this feature is available (and it also implements both Nginx and php-fpm in a single container).
- For the initial schema provisioning, couldn't we just run
php bin/console wallabag:install --env=prod -n
on container startup, like Ansible did in the old image? - For detecting version changes, would it make sense to log the current version in a file somewhere in
/config
; then, during container startup, we can detect whether Wallabag has been updated and runphp bin/console doctrine:migrations:migrate --no-interaction
andphp bin/console cache:clear
, if necessary. - For the redis/rabbitmq workers, don't those require a cron job setup to actually work? Or is the idea that the user just runs the import worker manually? I created an issue about this in the wallabag/doc repo, but didn't recieve a response.
I apologize for the barrage of questions. I could also potentially help with documentation for this image. Thanks again!
I also would really like a new docker image too :)
-
I guess it is a trade of.. In one case the patches would have to always be applied on container start, in the other case you can rebuild the image (in case of new patches) - this speeds up and simplifies the restart of the container.
-
I'm neither a docker expert myself but I will try to clarify point 3 anyways ^^ The reason why one would separate Nginx (and the database for the same reason) could be the separation of concerns. Docker is able to restart a container if something went wrong and if you separate the Nginx from Wallabag these can be restarted separately. In my opinion it is a best Practice to run one service per container (this view is backed up by the docker docs https://docs.docker.com/config/containers/multi-service_container/) Of course this creates the need for a docker-compose or something similar (the most basic thing would be to start the containers manually but I personally would really recommend a docker-compose file). Another Advantage is that I can choose to switch out the Nginx to something like caddy which I have already running on my server anyway - so I can save the Resources for running another Nginx ^^
. 5. / 6. I don't see any problems here :)
Quick comment to not forget this interesting fact:
This rework enables us to decrease image size from ~580MB to ~397MB.
@Kdecherf Thank you for the attempt. There are some parts of this PR that I don't like:
- IMHO embedded webserver is a must. If you want to use you own Nginx we can add some changes in the entypoint to choose the behavior.
- You are breaking backwards compatibility for several reasons. Removing the webserver. SYMFONY__ENV__SECRET is mandatory. Installation path changed...
- Patches are applied in "build time". It will be more useful for the user to mount
/patch
volume and apply the patches in the entrypoint (run time). - Dockerfile is hard to read for me
I opened #270 with the goal to be backward compatible and reduce the size. Previous 573.1 MB / This PR 225.8 MB We don't have to do all changes in the Docker image at once. After merging my PR you can add some interesting features from this PR like php.ini configuration, expose php-pfm, apply patches...
The only needed backwards compatibility is that the database upgrades run, and that ain't even working with the current one. #236. Modifying env variables is not a problem at all, at least in my case.
But I second that there should be a normal version also hosting the static files, so you can easily slap it behind any reverse proxy.
It would be great if the image didn't require to be run as root inside the container for added security.
Few things I noticed:
- MYSQL_ROOT_PASSWORD should not be readable by the server container. Providing MARIADB_USER, MARIADB_PASSWORD, MYSQL_DATABASE to the database container creates the database and user, allowing the server to connect and initialize the database.
- NginX should bind to a port higher than 1024, for instance 8080
- Ansible provisioning is handy, but requires elevated privileges
- It would be great to have an armv7 and aarch64 image in Docker Hub
In podman I can add --cap-add CAP_NET_BIND_SERVICE
to allow binding on port 80, but there is no workaround for Ansible
@Kdecherf can I help you to finish this PR? I have other open PRs meanwhile.
@ngosang I need to take some time to rebase my PR to reflect recent changes.
It seems that a discussion has been started to replace ansible. As I didn't address this point in my PR, we could consider that it will be handled separately, thus we could proceed with this PR to move forward.
I'm reconsidering my position regarding the embedded webserver, I'll work to keep it
@Kdecherf I already did all the work. Review is pending but I don't see any reason to be rejected. I tested it well and I'm keeping the same functionality. Maybe you can help in the review process #307 After that PR is merged I could review all open issues and PRs, I think most of them are already fixed anyway.
Has any more progress been made on this issue? Its a shame I cannot use wallabag easily with truenas scale