fix: Don't needlessly invalidate cache layers#2197
fix: Don't needlessly invalidate cache layers#2197polarathene merged 2 commits intodocker-mailserver:masterfrom
Conversation
Recent `sedfile` addition moved all scripts section earlier into the Dockerfile so that `sedfile` could be used within the Dockerfile. However whenever a change is made to scripts which is most of the time for this project, building the Docker image for tests results in all layers after the scripts being invalidated, notably ClamAV, wasting storage of previous instances and increasing build time unnecessarily. This isn't as noticeable of an issue via the CI as we don't leverage any caching at present there, but for iterating on a local branch and testing, it can be quite the drawback. - `sedfile` is handled early in the Dockerfile still, while the scripts have been moved as far down as it made sense to. - `chmod` was split out into it's own RUN command as again it's unnecessary for the rest of it's prior RUN command group to be invalidated.
|
❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ |
|
@NorseGaud gripe for you too in your long-lived PR I take it? 😂 I've been using this uncommitted in my current PRs, much better experience 😛 For a CI improvement, I notice that building images is taking 15-20 minutes. I assume our tests are only running on the x86_64 image, so building the other images is probably wasteful in resources and time. We could instead delay building for other platforms until tests pass / finish? This PR for example takes approx 50 minutes for the full test suite + CI builds to run. Tests time can be reduced via multiple job runners in parallel (might be complicated a bit by distribution of the testing image, I think we can use a similar technique that the doc previews use), I'd just need to look into what tests are expensive in time to offload to other jobs (eg the cipherlists). |
Yeah 😄 Parallel testing would be a huge win. The tests could be grouped into phases (things that don't impact other things in one phase, etc) and then kick them off in a wrapping script. |
|
I'd like to approve this PR but I'm seeing failures. Do we know why the actions are failing? Are they waiting on something else. I think merging without passing is a bad idea, even if the failing tests is a WIP. We might miss something critical and make it hard to fix later in the master branch. |
|
Should be the setup.sh issue again i think. +1 for your last thought. What speaks against merging the setup.sh changes again as we found out that it was an user issue? |
I think that's a great idea. |
|
Yes it's just the two We were delaying merging the 10.2 release until #2196 was ready (for the |
|
I'll merge this after #2196 is merged. |
|
@NorseGaud @wernerfred you're right, I will take care of this. But I can tend to it today in the evening at the earliest. |
@wernerfred @NorseGaud I changed #2189 to be the PR that merges the new |
georglauterbach
added
area/ci
kind/improvement
georglauterbach
removed
the
pr/waiting for other PR to get merged
5 participants
Description
Recent
sedfileaddition moved all scripts section earlier into the Dockerfile so thatsedfilecould be used within the Dockerfile.However whenever a change is made to scripts which is most of the time for this project, building the Docker image for tests results in all layers after the scripts being invalidated, notably ClamAV, wasting storage of previous instances and increasing build time unnecessarily.
This isn't as noticeable of an issue via the CI as we don't leverage any caching at present there, but for iterating on a local branch and testing, it can be quite the drawback.
sedfileis handled early in the Dockerfile still, while the scripts have been moved as far down as it made sense to.chmodwas split out into it's own RUN command as again it's unnecessary for the rest of it's prior RUN command group to be invalidated.Type of change
Checklist: