Like in the previous PR there are some tests failing:

not ok 242 checking opendkim: generator creates default keys size
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 479)
#   `assert_output 6' failed
# 
# -- output differs --
# expected : 6
# actual   : 4
# --
# 
not ok 243 checking opendkim: generator creates key size 2048
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 505)
#   `assert_output 6' failed
# 
# -- output differs --
# expected : 6
# actual   : 4
# --
# 
not ok 244 checking opendkim: generator creates key size 1024
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 531)
#   `assert_output 6' failed
# 
# -- output differs --
# expected : 6
# actual   : 4
# --
# 
not ok 245 checking opendkim: generator creates keys, tables and TrustedHosts
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 553)
#   `assert_output 6' failed
# 
# -- output differs --
# expected : 6
# actual   : 4
# --
# 
not ok 246 checking opendkim: generator creates keys, tables and TrustedHosts without postfix-accounts.cf
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 584)
#   `assert_output 5' failed
# 
# -- output differs --
# expected : 5
# actual   : 4
# --
# 
not ok 247 checking opendkim: generator creates keys, tables and TrustedHosts without postfix-virtual.cf
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 615)
#   `assert_output 5' failed
# 
# -- output differs --
# expected : 5
# actual   : 4
# --
# 
not ok 248 checking opendkim: generator creates keys, tables and TrustedHosts using domain name
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 646)
#   `assert_output 6' failed
# 
# -- output differs --
# expected : 6
# actual   : 4
# --
# 

Do you need help solving the issues?

Do you need help solving the issues?

Yes, please I am low on time now to fix this
Any help would be awesome, I rebased my work

I fixed the problem by removing the quotes from the argument as they are already set in function call (in this line)

diff --git a/target/bin/generate-dkim-config b/target/bin/generate-dkim-config
index 11b50dd..73ca42f 100755
--- a/target/bin/generate-dkim-config
+++ b/target/bin/generate-dkim-config
@@ -5,7 +5,7 @@ touch /tmp/vhost.tmp
 # if no keysize is provided, 2048 is default.
 KEYSIZE=${1:-2048}
 # optional domain names
-DOMAINS="${2:-''}"
+DOMAINS=${2:-''}
 
 if [[ -z "${DOMAINS}" ]]
 then

Next i wanted to add tests for this kind of functionality but then i discovered a test that is checking dkim only by using domain name which was introduced 4 years ago by 9845375

To be fair, this functionality isn't documented anywhere (got removed from README.md during refactoring by 115ad55 and is not in Wiki currently).

I propose that you take a closer look if generate-dkim-domain would fit your needs too and we then can decide if we want to abandon the "old" script (which is an additional file/command and does not allow the setting of keysize and is not documented but is already tested) in favor of this PR (which would be in the same file/command and does allow the setting of keysize and is documented but is not tested currently)

Either way would be a bit of work to invest. What is your opinion on that?

I would like to decide this before 8.0.0 as removing a old way of generating the dkim keys might break existing setups @aendeavor

Actually LGTM to me. @wernerfred is right, we should merge this before 8.0.0. As I have merged #1770, this is the last step before the next major release.

This was labelled with pr/missing integration tests. @williamdes how fast can you write up two or three tests?

I have added another commit that changes the behavior of the default value. I made this "mistake" earlier - using ${VAR:=} is better than ${VAR:-''}, since the new variable VAR have a completely empty value, instead of the string '' :)

This was labelled with pr/missing integration tests. @williamdes how fast can you write up two or three tests?

Maybe I find some time today for writing the test (at least in the same "dirty" style the other dkim tests are)

But that means that you prefer to delete the generate-dkim-domain script inclusive the tests and instead use the functionality of this PR?

Hi
Thanks for the comments and work
Actually I would let that up to you about writing tests because I am not working on my infrastructure this month

Let me know if you absolutely need something :)

I will be glad to review anyway

And dropping the old script looks like a good idea to me

@wernerfred please by all accounts merge this yourself if you think this is ready to be merged. Reach back to me in @docker-mailserver/maintainers in the discussion for releasing 8.0.0. We can then finalize the release.

@williamdes pls test this asap! All further commits from my side will now be documentation and typo fixing etc.

@williamdes just pinging you here:) See @wernerfred's last Comment.

@wernerfred if you'd like to bump to 4096 that's fine with me - just remember to adjust the test which checks the default value too as well as setup.sh. I may have even missed something more, please double-check on this:)

Good point as this is a breaking change, too.
Please merge #1771 first. I will implement the changes during the day.

Good point as this is a breaking change, too.

Please merge #1771 first. I will implement the changes during the day.

Tell me when you're ready. I will merge #1771 then if it has not been done until then.

LGTM

ready for merging? if yes, go ahead :D

30secs, nice :D