-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adding the possibility to use a third party milter without activating #1
base: develop
Are you sure you want to change the base?
Conversation
Frdric Maussion seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
jylibs/serverconfig.py
Outdated
|
||
if self["zimbraMtaSmtpdMilters"] is not None and milter is not None: | ||
self["zimbraMtaSmtpdMilters"] = "%s, %s" % (self["zimbraMtaSmtpdMilters"], milter) | ||
elif self["zimbraMtaSmtpdMilters"] is None and milter is not None: | ||
self["zimbraMtaSmtpdMilters"] = milter | ||
elif self["zimbraMtaSmtpdMilters"] is not None and milter is None: | ||
self["zimbraMtaSmtpdMilters"] = "%s, %s" % (self["zimbraMtaSmtpdMilters"]) |
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 template string is incorrect here (line 147). The template string is set to accept two strings ("%s, %s"), but only one is provided.
This also seems a bit redundant, since the value is already set and this logic would just simply reset the self["zimbraMtaSmtpdMilters"] value with the value it already has. What would the purpose of the logic on line 146/147 be?
…rServerEnabled is set to FALSE
Finally manage to take the time to take a look. |
@jeastman any chance to review the latest commit ? |
Hi @jeastman can you please comment on this? |
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 may have missed something, but if not, I think this could be simplified as identified below...
@@ -137,12 +137,14 @@ def load(self, hostname): | |||
if (self["zimbraMilterServerEnabled"] == "TRUE"): | |||
milter = "inet:%s:%s" % (self["zimbraMilterBindAddress"],self["zimbraMilterBindPort"]) | |||
else: | |||
self["zimbraMtaSmtpdMilters"] = "" | |||
milter = None |
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.
milter
is set to None on line 136. No need set that again. Why not just remove lines 139 and 140?
|
||
if self["zimbraMtaSmtpdMilters"] is not None and milter is not None: | ||
self["zimbraMtaSmtpdMilters"] = "%s, %s" % (self["zimbraMtaSmtpdMilters"], milter) | ||
elif self["zimbraMtaSmtpdMilters"] is None and milter is not None: | ||
self["zimbraMtaSmtpdMilters"] = milter | ||
else: |
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.
Forgive my ignorance, but is this else
and the next line even necessary?
Per default you can't configure a milter wihtout activating the one build in with Zimbra.
The proposed patch add a third scenario where you can use one or several milter wihtout activating the Zimbra one.
Normal Behavior with Milter Zimbra Activated
We add an additional milter with the zimbra milter activated
Normal Behavior without Milter Zimbra Activated
We add an additional milter with the zimbra milter desactivated
After patch