Build container and release it to github, merge files to pyproject.toml and handle basic setup/migrations automatically in container entrypoint-script#3636
Conversation
9afc7eb to
f6ce2c5
Compare
bebcb12 to
fbad6a6
Compare
|
rebased the PR on top of current main |
|
If there is something that you are not sure/just curious, please do ask and I'll try to clarify what it does. |
|
I'll add commits to this PR to have development flow working with local source-files. |
f9e0fc1 to
e816a01
Compare
4cd8fd9 to
9908401
Compare
|
I moved dev-tools to dev-compose file and point docker-compose bookwyrm images to ghcr.io, so release-wise the docker-compose modifications would be just adding some production related db-changes and change bookwyrm:main to bookwyrm:release-number. |
|
@ilkka-ollakka Can I check that consideration has been given to any potential impact of these changes on dockerless installs. For example, I noticed that requirements.txt has been removed, But we need to run |
Ah yes, good point. For now it should be something like |
9908401 to
e403bf8
Compare
|
Rebased on top of current main and update pyproject.toml with new Django-version from updated requirements.txt. |
e7f3c72 to
2afbcfa
Compare
2afbcfa to
f8f2277
Compare
|
rebased on top of current main with updated requirements.txt changes. |
f8f2277 to
98e27b3
Compare
|
rebased on top of main to update requirements changes |
|
I'll split separate PR's out from this, separatable changes so those are easier to review/merge separately after current release is in order. |
|
@ilkka-ollakka I'm trying to understand this. Can you explain how |
Ah yes, in Dockerfile we define 'ENTRYPOINT' pointing to that script in line 42. So all commands get run as parameter to entrypoint script. Documentation on the difference of ENTRYPOINT and CMD can be found in https://docs.docker.com/reference/dockerfile/#understand-how-cmd-and-entrypoint-interact so docker/podman itself handles the calling I think it is good idea use health checks to make entrypoint simpler. Or did you have some idead to use healtchecks instead entrypoint script? That would be also nice approach, and if you have something started with that flow, I would be happy to take a look and adjust the PR? |
otherwise it would shadow installed node_modules location
…ations Instead of checking file-presence, check that file contains build-time define hash of migration files This allows newer builds/migrations to be waited until those are checked properly
docker-compose.dev.yml mounts most of the files on top of container, so you can develop entrypoint.sh as well as bookwyrm code/locales etc with dev command.
we only expose that port in web role, not in celery etc containers
…ypoint Celery and flower containers wait until web container is healthy, so it has handled migrations and database init This allows to simplify entrypoint-script so it doesn't need to poll in other containers if migrations are done
c806b70 to
73e3d43
Compare
|
I modified the docker-compose to have depends_on to web-container being healthy on bookwyrm containers and defined healthcheck to web-container to check that django is up. Also cleaned out entrypoint to run migrations and not to do polling in other containers. |
|
also rebased on top of curren main |
|
Hi all, very happy to see this as someone looking to deploy this on a swarm. Is there any additional testing or development that I could help with on this? I see #3759 is approved and passing build; I'm happy to jump in wherever is useful. I think a canonical docker image is a fantastic addition to the project. |
Thats nice to hear! I've been splitting this PR to more bite-size PRs and I think only things left currently are:
I most likely try to split those remaining things during this or next week to new PRs, and if you can take a look and test/comment, it would be great. Ofcourse if you have some questions/comments already now in things, I would love to hear those. |
Sure thing! My docker compose experience is mildly limited; I have ~10 stacks running across either my NAS or my swarm and while my employer uses containers heavily I'm a step removed from the infra teams. I put myself firmly in the "know enough to be dangerous" category 😅 so just be aware that my questions are just as likely to come from a lack of (beginner) knowledge as being a structural concern, and I'll try to flag them accordingly. I'll add what questions I have against the PR and I'll see if I can get an instance running on my swarm with the compose file in this PR / the |
| - main | ||
| web: | ||
| image: ghcr.io/bookwyrm/bookwyrm:main | ||
| build: . |
There was a problem hiding this comment.
curiosity: for the other services in the compose file you removed the build directive in favor of the image: ghcr.io/bookwyrm/bookwyrm:main; why keep the build: . here?
There was a problem hiding this comment.
allowing development and local build. Also as all have same image it only builds it once, atleast podman compose builds currently all images separately.
| "ALLOWED_HOSTS = your.domain.here", | ||
| "BOOKWYRM_DATABASE_BACKEND = postgres", | ||
| "MEDIA_ROOT = images/", | ||
| "CELERY_BROKER_URL = memory://", |
There was a problem hiding this comment.
curiosity: is this meant to be a default? in the docker-compose case, would memory:// ever work when the broker is a separate container?
There was a problem hiding this comment.
this is oblyfor pytest run variables, so not default otherwise. It is in tool pytest.ini_options block
| chown -c -R bookwyrm /app/exports | ||
| chown -c -R bookwyrm /app/images | ||
| chown -c -R bookwyrm /app/static/css/themes |
There was a problem hiding this comment.
curiosity: what happens when the bookwyrm user doesn't exist on the host system? Docker user/group mapping is not something I understand well, and I feel like I often run into filesystem permissions errors.
There was a problem hiding this comment.
it doesnt need to exist in host system. bookwyrm account is created inside container in build phase. Entrypoint is also runned inside the container in container start.
| condition: service_healthy | ||
| redis_broker: | ||
| condition: service_started | ||
| restart: on-failure |
There was a problem hiding this comment.
this conflicts with the restart: always directive on line 105, above
| restart: always | ||
| depends_on: | ||
| - celery_worker | ||
| restart: on-failure |
There was a problem hiding this comment.
another pair of conflicting restart directives
| restart: always | ||
| depends_on: | ||
| - db | ||
| - redis_broker | ||
| db: | ||
| condition: service_healthy | ||
| redis_broker: | ||
| condition: service_started | ||
| web: | ||
| condition: service_healthy | ||
| restart: on-failure |
There was a problem hiding this comment.
good catches, I'll try to remember to sort those out when I rebase/split remaining things out.
|
@ilkka-ollakka is the idea that we won't actually need this PR once we merge #3803 #3809 plus maybe some small cleanup after that? |
yes, idea is to split things to smaller PRs and then this can be closed. |
|
@hughrun just making sure, there are still few PRs that are needed beside those that are already open. Mainly the publish change is one and the non-root/dev-tools thing is optional but nice to have. |
Description
This PR contains changes to build bookwyrm container and release it to be used without closing the source repository. Currently it builds arm64 and amd64 platforms.
Container builds are done when tag is pushed to use tag as version, for example
bookwyrm:0.8.0. Main repo push build container with namebookwyrm:main.It also combines requirements.txt, pylint/mypy/coverage configs to pyproject.toml config.
Container entrypoint script runs migrations,initdb and permissions checks before changing to
bookwyrm-user to start provided command. So Celery/bookwyrm services are running non-root user. I used the #2467 as initial entrypoint script and modified it littlebit.bw-devcommand now has commanddevto start local code in container instead of build container. It should allow local development easily.dev-config also setsDEBUG=1environment variable, making heavy assumption that you want to run it with debug-mode.I modified Dockerfile and entrypoint for this, to take build-timestamp as marker what to look for in file when waiting migration checks, still might not be fully working, but should handle new builds/migrations to be waited correctly.
Changes include using dev-tools container changes, build it on top of bookwyrm container, install all dev-dependencies there instead of main container and run all pytest/mypy etc using that container.
What type of Pull Request is this?
Does this PR change settings or dependencies, or break something?
Details of breaking or configuration changes (if any of above checked)
We don't anymore mount local directory to container, but build container. We also run migrations and database-initdb autotically on each start.
We run pytest/mypy/etc in derived container that mounts source-code in.
Dev flow now uses
bw-dev devinstead ofbw-dev upto start local code.requirements.txt,.pylintrc,.coveragercandmypy.iniare all merged topyproject.tomlDocumentation
Tests
I think this needs some manual testing, as it is big change and it is probable that I have missed some flow/use-case that is broken.