-
Notifications
You must be signed in to change notification settings - Fork 49
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: rework Container image, Compose manifest and CI #64
base: main
Are you sure you want to change the base?
Conversation
This PR presents the result of 16 hrs of work to reshape it into a more "production-ready" form. This is achieved by dropping the support for local source builds.
Additionally, following the 12factor.net approach of designing web applications, it is useful to consider the parts of the application self-contained and independent from external systems. Why the reliance on a A convention designed for running jobs in distributed systems is to use a worker queue and have it be triggered with timers. Fortunately Celery is already used for dispatching asynchronous tasks. Its Celery Beat feature can also be used to regularly schedule jobs, removing the need for a separate
I'm especially invested in this, since the cron implementation took a quarter of the time from the overall procedure and is prone to fail in a container. What could possibly go wrong?
To complete the move towards "cloud-native", distributed application design, it is also advisable to implement the database URL pattern, which had been introduced by Heroku and is of good ease, convenience and use to DevOps people.
Further on, now that the application setup is more decoupled, images are more lightweight and deterministic, due to switching to pinned release versions of Pretalx, it becomes possible to imagine to further
I'm thinking of the way how these two repositories react to their upstream, adapted from Node to Python: In case you agree with the follow ups as outlined above, I'd step ahead and open respective issues. |
More inspiration could be taken from allmende/docker-fiduswriter. |
context/default/entrypoint.sh
Outdated
export PRETALX_DATA_DIR="${PRETALX_DATA_DIR:-/data}" | ||
|
||
PRETALX_FILESYSTEM_LOGS="${PRETALX_FILESYSTEM_LOGS:-/data/logs}" | ||
PRETALX_FILESYSTEM_MEDIA="${PRETALX_FILESYSTEM_MEDIA:-/data/media}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these defaults match what's set in the .env.example
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was adapted from the original image and I guess for now they do not show the container defaults, but rather the application defaults.
I'm open to hearing further opinions on this.
This looks generally good to me. While in a Kubernetes environment I'm used to having CronTab jobs running periodic stuff in containers, I don't see too much harm in having crontab running within the application container. That is, unless someone wants to create replicas of the application container and run the worker things isolated on their own, but I doubt that this would be possible with Pretalx, so this is likely a non issue. For static file serving I think that nginx as a separate container (or any other simple file server like 👍 Overall, I'm happy with these changes. Could we maybe consider having a GitHub workflow which automates new releases to DockerHub (or GitHub Packages)? This would avoid having bug-fixes merged into |
partially reverts a00895e
…ISO date versions
The last two days have brought new image variants, local build scripts, a build pipeline and the proposed migration hints to the README. For now, the pipeline can be triggered manually and by releases. The other push and PR triggers have been removed, until this has been settled a little. The image variants reintroduce the The extended and cron variants from the "stock" image have also been applied to the standalone one for sake of completeness. This means people can potentially remove the external dependency to cron from their existing containerised application setup. There are also new example contexts to build the application from source, of which one mimics the current state of affairs, namely The one interesting for me to be able to run from main, including the fix for pretalx/pretalx#1773, is The README tries to be complete and concise at the same time. Due to the multiple ways to approach the repository, its images, the Compose manifests and also the CI pipeline(s) we cannot be exhaustive here. Maybe the setup is complex enough to consider adding separate documentation to This is now ready for merge with approaching the 50th hour. I would leave the click to another maintainer, as I wouldn't want to misuse the gained write-permission to the repository with my own and first contribution. What's left for future cycles is roughly:
Also new usages for the repository appear within reach:
Ultimately, I will also raise conversations upstream about implementing Celery Beat, which would allow to get rid of the cron dependency entirely, and about a containerised development environment and why one might like that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, what a huge PR! First of: sorry that i'm not active here. As the story often goes, i don't use pretalx anymore, so i'm not keeping up here and (as the state of this repo shows) i only maintain this on paper..
Nevertheless i took the time to look at your PR. All in all, i like a lot of your changes. Especially breaking up the container images in multiple ones, going more in the direction of one container, one process, as it should be. I left a few comments, but please, just take them as suggestions, nothing important.
I have two main suggestions:
Simplifying
As nice as having all the different variants is, i fear this might be way to much to maintain in the future. If you're up for the job: awesome! Otherwise i would suggest deleting some of them. E.g. do we need the ability to build from source? I know it would be nice to be able to run pretalx:main
, but since @rixx wants to release more often, anyways, i don't know if its worth the extra trouble.
Readme
I think most people just want a quick, opinionated "how to run this in the simplest way on production" from the readme. Right now this seems kind of buried. Would you be up for writing a short "how to use in production" and put it up top?
I would also suggest moving everything related to building the containers and developing this repo out of the readme, e.g. in a docs
folder.
As is said, please just take these points as suggestions. As these changes go in the right direction overall, i'm not objected to merging as is.
FROM ${BASE_IMAGE}:${BASE_TAG} | ||
|
||
# Install system dependencies | ||
RUN apt update && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the modern way of doing this would be to use multi stage builds. then you can avoid the "install and clean" dance in one giant "RUN" command and copy only the bits over to the next stage that you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not proficient enough in picking the artifacts of system packages to the next stage.
Are you suggesting to use multi-stage builds to separate build dependencies and runtime dependencies in the image? How do we differentiate them, e.g. are the -dev
apt packages needed to run pretalx or only to build its dependencies during installation?
I didn't dig too deep into the actual implementation of the application yet and was rather suggesting these changes to improve upon in subsequent iterations.
libmemcached-dev \ | ||
locales \ | ||
nodejs \ | ||
npm \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do we need node and npm? in the source variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's part of the rebuild
, rebuild --npm-install
and regenerate_css
management commands, but please correct me, if I'm wrong.
@@ -0,0 +1,145 @@ | |||
# Legacy docker-compose support | |||
|
|||
This example demonstrates a setup that is compatible with the legacy version of Docker Compose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make it more obvious that you mean docker-compose
with the dash?
This example demonstrates a setup that is compatible with the legacy version of Docker Compose. | |
This example demonstrates a setup that is compatible with the legacy version of `docker-compose` v1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, like in the headline of that section. Eventually it will be good to be ultra-specific and talk about:
the old Docker Compose version 1.x,
docker-compose
docker compose -f compose.yml -f compose/local.yml -f compose/traefik.yml config | ||
``` | ||
|
||
### Live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Live | |
### Production |
I feel the word "production" is more commonly used, than live
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! You catched me here being weary of that term. I'm choosing live, because for me running a web application is more like publishing a live radio or television show, which needs constant attention, rather than delivering a fixed asset, such as a product. Since we're at it, I'm also weary of concealing the fact of human labour involved by using corporate language, which often dehumanises processes.
If others insist on this, I'm willing to change the phrasing to the more commonplace industry convention.
|
||
This repository follows the Pretalx installation instructions as closely as possible. | ||
|
||
- [Installation — pretalx documentation](https://docs.pretalx.org/administrator/installation/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems kind of misleading, since the official docs don't have anything to do with docker at all..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
It should read
This repository imitates the pretalx installation instructions as closely as possible in a containerised environment.
Thank you for taking the time to look into this. The two points you mention reflect very well the course this has taken:
I agree that the primary use case of this repository should be to show how to run pretalx in a real-world environment, in opposition to its earlier use for building an opinionated standalone image from a tightly coupled git submodule source. The effort of trying to answer to all possible use cases makes the individual choices harder to stand out. If there would be support by upstream, we would maybe do good in considering to move the Container image + CI manifests for base/ and default/ into the main pretalx repository, in order to build local development containers and the release artifacts where development and releases happen. This repository could then focus on the Compose setup and the image variants. I agree to decouple the image development instructions into separate docs/ and to render the readme much more accessible for casual visitors. Allow me to respond to the suggestions in the next two weeks. We are not in a rush here and can take the time to introduce these breaking changes in a well-documented and -tested fashion. |
@almereyda Just a ping: are you still planning to work on this? |
Yes, I got sick back then. I'm currently refactoring and documenting a few Django Compose repositories, from which I plan to backport some of their patterns here, including more simple documentation, as suggested. I'd say at least another two months, given life circumstances. |
Thanks a lot for the PR. IMO it's over complicated for docker compose repo. It took me quite a while to understand what goes where. I made it work: extended version with configured traefik TriplEight#3 |
Great suggestions and ideas in that branch, esp. around the auxiliary services cron, Traefik. I'm also considering using more of Compose's newer I especially like how you shoretend the As a style note to TriplEight#3, I would like to suggest to refrain from using the Also defining The FQDN for I'll be able to return work on this from mid November on, with aiming for a merge in the first two weeks of December. |
This change request presents a major opinionated rewrite of this repository.
.env
to configure Pretalx as a container-native 12 factor appstatic/
assets andmedia/
plugins
workflow to build apretalx-extended
imagepretalx/pretalx
in CI and Composepretalx.cfg
example to be compatible with the changed setuplinux/arm64
build targetcloses #21
closes #56
closes #59 ?
closes #62