If that is the reason wouldn't it then make sense to modify the other commands to use _docker_container instead of _docker_image too?

First of all: Why was the _docker_image thing chosen in the first place. I'm under the impression that there's a valid reason and I feel like someone may know more: @erik-wramner do you have an idea?

_docker_image was fine, until #897 got merged. That commit introduced the "delete mailbox feature".
This makes it necessary to access the mailbox files (to delete them). delmailuser works fine within the container. But when called via setup.sh, a new container was started, that didn't have access to the mailbox files.

_docker_image was chosen before that feature, because no access to the mailbox was necessary. Only access to /tmp/docker-mailserver/postfix-accounts.cf was needed, which _docker_image provided when starting a new container:

History shows the original _docker method for image use was replaced in 2016 with a separate command for image, and a new one for a running container. There wasn't much discussion about the inclusion.

@wernerfred

If that is the reason wouldn't it then make sense to modify the other commands to use _docker_container instead of _docker_image too?

The advantage of _docker_image is, that no running container is necessary. So you can for example create new mailboxes, before having a fully configured/running docker-mailserver yet. The downside is, that when you already have a running container, starting a new separate one will cost some resources.

Are the other commands working bc they do only modify options/settings that are persisted in the mailstate? But those states arent mounted by _docker_image too?

Yes. Because we don't know, where the maildata exactly resides on the host (no we should not think about parsing docker-compose.yaml 🙏), we cannot start a new container and mounting the maildata easily. That's why for this kind of task, a running container is probably the best way to have access to the needed mounts.

To sum things up:
_docker_image for tasks that only need access to /tmp/docker-mailserver
_docker_container for tasks that need also access to maildata, maillogs and mailstate mounts. But also for tasks that obviously need a running container like setup.sh debug login.

While writing this, I noticed, that _docker_image also has the ability, to use a running container. But then, -c (CONTAINER_NAME) must be given to setup.sh.

We could discard this PR, but then -c would be mandatory for the setup.sh email del command.

We could discard this PR, but then -c would be mandatory for the setup.sh email del command.

If there isn't a situation where it'd make sense to use _docker_image, then the PR should be merged?

Regarding the rest of your comment, that seems like useful information to add to documention? Seems some of it would be relevant to maintainers/contributors and another part maybe for the existing setup.sh docs page?

After reading through all of this, I could provide a PR that reverses my last PR. With my last PR I disabled the possibility to delete more than one user at a time with one run of setup.sh.

Ping me if I should re-enable this.

This look good however:)

Regarding the rest of your comment, that seems like useful information to add to documention? Seems some of it would be relevant to maintainers/contributors and another part maybe for the existing setup.sh docs page?

I had a look at the documentation, but to be honest, I've no idea where to put it at best. What exactly would you suggest to put at setup.sh?

Creating a new category for developers, which e.g. describes the internal works of setup.sh, would be a bigger task. If someone is going to start this, then it should be done in a separate PR.

Ping me if I should re-enable this.

Depends on your spare time 😉 Go ahead if it's no big deal for you 👍 But if we leave it as it is, it's also fine I think.

Are you going to document this within this PR?

I don't think so at the moment. But I'll wait for @polarathene feedback or from anybody else of course.

What exactly would you suggest to put at setup.sh?

It could be added as a tip admonition. The syntax in markdown for that can be noticed by !!! or ???, followed by a type such as tip,note,etc and a quoted string for title, a blank line afterwards followed by content that is indented by 4 spaces.

I can add it if you like. I'll add it to my backlog?

I'm presently chipping away at the contributor section for documentation (both a contributor and maintainer page). That has priority as it will help better inform contributors how to write documentation.

Creating a new category for developers, which e.g. describes the internal works of setup.sh, would be a bigger task. If someone is going to start this, then it should be done in a separate PR.

This can just be a maintainer notes page with a heading/section for notes on setup.sh. That information can be more informal and sparse, it's still useful to collect in a common place rather than scattered issue threads :)

We have a roughed out a contributing section already with stubs for some pages like writing tests for example. Notes for maintainers could be added to a new maintainers.md file or similar there.

From a new user perspective, viewing the current setup.sh docs page, there is a bunch of example commands shown without any usage of -i or -c. There's no way to differentiate when it's being handled differently or is required to be specifically -c via this command help/examples.

The advantage of _docker_image is, that no running container is necessary. So you can for example create new mailboxes, before having a fully configured/running docker-mailserver yet. The downside is, that when you already have a running container, starting a new separate one will cost some resources.

This could be a note after the Usage section on the docs page for users. But with _docker_image changed to be tailored as a note about when to choose -i vs -c, or if they don't need to bother with either (default -i value may be sufficient, or -c may be required as noted here?).

I'm not great at parsing the setup.sh myself, it looks like there is some fallback to try get the container name without using -c?

• _docker_image for tasks that only need access to /tmp/docker-mailserver
• _docker_container for tasks that need also access to maildata, maillogs and mailstate mounts. But also for tasks that obviously need a running container like setup.sh debug login.

Probably useful for anyone maintaining/auditing the script. Could be added to maintainer docs, or inline documentation within the script file itself? (The docs Usage section with the CLI output looks like it's possibly out of date/sync?)

While writing this, I noticed, that _docker_image also has the ability, to use a running container. But then, -c (CONTAINER_NAME) must be given to setup.sh.

Would this have alleviated the issue prior to any changes? Is it an indication that maintainers and users aren't as aware of this approach and it could be better highlighted/emphasized in our new docs?

!!! tip "Target a running container instance"

    By default `setup.sh` commands will run in a new container without any volumes attached. This is useful when setting up a mailserver, however for maintenance sometimes you need to target a running instance that has volumes attached.

    Use `-c <container name>` to target an existing `docker-mailserver` instance for `setup.sh` commands to run within.

That may be a bit inaccurate, I haven't actually used this project properly in some time and forget what it's like using this file :)

It seems to suggest it's run externally of the container, to run some commands within the container? A little puzzling since we have docker exec / docker-compose exec that could run the correct setup.sh embedded within the image release that's running the container.

May be better to move this off to a separate discussion/issue thread. I have no opinion on merging or closing the PR personally, just an interest in the UX and maintainance of setup.sh itself.

I can add it if you like. I'll add it to my backlog?

Yes please ❤️

there is a bunch of example commands shown without any usage of -i or -c.

I guess, that's because -i/-c aren't required in a standard setup. IMAGE_NAME is hardcoded to docker-mailserver:latest and CONTAINER_NAME is automatically detected. So normally you don't have to use one of those switches. I only mentioned it, as a workaround for the currently not working email del command. My point of view is, that setup.sh should work out of the box and not requiring to manually give the image or container name. I cannot imagine a real use case for those switches.

Would this have alleviated the issue prior to any changes?

I am not sure. The whole error was a bit hard to track down IMHO. Also it was a clear bug. Not some kind of misusage of setup.sh.

Is it an indication that maintainers and users aren't as aware of this approach and it could be better highlighted/emphasized in our new docs? it could be better highlighted/emphasized in our new docs?

More/better documentation is always good 👍

May be better to move this off to a separate discussion/issue thread.

Yes. I am going to merge this now. As you may noticed, documentation does not belong to my strengths 😕. However I am happy to support/review/help when it goes to technical contents.

@casperklein

My point of view is, that setup.sh should work out of the box and not requiring to manually give the image or container name.

I just spent hours of my life trying to figure out what the heck is wrong with my delete and create commands, all to find out that -c is necessary... This definitely needs to work out of the box :)

As this has been fixed, you should update to the latest setup.sh file.