kubernetes fix#484
Conversation
|
Thank you @KCrawley |
tomav
left a comment
tomav
left a comment
There was a problem hiding this comment.
Thanks for the job to finish this PR.
No major comment on my side except this default value and the missing integration test.
I can provide help on test if needed, but existing example may be helpful.
| DEFAULT_VARS["SMTP_ONLY"]="${SMTP_ONLY:="0"}" | ||
| DEFAULT_VARS["VIRUSMAILS_DELETE_DELAY"]="${VIRUSMAILS_DELETE_DELAY:="7"}" | ||
| DEFAULT_VARS["DMS_DEBUG"]="${DMS_DEBUG:="0"}" | ||
| DEFAULT_VARS["OVERRIDE_HOSTNAME"]="${OVERRIDE_HOSTNAME:="false"}" |
There was a problem hiding this comment.
I don't think "false" is a good default value in that case.
This should remain empty if not provided, no?
| notify "task" "Check that hostname/domainname is provided (no default docker hostname) [$FUNCNAME]" | ||
| notify "task" "Check that hostname/domainname is provided or overidden (no default docker hostname/kubernetes) [$FUNCNAME]" | ||
|
|
||
| if [[ ${DEFAULT_VARS["OVERRIDE_HOSTNAME"]} != "false" ]]; then |
|
Additionally, I think it will be right to remove this and restore this stuff for OpenDMARC with this PR. Currently it uses
|
|
|
||
| run: | ||
| # Run containers | ||
| docker run -d --name mail_hostoverride \ |
There was a problem hiding this comment.
Could you use an existing container like mail_smtponly?
It avoids to launch to many of them consuming CPU and memory.
Otherwise it's ok!
Thanks.
| } | ||
|
|
||
| @test "checking configuration: hostname/domainname override" { | ||
| run docker exec mail_hostoverride /bin/bash -c "cat /etc/mailname | grep my-domain.com" |
|
LGTM, waiting for travis (just restarted it) |
|
@tyranron this can be merged without your last comment or not? |
|
It's not difficult to |
|
And thanks @iMartyn for the initial PR. |
* Allow OVERRIDE_HOSTNAME * Document the new environment variable
4 participants
This was derived from #390. I'm not trying to step on toes, I just happened to be setting up a mail server on my kubernetes homelab this evening.
I believe amavis should pass integration tests now with the changes I've made. We'll see.