quality-of-life: improve the `clean` recipe (don't require `sudo` anymore) by georglauterbach · Pull Request #3020 · docker-mailserver/docker-mailserver

georglauterbach · 2023-01-22T11:28:37Z

Description

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

Type of change

Improvement (non-breaking change that does improve existing functionality)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)

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

polarathene

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.

georglauterbach · 2023-01-22T12:11:36Z

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..?

Sounds good! Until we have your solution in-place, I think this PR is well worth it :)

Co-authored-by: Brennan Kinney <[email protected]>

casperklein · 2023-01-23T22:35:05Z

LGTM. Two thoughts:

Add some line breaks to make it more readable?
declare -a FILES is not needed.

georglauterbach · 2023-01-23T22:51:56Z

Add some line breaks to make it more readable?

Yeah, I can do that :)

declare -a FILES is not needed.

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 ❤️

casperklein · 2023-01-23T22:58:54Z

PS: you only need declare, when you want to use an associative array (declare -A).

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 ❤️

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.

casperklein · 2023-01-24T16:18:23Z

+	-@ 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[@]}"


Suggested change

-@ 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[@]}"

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.

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.

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.

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).

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 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.

…an-recipe

polarathene

That commit message 😂 I know the feeling! 😛

georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 22, 2023

georglauterbach added this to the v12.0.0 milestone Jan 22, 2023

georglauterbach requested review from a team, casperklein and polarathene January 22, 2023 11:28

georglauterbach self-assigned this Jan 22, 2023

georglauterbach force-pushed the makefile/adjust-clean-recipe branch from 73bab98 to d46abb4 Compare January 22, 2023 11:31

polarathene previously approved these changes Jan 22, 2023

View reviewed changes

Comment thread Makefile Outdated

georglauterbach dismissed polarathene’s stale review via 1004a2d January 22, 2023 12:11

apply PR feedback (by @polarathene)

1004a2d

Co-authored-by: Brennan Kinney <[email protected]>

georglauterbach requested a review from polarathene January 22, 2023 12:12

polarathene previously approved these changes Jan 22, 2023

View reviewed changes

georglauterbach dismissed polarathene’s stale review via c925438 January 24, 2023 10:02

georglauterbach requested a review from polarathene January 24, 2023 10:04

casperklein previously approved these changes Jan 24, 2023

View reviewed changes

Merge branch 'master' into makefile/adjust-clean-recipe

bc19069

polarathene previously approved these changes Jan 24, 2023

View reviewed changes

georglauterbach added 2 commits January 25, 2023 01:07

resolve issues with philosophical dimensions

268c42b

I loose all the nice approvals I worked for because of this change. That's sad.

Merge remote-tracking branch 'origin/master' into makefile/adjust-cle…

8f006ba

…an-recipe

georglauterbach dismissed stale reviews from polarathene and casperklein via 8f006ba January 25, 2023 00:09

georglauterbach requested review from casperklein and polarathene January 25, 2023 00:10

polarathene approved these changes Jan 25, 2023

View reviewed changes

casperklein approved these changes Jan 25, 2023

View reviewed changes

georglauterbach merged commit 2033eea into master Jan 25, 2023

georglauterbach deleted the makefile/adjust-clean-recipe branch January 25, 2023 08:16

Uh oh!

Conversation

georglauterbach commented Jan 22, 2023

Description

Type of change

Checklist:

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

georglauterbach commented Jan 22, 2023

Uh oh!

casperklein commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach commented Jan 23, 2023

Uh oh!

casperklein commented Jan 23, 2023

Uh oh!

casperklein Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

casperklein Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

georglauterbach Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

georglauterbach Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

casperklein commented Jan 23, 2023 •

edited

Loading