-
Notifications
You must be signed in to change notification settings - Fork 10
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
Make Docker setup less memory-intensive & build deps at container start #736
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Splines
commented
Jan 21, 2025
…731) * Init first attempt to download latest database dump * Overhaul just command to download the latest database dump * Repurpose the download command to just download the dump * Use existing /db/backups/ folder for storage * Init dummy reseed from file command * Check if preseed is url or file * Move download db command to new `prod.justfile` * Fix punctuation * Init basic up-reseed-from-dump command (not tested yet) * Tear down db, unzip dump, copy over and try to restore (restoring is not working yet at this point) * Get local restoring of the database to work * Tear down database before up-reseed & fix pgadmin config * Restart containers afterwards * Make handling of empty database more robust - Create mampf db if necessary - Start db and wait for postgres automatically * Move wait for postgres to db-tear-down method * Don't wait for postgres at start of db-tear-down * Clarify that up command assumes a valid database
fosterfarrell9
approved these changes
Feb 3, 2025
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.
LGTM. For later reference: Discussions took place on basecamp.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our local dev/test docker setup had some inconsistencies and annoyances that I fix in this PR. It touches on many different config settings that are (in my opinion) sensible to modify in one PR.
Changes to local setup
bundle install
will return after less than 1s. And we now store those gems in a new volume:bundle_cache
of around 350MB. Should you still have to rebuild the MaMpf container (not necessary anymore when you switch branches with different dependencies on them), you can do so more quickly since dependencies don't have to be refetched and installed again from scratch (due to thebundle_cache
volume which acts as a local cache).yarn
whenever the container starts. Therefore, you should always have those dependencies available from the PR you've checked out, which eases jumping around between branches.13
andlatest
). This is now unified and saves some more storage + network time.pull_policy
tonever
.initialize.sh
andentrypoint.sh
scripts fail directly (set -e
) if any error occurs. Those scripts are also renamed and improved to make more clear what they actually do.-Furthermore, we get rid of the workaround for creating a new MaMpf database at the beginning (the workaround was necessary since we renamed the default database user from
mampf
tolocalroot
). Instead, we use a special docker file that is automatically executed at the start of postgres, see here. Note that you will still encounter the errorPG::UndefinedTable: ERROR: relation "media" does not exist
in a completely empty database, but this issue is another one that won't be fixed in this PR.The previously used recipe
create-empty-mampf-db-if-not-exists
in thedocker.justfile
is just left there as reference and might be used manually should something fail. But it is expected, that postgres now reads our custom startup file to just execute aCREATE DATABASE mampf;
.Important
You should completely wipe your Docker setup once. This will also give you some storage back ;) To do so, run the following commands (at your own risk):
If you run into the weird issue error: RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly, just tell git to use HTTP/1.1 for the mampf repo via
git config http.version HTTP/1.1
. See also this comment.Changes to CI/CD
I've reworked the Dockerfiles and docker-compose files to make more transparent what is actually going on and especially what differences between the CI/CD runs and the local setup are. This is achieved by creating new files:
docker-compose.cicd.yml
anddocker-compose.cicd.build.yml
. (Note that testing also uses thedocker/development/Dockerfile
. There is nodocker/test/Dockerfile
anymore.)I've reworked our
tests.yml
GitHub workflow:Build MaMpf
job. It will use a buildcache to speed up the build. We now also usemode=max
to be able to cache multi-stage builds (we have twoFROM
statements in ourdocker/development/Dockerfile
, that's why it is "multi-stage").buildcache
andìmage
. See also Cache storage backends. We use theregistry
type: "embeds the build cache into a separate image, and pushes to a dedicated location separate from the main output.". I.e. the actual MaMpf image with the precompiled assets is calledmampftest:image
. This is pulled in the test jobs viadocker pull
. The buildcache (mampftest:buildcache
) is not an actual Docker image; instead its a special cache format that will be used by the build job. Trying to docker pull from it will fail asdocker pull
expects an actual image.This PDF showcases some pipeline run times before and after this PR. This is not a thorough research. You will notice that speed benefits are marginal. However the separation is good for a better conceptualization of what is going on and eases modifying caching behavior in the future.
Reviewers
The Docker documentation is actually really good and it describes very clear what config options do. I've included some links in the code. Nevertheless, please ask for any clarification if I made a modification is not 100% clear. I had to read a lot, so maybe I can answer the question.
Pleaes also report any warning you get during execution of any just command. There should be NO warnings after this PR. Please report those warnings with a PR review comment at the respective file, such that we can resolve those comments later on. (For the only warning that you could get, I explicitly print "Ignore the error" beforehand).
compile
is set to true for local dev and testing, that is, we have some lazy-loading going on, i.e. webpacker will only compile the assets if you really need them. This is why on the first visit of a new page, it might take a few seconds longer to load.just test cypress
. Note that cypress is the biggest image of all with a size of 3GB.just docker rspec
for that to see explicitly what is going on during container start, e.g. bundler will install all gems once such that they are stored in the testbundle_cache
volume. When you then rerun, you will notice how the cached gems will be used. After that, also make sure you can run the rspec tests in VSCode via the test runner.development_bundle_cache
and atest_bundle_cache
, both with the same content of ~340MB size. This is just a space, not a speed problem. Let me know if you have an idea how to solve this. Otherwise, this is still better than having to rebuild everything from scratch every time.)As a sidenote: working with the webpacker config here was such a pain as it was not at all clear which of the many config files it is actually executing and which env variables it respects. Thus a LOT of workflow runs where needed to test different settings (see the number of commits on this PR 😅). I hope I don't have to touch this setup again before we migrate away from it to Vite ;)
TODO:
localroot
after Init commands to download latest database dump & reseed db from file #731 is merged