function _defunc removed

[casperklein]

Description

Follow up PR as discussed in #2196

This removes the function _defunc, which probably never worked as intended.

_defunc calls were replaced by _shutdown.

Fixes #

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change that does improve existing functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

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/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[georglauterbach]

LGTM 👍🏼

[polarathene]

LGTM 👍

I see a common pattern though with the "Failed to start.." lines. In some cases the original || _defunc handled in the loop could have been kept to call _shutdown, although I guess that's more complicated/hassle in shell scripts to reference the service index/key :/

Could still be a good case for adding a new panic type to replace the _shutdown calls as this is kind of what it's intended for (repetition and re-use across scripts with common error messages):

function dms_panic
{
# ...
( 'fail-init' ) # PANIC_INFO == <name of service or process that failed to start / initialize>
      SHUTDOWN_MESSAGE="Failed to start ${PANIC_INFO}!"
    ;;
# ...
}

function dms_panic__fail_init { dms_panic 'fail-init' "${1}" "${2}"; }

The other two variants "Failed to fix" / "Failed to remove" may not benefit as much atm 🤷‍♂️

I don't mind either way with _shutdown as is, or adopting dms_panic like above.

[polarathene]

Is there a reason for dropping -f on rm calls?

rm docs:

-f, --force

ignore nonexistent files and arguments, never prompt

I understand that usually docker-mailserver isn't run with an interactive terminal, and prompting probably doesn't happen in that case, but is it desirable to be prompted when using something like docker run -it? We usually only do docker run -t if not using compose commands.

Without -f that also implies it'll error if the file does not exist, which in this context doesn't seem like it should be a concern to throw such an error?

[casperklein]

When using -f, rm will never fail (exit 1), which makes testing for success pointless.

[casperklein]

it'll error if the file does not exist

This should never be the case when using the images we provide?

[polarathene]

This should never be the case when using the images we provide?

🤷‍♂️ haven't looked into it myself, I guess we'll know soon enough 😂

Is the other concern about being prompted when using docker ... -it a misunderstanding on my part too?

[casperklein]

rm <file> never prompts for confirmation. However, many Linux distributions ship with some default aliases like:

alias rm='rm -i' # prompt before every removal
alias cp='cp -i' # prompt before overwrite

PS: Aliases do not work in shell scripts. So it doesn't matter if they exist or not when running a shell script.

[polarathene]

Did you also want to add -SIGTERM to the _shutdown kill command as discussed at the end of #2196 ?

[casperklein]

In some cases the original || _defunc handled in the loop could have been kept to call _shutdown

In most cases, ${FUNC} || _defunc was pretty pointless. This checked only, if the last command in the called function was successful or not. For example:

_importantFunction() {
  doImportantThing1
  doImportantThing2
  echo "useless info"
}

_imortantFunction || echo "something went wrong"

You'll only see the "something went wrong" output, if the last echo command in that functions fails. Pretty pointless if you ask me.

Therefore I've added the extra _shutdown calls explicitly to operations, where I thought they are important.

Ideally, we should set -ueo pipefail or similar on top, so that the scripts immediately fails if something went wrong.

This PR should only obsolete _defunc. I agree, with a further PR the whole thing could be reworked and improved.

[casperklein]

Did you also want to add -SIGTERM to the _shutdown kill command as discussed at the end of #2196 ?

Nope. If someone wants to add it - okay. But I don't see a reason for it (see my comment there). We shouldn't start, providing default settings on the command line.

[polarathene]

Ideally, we should set -ueo pipefail or similar on top, so that the scripts immediately fails if something went wrong.

This sounds good (the functionality, my shell-fu doesn't recall that incantation that well), I suppose it might make sense to do that as early as possible during init scripts?

[casperklein]

set -ueo pipefail That is btw the unofficial BASH strict mode 😉

I suppose it might make sense to do that as early as possible during init scripts?

Yes. But keep in mind, it's not just putting the line on top of the script. The script itself may need some adjustments.

Every command that is expected to fail for whatever reasons, must be catched or the script will fail.

this-command-may-fail-or-not-we-dont-care || true

This allows the rest of the script to continue. Otherwise, in "strict mode" the script will exit on the first failed command.

[georglauterbach]

At the moment, I'd advise against using set -euEo pipefail in the start-stack. This needs seperate PR:)

[casperklein]

The test failed 😞

 not ok 294 checking system: postfix should not log to syslog
# (from function `assert_failure' in file test/test_helper/bats-assert/src/assert.bash, line 140,
#  in test file test/tests.bats, line 538)
#   `assert_failure' failed
# 
# -- command succeeded, but it was expected to fail --
# output : Sep 23 12:24:31 mail postfix/postfix-script[2647]: fatal: the Postfix mail system is already running

[casperklein]

The failed test does not seem to be related to this PR, as it was successful in the past.

Edit: I've restarted the test.
Edit2: Tests passed. Merging now..

[georglauterbach]

I was seeing multiple issues with this test too, unrelated to any of my changes. Disable it too?