Thanks for reviving and completing my work from #1258 I did close this pr just now.

Looking at the use of set -eu, I am really not sure whether this is the best solution to be honest, and I would like to hear an opinion on this.

The main intention behind this is to force some "hygiene" when working with bash scripts. set -u makes scripts terminate when an assigned variable does not have a value and set -e terminates the script when one of the commands it uses gives a non-zero return code (i.e. produced an error).

Alright. I was aware of what set -eu does, I was just curious because I read that the use of set -e can have unintentional side effects. Sadly, the Travis build failed. Not sure why though. @fbartels, can you help?

EDIT: Same error as in #1550

the use of set -e can have unintentional side effects

I would say it the other way around. If a step in between fails, but the script continues that could very much have unintentional side effects. But yes, forcing set -e will mean that scripts need to come with a certain level of robustness, make them clean up after themselves, etc. I am ok with removing it, but keeping it forces a higher level of care.

The concrete errors I see are:

not ok 257 checking setup.sh: setup.sh email add and login
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 1251)
#   `assert_output "passdb: [email protected] auth succeeded"' failed
# 
# -- output differs --
# expected : passdb: [email protected] auth succeeded
# actual   : passdb: [email protected] auth failed
# --
# 

Test https://github.com/tomav/docker-mailserver/blob/f225e14a219824e4ebb34e63b144e6d7c1314bcb/test/tests.bats#L1250-L1251

This indeed looks like its unrelated to your change.

not ok 267 checking setup.sh: setup.sh config dkim
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
#  in test file test/tests.bats, line 1415)
#   `assert_success' failed
# 
# -- command failed --
# status : 2
# output : ./setup.sh: 279: ./setup.sh: 2: parameter not set
# --
# 

Not enough parameters left after fail2ban ) shift ; _docker_container fail2ban "$@" ;;?

ot ok 268 checking setup.sh: setup.sh debug fetchmail
# (in test file test/tests.bats, line 1426)
#   `[ "$status" -eq 11 ]' failed

this is the line of the tests https://github.com/tomav/docker-mailserver/blob/f225e14a219824e4ebb34e63b144e6d7c1314bcb/test/tests.bats#L1424-L1426. what happens when you manually run setup.sh with these parameters?

Seems like you mixed up line 279 with 297. The script fails because with set -u, the default value (2048) is not being used - instead the script is stopped with a non-zero exit code. I corrected this with

279 dkim     ) _docker_image generate-dkim-config "${2:-2048}" ;;

instead of just $2.

EDIT: current error

not ok 268 checking setup.sh: setup.sh debug fetchmail
  (in test file test/tests.bats, line 1426)
  `[ "$status" -eq 11 ]' failed

Will tend to it.

EDIT2: I had a look at it, even tested it with my working mailserver instance and the setup.sh from the current master branch. The error message is:

$ ./setup.sh -c mail debug fetchmail
fetchmail: no mailservers have been specified.

To me, this seems to be broken even before I refactored it. Is this even possible?

Investigated error tracking. There was the idea of showing where an error was, and that there was an error. I tried

set -euE
trap '_report_err() $LINENO' ERR

function _report_err()
{
  echo "Error happened on line $1" | tee -a $LOG
}

until I realized:

1. set -E is not standard POSIX sh and therefore incompatible
2. trap '...' ERR is not standard POSIX sh and therefore incompatible
3. the function keyword is not POSIX sh too!?

The first two are important (obviously). Unless we switch to a different interpreter (looking at you, bash), this feature cannot easily be implemented in sh.

Well, personally I wouldn't mind using bash. The setup script does run on the host though, so bash may not be available. In practice I haven't seen a Linux host without bash for ages, though. @fbartels what do you think? Upgrade to bash?

Well, personally I wouldn't mind using bash. The setup script does run on the host though, so bash may not be available. In practice I haven't seen a Linux host without bash for ages, though. @fbartels what do you think? Upgrade to bash?

In addition to the already mentioned points, setup.sh uses EUID as an environment variable. This variable is not defined in POSIX sh too, making the podman part basically unreliable. I had to default to a value to not trigger set -u.

Yes I agree. Almost every host (especially if its also running Docker) will have bash or something that provides the ability to interpret bash. So upgrading this helper script should be no problem.

Implemented the feature, but still: 268 is failing.

EDIT2: Previous edit seemed wrong

Yes, nevermind. My refactoring was incomplete. This should in practice work now. Can you approve @fbartels @erik-wramner ?

@fbartels I am currently working on a bigger PR obsoleting #1228 and #1550

Therefore, I would like to know when this PR (#1595) is to be merged. I'd like to become a contributor to manage these PR's. But first, this and #1596 must be merged, as the new PR I'm currently working on is based on them.

Do you have any information? What about @erik-wramner ?

Usually I leave the merging to @erik-wramner, but you changes look good to me so I'll merge them in now.

Image/Container name detection was removed. Was that on purpose?

INFO=$($CRI ps \
  --no-trunc \
  --format "{{.Image}};{{.Names}}" \
  --filter label=org.label-schema.name="docker-mailserver" | \
  tail -1)

IMAGE_NAME=${INFO%;*}
CONTAINER_NAME=${INFO#*;}

Image/Container name detection was removed. Was that on purpose?

INFO=$($CRI ps \
  --no-trunc \
  --format "{{.Image}};{{.Names}}" \
  --filter label=org.label-schema.name="docker-mailserver" | \
  tail -1)

IMAGE_NAME=${INFO%;*}
CONTAINER_NAME=${INFO#*;}

Not sure what you mean @casperklein, but looking at lines 186 ff. in function main(), both in this PR as well as on master, name detection is there.

You are absolutely right. I don't know, why I missed that. Sorry for the noise.