Skip to content
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 blank SMTP_AUTHENTICATION if the SMTP_AUTHENTICATION env var is empty. #60

Merged
merged 1 commit into from
May 6, 2014

Conversation

arnos
Copy link

@arnos arnos commented May 5, 2014

Added if statement to disable SMTP_AUTHENTICATION when SMTP_AUTHENTICATION env var is empty #59

@arnos
Copy link
Author

arnos commented May 5, 2014

@sameersbn I thought so but @LordFPL is using :plain as SMTP_AUTHENTICATION whereas I'm not using anything, though I don't mind passing an empty environment variable when I launch the container.

@sameersbn
Copy link
Owner

@LordFPL do you think removing smtp user, pass and authentication parameters from the config when the smtp user is not defined a good idea? does it work for you?

@LordFPL
Copy link
Contributor

LordFPL commented May 5, 2014

Hi @sameersbn , yes it was the only way to have mails working (smtp, plain).
Warning, there was a modification during the last versions : "plain" is now defined with "plain" and not ":plain" as seen before.
Removing smtp auth is working as "plain" when i made some tests (so, it's the simpliest solution i think ;))

@sameersbn
Copy link
Owner

@arnos can you rebase the PR

@arnos
Copy link
Author

arnos commented May 5, 2014

Yes retesting it and will rebase.

On Mon, May 5, 2014 at 2:34 PM, Sameer Naik notifications@gh.neting.ccwrote:

@arnos https://github.com/arnos can you rebase the PR


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-42221246
.

@arnos
Copy link
Author

arnos commented May 5, 2014

Ok the rebase is done.

@LordFPL I have one issue that doesn't make sense the if statements for SMTP_USER and SMTP_BASE should start with -z (check if length is 0) instead of -n (check if length > 0) no?

@sameersbn
Copy link
Owner

@arnos thats right, it should be -z

@LordFPL
Copy link
Contributor

LordFPL commented May 5, 2014

Oups... my fault... i already made the correction, but forget to copy it in the patch, sorry...

@arnos
Copy link
Author

arnos commented May 5, 2014

Ok then I'm not sure why my if is working with -n (if I put -z the default
value of :login gets set)

On Mon, May 5, 2014 at 3:18 PM, LordFPL notifications@github.com wrote:

Oups... my fault... i already made the correction, but forget to copy it
in the patch, sorry...


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-42226733
.

@sameersbn
Copy link
Owner

I think when -z / -n are used, the test string should not be surrounded by quotes. That's why I prefer using == operators

@arnos
Copy link
Author

arnos commented May 5, 2014

Ok that makes sense. In this case if we can keep the -n for
SMTP_AUTHENTICATION it works (ditto entering a value will pass the value
and not passing the parameter will set :login) and I'm using -z for the
SMTP_USER/SMTP_PASS and everything is working (finally)

Thanks Sameer.

On Mon, May 5, 2014 at 3:32 PM, Sameer Naik notifications@gh.neting.ccwrote:

I think when -z / -n are used, the test string should not be surrounded by
quotes. That's why I prefer using == operators


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-42228387
.

@sameersbn sameersbn merged commit 8455e5c into sameersbn:master May 6, 2014
@sameersbn
Copy link
Owner

@arnos I have fixed the issue with the -z/-n

Ok then I'm not sure why my if is working with -n (if I put -z the default value of :login gets set)

:login gets set on SMTP_AUTHENTICATION because it is set by default, even if you don't pass SMTP_AUTHENTICATION or pass it as an empty string this value is internally set to :login
SMTP_AUTHENTICATION=${SMTP_AUTHENTICATION:-:login}

I have changed the code b8266c0 so that the default value of SMTP_AUTHENTICATION is set to :login only if SMTP_USER is set. So i think this should work

@LordFPL
Copy link
Contributor

LordFPL commented May 6, 2014

Hi,

Just back to work, i try the new version : all is working ! :)
Thx :)

@arnos
Copy link
Author

arnos commented May 6, 2014

I'll retest it tomorrow, this was bugging me as it didn't make sense that
it worked

On Tuesday, May 6, 2014, Sameer Naik notifications@github.com wrote:

@arnos https://github.com/arnos I have the issue with the -z/-n

:login gets set on SMTP_AUTHENTICATION because it is set by default, even
if you don't pass SMTP_AUTHENTICATION or pass it as an empty string this
value is internally set to :login
SMTP_AUTHENTICATION=${SMTP_AUTHENTICATION:-:login}

I have changed the code so that the default value of SMTP_AUTHENTICATION
is set to :login only if SMTP_USER is set. So i think this should work


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-42269752
.

@sameersbn
Copy link
Owner

Have added a new SMTP configuration option named SMTP_ENABLED which is set to false by default. Please refer #39 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants