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
quality-of-life: improve the clean
recipe (don't require sudo
anymore)
#3020
Conversation
This bugged me for a long time: we need `sudo` when running `make clean`, which is undesirable as you can image. I implemented a proper solution: We run a small container (Alpine) and mount the files we need to remove into the container. This has the advantage that users that run Docker with an unprivilged user now don't have to type their sudo-password anymore and users that run Docker by default with password will need to type in the passward, but they need to do this anyways because of the command before. --- To explain what I did: I introduced an array where I save files in. The `.gitignore` is still read, but the files read in the loop are now saved in the array. I adjust the path, prefixing `/mnt` and removing the `test` prefix from the file path before so the path is correct inside the container. I can then just mount the whole test directory at `/mnt` inside the container and run an `rm -rf` inside the container on the `FILES` array. Done. Enjoy :D
73bab98
to
d46abb4
Compare
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.
Not entirely fond of the approach (albeit clever! 😛 )
It's definitely been something that has annoyed me too. My plan was to wait until tests are all migrated to the common container method, and then just leverage docker cp
(or a Dockerfile
that extends mailserver-testing:ci
with COPY
), or an anonymous volume..?
Although my approach is still a bit more work to sort out. We're reasonably close to being able to mount as :ro
in some cases too, but need to handle a few startup scripts that currently perform some sanitation pre-processing IIRC.
Sounds good! Until we have your solution in-place, I think this PR is well worth it :) |
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
LGTM. Two thoughts:
|
Yeah, I can do that :)
I will remove it. Just FYI: #3016 is important for me, so if you think about which PR to review next, it'd be nice of you picked this one ❤️ |
PS: you only need
Ack 👍 |
I removed the long options from the command and replaced them with their short versions. This allowed me to then nicely add line breaks to increase readability of the command. I couldn't withstand reworking the above command too; it is now using a proper array as well (with proper splitting before). Moreover, I added the `dms-test_` prefix to the linting tests so we do not need to grep for them anymore, which shortens the commands. If you have 100 characters in width for the `Makefile`, the clean target should be properly displayed now.
Makefile
Outdated
-@ while read -r LINE; do CONTAINERS+=("$${LINE}"); done < <(docker ps -qaf name='^(dms-test|mail)_.*') ; \ | ||
for CONTAINER in "$${CONTAINERS[@]}"; do docker rm -f "$${CONTAINER}"; done | ||
-@ while read -r LINE; do [[ $${LINE} =~ test/.+ ]] && FILES+=("/mnt$${LINE#test}"); done < .gitignore ; \ | ||
docker run --rm -v "$(REPOSITORY_ROOT)/test/:/mnt" docker.io/alpine:3.17.1 ash -c "rm -rf $${FILES[@]}" |
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.
-@ while read -r LINE; do CONTAINERS+=("$${LINE}"); done < <(docker ps -qaf name='^(dms-test|mail)_.*') ; \ | |
for CONTAINER in "$${CONTAINERS[@]}"; do docker rm -f "$${CONTAINER}"; done | |
-@ while read -r LINE; do [[ $${LINE} =~ test/.+ ]] && FILES+=("/mnt$${LINE#test}"); done < .gitignore ; \ | |
docker run --rm -v "$(REPOSITORY_ROOT)/test/:/mnt" docker.io/alpine:3.17.1 ash -c "rm -rf $${FILES[@]}" | |
-@ while read -r LINE; do \ | |
CONTAINERS+=("$${LINE}"); \ | |
done < <(docker ps -qaf name='^(dms-test|mail)_.*') ; \ | |
for CONTAINER in "$${CONTAINERS[@]}"; do \ | |
docker rm -f "$${CONTAINER}"; \ | |
done | |
-@ while read -r LINE; do \ | |
[[ $${LINE} =~ test/.+ ]] && \ | |
FILES+=("/mnt$${LINE#test}"); \ | |
done < .gitignore ; \ | |
docker run --rm -v "$(REPOSITORY_ROOT)/test/:/mnt" docker.io/alpine:3.17.1 ash -c "rm -rf $${FILES[@]}" |
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.
At this point I'm wondering if it should just be a small script that gets called outside of the Makefile 😅
I don't know about pinning the alpine version, the minor changes every few months, we're not likely to keep that maintained, especially with the point releases. Perhaps just omit the tag for implicit :latest
? Shouldn't cause any issues for this usage.
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 point about the version pinning. I don't think the behaviour of rm
will significantly change with newer versions 😆 Sticking to :latest
should be fine.
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.
Definitely keep it in the Makefile, no outside script. And the version pinning is just so Docker does not pull a new version ever time the image is updated.
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.
Docker does not pull a new version ever time the image is updated.
It shouldn't for :latest
, that's often a drawback due to newer Docker users assuming it always gets the :latest
rather than the latest digest at the time it was originally pulled, then uses the local :latest
tagged image from then on (unless explicitly pulling again).
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 overly fussed either way due to usage, but the version pinning here isn't serving much purpose.
I know in our tests we have a very outdated version pinning for OpenLDAP container that I'll sort out when I get around to working on the LDAP test.
Our CI may want to bump from Ubuntu 20.04 LTS to 22.04 LTS at some point, but no rush there as it shouldn't offer any advantage to us I think. Github keeps the pre-installed software we care about updated (eg Docker Engine).
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.
There you go: 8f006ba
Can we please get over with this?
I loose all the nice approvals I worked for because of this change. That's sad.
8f006ba
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.
That commit message 😂 I know the feeling! 😛
Description
This bugged me for a long time: we need
sudo
when runningmake clean
, which is undesirable as you can image. I implemented a proper solution:We run a small container (Alpine) and mount the files we need to remove into the container. This has the advantage that users that run Docker with an unprivilged user now don't have to type their sudo-password anymore and users that run Docker by default with password will need to type in the passward, but they need to do this anyways because of the command before.
To explain what I did: I introduced an array where I save files in. The
.gitignore
is still read, but the files read in the loop are now saved in the array. I adjust the path, prefixing/mnt
and removing thetest
prefix from the file path before so the path is correct inside the container. I can then just mount the whole test directory at/mnt
inside the container and run anrm -rf
inside the container on theFILES
array. Done. Enjoy :DType of change
Checklist:
docs/
)