-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added smarthost support #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Duumke good job, thanks for the contribution.
I left some reviews for you, please check.
@@ -18,6 +18,7 @@ Environment Variables: | |||
SMF_DOMAIN - mail server hostname. use tutum/docker hostname if omitted. | |||
SMF_CONFIG - mail forward addresses mapping list. | |||
SMF_MYNETWORKS - configure relaying from trusted IPs, see http://www.postfix.org/postconf.5.html#mynetworks | |||
SMF_SMARTHOST - configure a smarthost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we should use SMF_RELAYHOST
as the variable name as we are modifying the relayhost
parameter of postfix.
@@ -171,6 +172,15 @@ function start_postfix { | |||
echo "$HOSTNAME" > /etc/mailname | |||
echo "$HOSTNAME" > /etc/hostname | |||
|
|||
if [ "$SMF_SMARTHOST" = "" ]; then | |||
SMF_SMARTHOST=$1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not necessary to use $1
as the default value of SMF_RELAYHOST
. Because it's not a so important setting for all users.
If there's no SMF_RELAYHOST set, we should just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks no problem now, please also add related document to README.
@@ -18,6 +18,7 @@ Environment Variables: | |||
SMF_DOMAIN - mail server hostname. use tutum/docker hostname if omitted. | |||
SMF_CONFIG - mail forward addresses mapping list. | |||
SMF_MYNETWORKS - configure relaying from trusted IPs, see http://www.postfix.org/postconf.5.html#mynetworks | |||
SMF_RELAYHOST - configure a smarthost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smarthost
should also rename to relayhost
I hope you agree with it now. If it's still not right, I'll leave it up to you to accept and adjust or deny completely. |
All looks good, thanks! |
Smarthost support added; this wil solve issue #18