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

feat(docker): add basic docker support #1433

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

desaintmartin
Copy link
Contributor

@desaintmartin desaintmartin commented May 29, 2023

Here is a first try of #1350 as a simpler alternative / first step than #1170

Here are some notes:

  • Scope is much smaller than Docker #1170: only having a production container image
  • It's a two-stages image, instead of the separated "composer" stage of Docker #1170. If you think this could easily be done, feel free to point me to the right direction, but I was not able to properly separate build from serve (due to always needing to build extensions...) edit: https://hub.docker.com/_/composer/ recommends doing what is done in this pr, so everything seems ok, and I now delete development packages.
  • cron in included within the container, which seems to be a very good compromise but may not be ideal in some rare situations
  • I'm assuming that SQLite is used and a data volume is mounted and config can be customized (mounted as volume). I haven't tried with external database.
  • It's not using fpm but ol' good apache. I'm assuming this is ok for now.

What do you think?

Potential next steps if this gets merged:

  • image building & hosting automation to GitHub Container Registry using Github Actions (I can do)
  • Kubernetes compatibility through Helm Chart with dedicated database (I can do)
  • dev-container
  • profit and glory

@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for selfoss canceled.

Name Link
🔨 Latest commit 2910891
🔍 Latest deploy log https://app.netlify.com/sites/selfoss/deploys/65441d7039c136000849f091

@desaintmartin desaintmartin changed the title feat(docker): add basic docker support. feat(docker): add basic docker support May 29, 2023
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Still the main obstacle is that I do not understand how users that want Docker expect to use the image so I have no way of evaluating whether the PR fits that.

Do people just run docker commands manually? Or use something like docker-compose? How do they handle updates? What are security considerations? How do people configure software? How do they manage data backups?

.dockerignore Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I do not really like these huge blacklists with patterns that are not relevant. Maybe just copy what we need. Or better copy the result on npm run dist, which also prunes the vendor/ directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reduced the blacklist. Usually we want to "copy everything relevant" (COPY . .) without having to bother for new / changed directories, although the opposite is also valid.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Which is why I also suggested using npm run dist command. It will build a production package, including filtering the PHP directories. It would allow us to clean the Dockerfile even further and we would not need the .dockerignore.

Of course, it would mean building the image from scratch every time but perhaps that is fine for production/non-dev image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, when you COPY something into a container image, it can't get removed, so if we copy local data, for example, for local builds, it will go into the final image size even if scripts inside container build filter them. This is a good practice in the docker community to filter out everything that could exist in a local development environment to avoid side effects.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, but having more separate, atomic layers with fine-grained COPY is far better than a few non-atomic layers. Each layer actually add a few bytes (technically speaking, a layer is a tar file, and all layers of an image are mounted on top of the previous ones)

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@desaintmartin
Copy link
Contributor Author

A few months later, let me try to answer.

Still the main obstacle is that I do not understand how users that want Docker expect to use the image so I have no way of evaluating whether the PR fits that.

I understand this issue. Maybe I can just say that I'm a Kubernetes contributor (see my profile) and my production clusters manage ~15000 running containers right now, if that helps. Of course I don't want to pretend to be someone here! ;)

Do people just run docker commands manually? Or use something like docker-compose? How do they handle updates? What are security considerations? How do people configure software? How do they manage data backups?

It really depend, some use docker-compose, some use raw docker/containerd commands, some use Kubernetes either directly or through Helm, some use home NAS like synology to manage containers for them, but all of them use container image definition.

I'm going to update a bit the PR to be more up-to-date and take into account your suggestions!

@desaintmartin
Copy link
Contributor Author

desaintmartin commented Nov 2, 2023

Notes:

  • I haven't included a docker healthcheck. I am not sure it is always wanted. On kubernetes, healthcheck are a completely different system anyway
  • I disabled cron, so we still have to do it like today. Much more atomic / simpler like a container is supposed to be, much less surprise / bugs.
  • php:8.2-apache image, built on top of debian:12-slim, is 710MB (!!), final image is 770MB so 60MB more than upstream image, which is not bad, relatively speaking (absolutely speaking, this is huge, but this seems to be how it works today).
  • I now drop root, according to production good practices
  • We need minimalistic documentation, just tell me where you want it to be.
  • To build it: docker build --tag selfoss:docker-beta .
  • To run it: docker run --rm --name my-selfoss -p 80:80 -v data:/var/www/html/data selfoss:docker-beta

@desaintmartin desaintmartin force-pushed the master branch 2 times, most recently from c78629b to 2910891 Compare November 2, 2023 22:06
@desaintmartin desaintmartin requested a review from jtojnar November 2, 2023 22:07
@desaintmartin
Copy link
Contributor Author

Hello @jtojnar, would you have any time to review this PR?

@desaintmartin
Copy link
Contributor Author

I would really love to use selfoss from a container so that I can automate / simplify my self-hosting, and at the same time I would hate to have a long-running fork of selfoss. What is your opinion on having an official image @jtojnar ?

@jtojnar
Copy link
Member

jtojnar commented Jul 21, 2024

Sorry about not replying sooner, I have opened the tab multiple times but then got distracted each time.

I think this is valuable but at the same time I still worry about my ability to keep it working, as I do not use Docker. Especially when I have had less time for my open-source projects since I started a full-time job.

After giving it some more thought, I think I would be fine with merging this as long as the level of support is clearly communicated. For example, we could have it called selfoss-community/selfoss in Docker Hub. We should also mention in the description who is currently maintaining it (even explicitly inviting people to try to increase the bus factor). Edit: I tried adding something similar to selfoss README: #1495

I could also grant you and @radek-sprta commit access to this repo so you could maintain this without depending on me for merging.

Alternately, we could create a separate selfoss-docker repo. This would allow us to invite even more people as committers without having to trust them with the main repo. But that would probably require annoying amounts CI code for synchronization.

Copy link

netlify bot commented Jul 21, 2024

Deploy Preview for selfoss canceled.

Name Link
🔨 Latest commit 0bf1e20
🔍 Latest deploy log https://app.netlify.com/sites/selfoss/deploys/66c44aa5ada4250008666229

@desaintmartin
Copy link
Contributor Author

Either ways are fine for me, and of course I think, as there seem to be several people interested into maintaining a Docker image with several tries over the years, that we should find a way to have the most maintainable solution.

Pros of having a separate repo: more secure

Cons of having a separate repo:

  • Usual way to build a container is from sources, so it would actually... be a fork?
  • we can't use the github container registry in a transparent manner with UI from the main repository (See https://github.com/desaintmartin/selfoss for an example, for now with unpublished versions). But that may be against your goals of marking it as "unofficial".

I don't think too many people with write access (btw, not push, only merge!) are needed.

dependabot.yml Outdated Show resolved Hide resolved
package.json Outdated
Comment on lines 34 to 35
"install-dependencies:client": "npm install --production=false --prefix client/",
"install-dependencies-ci:client": "npm ci --production=false --prefix client/",
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to just use npm ci for install-dependencies:client – reading the docs, I do not see why one would want an unclean installation of dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, fixed!

.dockerignore Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Right. Which is why I also suggested using npm run dist command. It will build a production package, including filtering the PHP directories. It would allow us to clean the Dockerfile even further and we would not need the .dockerignore.

Of course, it would mean building the image from scratch every time but perhaps that is fine for production/non-dev image.

@jtojnar
Copy link
Member

jtojnar commented Jul 21, 2024

Usual way to build a container is from sources, so it would actually... be a fork?

Not necessarily, see e.g. https://github.com/wallabag/docker. That one currently only builds tagged versions but nighlties could be handled by GitHub actions in the main repo triggering builds in the docker one.

  • we can't use the github container registry in a transparent manner with UI from the main repository

Yeah, I think that would be a good thing – at least until selfoss development team contains enough people who could maintain it so that the risk of it becoming unmaintained is reduced.

Cons of having a separate repo:

Additionally, separating would make synchronous changes more work: contributor would need to make PRs in two different places. And the repos could get out of sync breaking compatibility.

That could be minimized by using a single entry point like nix run dist and keeping the structure unchanged. But I think I still lean towards using the main repository for this reason.

I don't think too many people with write access (btw, not push, only merge!) are needed.

Right, we could require going through PRs and then adding review requirement in branch protections.

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