Skip to content

Fix connecting SMTP on localhost only - #194

Merged
justb4 merged 1 commit into
geopython:masterfrom
faegi:smtpfix
Oct 16, 2018
Merged

Fix connecting SMTP on localhost only#194
justb4 merged 1 commit into
geopython:masterfrom
faegi:smtpfix

Conversation

@mkofahl

@mkofahl mkofahl commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

Fix connecting localhost by calling connect() on an already initialized smtplib object, introduced in 5ef60b3. According to the docs, smtplib.SMTP() calls the connect method for the given host. Calling connect again but without any parameters will fall back to smtplibs default, which is localhost:25.

…alized smtplib object, introduced in 5ef60b3. According to the docs, `smtplib.SMTP()` calls the `connect` method for the given host. Calling `connect` again but without any parameters will fall back to smtplibs default, which is localhost:25.
@justb4 justb4 added this to the Version 0.4.0 milestone Sep 15, 2018
@justb4
justb4 self-requested a review September 18, 2018 08:35
@justb4

justb4 commented Sep 18, 2018

Copy link
Copy Markdown
Member

I need to understand the rationale for this PR, according to the docs:

 server = smtplib.SMTP(config['GHC_SMTP']['server'],
                          config['GHC_SMTP']['port'])

will already connect(). As config['GHC_SMTP']['server'] and config['GHC_SMTP']['port'] should always be set (see check at notifications.py#L85), I don't see the need for calling server.connect(). But I could be missing something here. In all deployments up to now we have been using SSL/TLS so this case may have never emerged.

@ghost

ghost commented Oct 15, 2018

Copy link
Copy Markdown

After doing some tests with @tomkralidis on our version of GeoHealthCheck, we discovered that we needed those changes in order to resolve an issue that we had with the email notifications. After applying them, our notifications worked again.

Thanks.

@justb4

justb4 commented Oct 16, 2018

Copy link
Copy Markdown
Member

Sorry folks I overlooked: yes indeed:

  • GHC requires host and port in GHC_SMTP config (when email notif enabled)
  • smtplib.SMTP() constructor always calls connect() when/with host+port specified
  • GHC should not call connect() again (defaults to localhost:25)
  • problem only will popup when SSL/TLS is not used (and host=otherhost port=any)

So merging!

@justb4
justb4 merged commit 0201a26 into geopython:master Oct 16, 2018
@justb4

justb4 commented Oct 16, 2018

Copy link
Copy Markdown
Member

Thanks for patience!

@justb4 justb4 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

@justb4 justb4 added the bug label Oct 16, 2018
justb4 added a commit that referenced this pull request Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants