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

Proposal: Dockerfile rewrite #256

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Proposal: Dockerfile rewrite #256

wants to merge 1 commit into from

Conversation

Kdecherf
Copy link
Member

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.

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
@gotmax23
Copy link

gotmax23 commented Mar 31, 2021

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:

  1. 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.
  2. 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.
  3. 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 and nginx.conf instead?
  4. Could we also implement PGID and PUID selection (see Run as specific user #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).
  5. 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?
  6. 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 run php bin/console doctrine:migrations:migrate --no-interaction and php bin/console cache:clear, if necessary.
  7. 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!

@HoroTW
Copy link

HoroTW commented Mar 31, 2021

I also would really like a new docker image too :)

  1. 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.

  2. 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 :)

@Kdecherf
Copy link
Member Author

Quick comment to not forget this interesting fact:

This rework enables us to decrease image size from ~580MB to ~397MB.

@ngosang
Copy link
Contributor

ngosang commented Aug 29, 2021

@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...

@luckydonald
Copy link

luckydonald commented Dec 16, 2021

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.

@AlphaJack
Copy link

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

@ngosang
Copy link
Contributor

ngosang commented Oct 25, 2022

@Kdecherf can I help you to finish this PR?
I have other open PRs meanwhile.

@Kdecherf
Copy link
Member Author

Kdecherf commented Nov 1, 2022

@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

@ngosang
Copy link
Contributor

ngosang commented Nov 1, 2022

@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.

@flan7
Copy link

flan7 commented Mar 12, 2023

Has any more progress been made on this issue? Its a shame I cannot use wallabag easily with truenas scale

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.

None yet

7 participants