Allow handle_EHLO to change the responses to EHLO #157

cnicodeme · 2018-12-10T13:13:46Z

Closes #155

aiosmtpd/smtp.py

waynew

I've finally found time to jump back in the saddle here!

I really like this PR and I think it's shaping up to be the right thing. My other comments mentioned some other changes that I'd like to see, but if we could get some tests and docs updated, I think that this PR would be great!

aiosmtpd/smtp.py

codecov · 2020-12-29T08:16:41Z

Codecov Report

Merging #157 (a3fe96f) into master (46540ff) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #157   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1100      1121   +21     
  Branches       197       201    +4     
=========================================
+ Hits          1100      1121   +21

Impacted Files	Coverage Δ
aiosmtpd/smtp.py	`100.00% <100.00%> (ø)`
aiosmtpd/lmtp.py	`100.00% <0.00%> (ø)`
aiosmtpd/main.py	`100.00% <0.00%> (ø)`
aiosmtpd/handlers.py	`100.00% <0.00%> (ø)`
aiosmtpd/controller.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46540ff...aded931. Read the comment docs.

pepoluan · 2020-12-29T08:41:49Z

Okay I'm really thinking about backward compatibility here...

Current behavior is that handle_EHLO only adds to existing list. This is the behavior of the code in master, and it is documented.

Changing this behavior will break code that expects that behavior.

So I will implement the following strategy:

Depending on the signature of the handle_EHLO hook,

If handle_EHLO takes only 4 (three) args -- as is expected currently -- do the old behavior (adding)
If handle_EHLO takes > 4 args -- adding the list of responses as the 5th arg -- do the new behavior (replacing)

pepoluan · 2020-12-29T17:49:12Z

Changing this to draft because there are some behavior that might break existing code.

Will undraft after I implement backward-compatibility workarounds.

The older behavior has been documented. Existing code that relies on the older behavior will break if backward-compatibility logic is not implemented. This workaround has been documented in handlers.rst

pepoluan · 2020-12-29T17:58:45Z

I will undraft this after testing on my test systems.

waynew

Just one question but not a blocker. I like the backwards compatibility here - I think we should plan on doing a deprecation/warning introduction release regularly, like maybe once per year or something. But that's orthogonal to this PR 🙃

aiosmtpd/smtp.py

Also explaining the effect of setting host_name to false-y in the documentation.

Also do optional-import of colorama in conf.py Plus rewriting many os.path things into pathlib.Path things to guarantee cross-platform compatibility.

pepoluan · 2020-12-30T06:53:04Z

Currently doing testing on my test systems:

Windows 10 (via PyCharm tox runner): (ALL)
Windows 10 (via PSCore 7.1.0): (ALL)
Windows 10 (via Cygwin): qa,py36-{nocov,cov}
Ubuntu 18.04 on WSL 1.0: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 18.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 20.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
FreeBSD 12.1 on VBox: (ALL) + pypy3-nocov

(BTW, these tests are running against commit a0f963b [see below] ... I just forgot to push to cnicodeme's branch)

lgtm-com · 2020-12-30T08:53:12Z

This pull request introduces 1 alert when merging a0f963b into 6c8fdbc - view on LGTM.com

new alerts:

1 for Unused import

pepoluan · 2020-12-30T10:00:13Z

This pull request introduces 1 alert when merging a0f963b into 6c8fdbc - view on LGTM.com

new alerts:
* 1 for Unused import

Huh? I thought I've removed that line?

Oh well, here's to another edit-commit-push-check.

Had this not been excluded, "qa" would catch the unused import there. Why it was excluded, I do not know ... after all, running qa on conf.py passes with flying colors...

pepoluan · 2021-01-01T07:24:42Z

All my tests passed, I'm Undrafting this.

pepoluan · 2021-01-01T07:26:02Z

Whoops, there are some conflicts. Redrafting while I fix those.

# Conflicts: # aiosmtpd/smtp.py

Because this is milestoned for 1.3 I want to close 1.2.x line by mid-2021-01

pepoluan · 2021-01-01T07:38:28Z

Because of the major merging, I'm rerunning my tests

Progress:

Windows 10 (via PyCharm runner): (ALL)
Windows 10 (via PSCore 7.1.0): (ALL)
Windows 10 (Cygwin): qa,py36-{nocov,cov}
Ubuntu 18.04 on WSL 1.0: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 18.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 20.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
FreeBSD 12.1 on VBox: (ALL) + pypy3-nocov

pepoluan · 2021-01-03T08:23:01Z

Ugh, finally FreeBSD testing is complete.

I don't know why but my FreeBSD VM seems to ... degrade? It usually finishes in an acceptable time without much fuss, but now it runs really slowly I had to add another core ... and I get intermittent failures due to timeouts.

I'll undraft this and open this for review.

pepoluan · 2021-01-03T08:50:22Z

aiosmtpd/tests/test_smtp.py

+            ]
+            # for actual, expected in zip(lines, expecteds):
+            #     self.assertEqual(actual, expected)
+            self.assertEqual(expecteds, actuals)


Better to compare whole-list against whole-list, to see where we went wrong.

pepoluan · 2021-01-03T08:51:45Z

tox.ini

@@ -74,7 +75,6 @@ deps:
    {[qadocs]deps}

 [flake8]
-exclude = conf.py


Do not exclude conf.py se we can catch possible problems.

pepoluan · 2021-01-03T08:52:10Z

tox.ini

@@ -54,6 +54,7 @@ passenv =
 basepython = python3
 envdir = {toxworkdir}/python3
 deps =
+    colorama


Small test-dep, really helpful for parsing the reams of output.

pepoluan · 2021-01-03T09:12:28Z

If no objections within 2x24 I'll squamerge this 😄

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # conf.py

lgtm-com · 2021-01-19T16:52:03Z

This pull request introduces 1 alert when merging a682b4a into 9432d76 - view on LGTM.com

new alerts:

1 for Unused import

pepoluan · 2021-01-19T16:58:14Z

This pull request introduces 1 alert when merging a682b4a into 9432d76 - view on LGTM.com

new alerts:
* 1 for Unused import

Oh fer cryin' out loud ...

# Conflicts: # aiosmtpd/smtp.py

pepoluan · 2021-01-25T08:00:17Z

Carrying out final verification, just in case merging with master causes problems:

Windows 10 (via PyCharm): (ALL)
Windows 10 (via PSCore 7.1.1): (ALL)
Windows 10 (Cygwin): qa,py36-{nocov,cov}
Ubuntu 18.04 on WSL 1.0: (ALL) + pypy3-{nocov,cov,diffcov}
FreeBSD 12.2 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
OpenSUSE 15.2 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}

(On commitpush, this PR's 'new' GA yml [adopted from #202 ] will test on Ubuntu-{18,20}.4, MacOS, and Windows Server.)

This post is undergoing editing

Reasons: 1. I'm going to drop my Ubuntu-on-VBox testing systems 2. I can't test on MacOs, GA can

Don't worry, PLATFORM==linux is not used anywhere in the code.

pepoluan · 2021-01-26T09:32:37Z

All 24 checks passed. Marinating this for 2x24, then squamerge.

Suggestion on fixing issue aio-libs#155

fc1e030

cnicodeme mentioned this pull request Dec 10, 2018

EHLO command returns all bunch of 250- data, even when the hooks returns otherwise #155

Closed

balki reviewed Jan 4, 2019

View reviewed changes

aiosmtpd/smtp.py Show resolved Hide resolved

waynew requested changes Jun 1, 2019

View reviewed changes

aiosmtpd/smtp.py Outdated Show resolved Hide resolved

pepoluan added 3 commits December 29, 2020 15:07

Merge branch 'master' into defer-ehlo-resp-155

12c7a98

Change test to compare whole list at once

dbbd2a5

Don't immediately push 250-AUTH response

95d4f68

pepoluan changed the title ~~Suggestion on fixing issue #155~~ Allow handle_EHLO to change the responses to EHLO Dec 29, 2020

pepoluan marked this pull request as draft December 29, 2020 17:48

pepoluan added 3 commits December 30, 2020 00:55

Implement backward-compatibility

dbab4ee

The older behavior has been documented. Existing code that relies on the older behavior will break if backward-compatibility logic is not implemented. This workaround has been documented in handlers.rst

Implement test for hybrid behavior

ecde296

Some cosmetics

5746ac5

pepoluan added this to the 1.3 milestone Dec 29, 2020

waynew approved these changes Dec 30, 2020

View reviewed changes

aiosmtpd/smtp.py Outdated Show resolved Hide resolved

pepoluan added 3 commits December 30, 2020 13:21

Remove defensive check-and-set of host_name

dc785da

Also explaining the effect of setting host_name to false-y in the documentation.

Add colorama to qa/docs testenv

85a34de

Also do optional-import of colorama in conf.py Plus rewriting many os.path things into pathlib.Path things to guarantee cross-platform compatibility.

Add news fragment to NEWS.rst

f2f9ba4

BUGFIX: set host_name for no EHLO hook situation

a0f963b

pepoluan added 2 commits December 30, 2020 17:02

Remove unused import

837bbf3

Remove exclusion of conf.py

f6c084a

Had this not been excluded, "qa" would catch the unused import there. Why it was excluded, I do not know ... after all, running qa on conf.py passes with flying colors...

pepoluan marked this pull request as ready for review January 1, 2021 07:24

pepoluan added 2 commits January 1, 2021 14:35

Merge branch 'master' into defer-ehlo-resp-155

fbacc3b

# Conflicts: # aiosmtpd/smtp.py

Version upped to 1.3.0a1

c0c6e54

Because this is milestoned for 1.3 I want to close 1.2.x line by mid-2021-01

pepoluan marked this pull request as ready for review January 3, 2021 08:23

pepoluan added 3 commits January 3, 2021 15:44

Explicitly warning deprecation in v2.0

0f0f167

Add test to ensure DeprecationWarning for 4-arg

1de3d6b

Rename EHLOHandler to EHLOHandlerDeprecated

17e27f5

pepoluan reviewed Jan 3, 2021

View reviewed changes

Change variable name to "_" because "qa" complained

16e34c7

pepoluan mentioned this pull request Jan 7, 2021

Hooks returning None should make smtp.py behave like nothing happened. #154

Open

Merge branch 'master' into defer-ehlo-resp-155

a682b4a

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # conf.py

pepoluan added 2 commits January 20, 2021 00:00

Remove unused import to silence LGTM.com

7555fda

Merge branch 'master' into defer-ehlo-resp-155

a3fe96f

# Conflicts: # aiosmtpd/smtp.py

pepoluan added the enhancement label Jan 24, 2021

Merge branch 'master' into defer-ehlo-resp-155

52e2522

# Conflicts: # aiosmtpd/smtp.py

pepoluan added 4 commits January 26, 2021 11:23

Adopt GA workflow YAML from pytest_based

87112dd

Reasons: 1. I'm going to drop my Ubuntu-on-VBox testing systems 2. I can't test on MacOs, GA can

Change default PLATFORM from linux to posix

4283003

Don't worry, PLATFORM==linux is not used anywhere in the code.

Impose minversions for tox and pytest

a465f44

Add a comment w.r.t. flake8 config in tox.ini

aded931

pepoluan added the ready-to-merge PRs that are ready to merge label Jan 26, 2021

pepoluan merged commit 1520c04 into aio-libs:master Jan 28, 2021

pepoluan mentioned this pull request Feb 17, 2021

[2.0] Possible Breaking Changes #229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow handle_EHLO to change the responses to EHLO #157

Allow handle_EHLO to change the responses to EHLO #157

cnicodeme commented Dec 10, 2018 •

edited by pepoluan

Loading

waynew left a comment

codecov bot commented Dec 29, 2020 •

edited

Loading

pepoluan commented Dec 29, 2020 •

edited

Loading

pepoluan commented Dec 29, 2020

pepoluan commented Dec 29, 2020

waynew left a comment

pepoluan commented Dec 30, 2020 •

edited

Loading

lgtm-com bot commented Dec 30, 2020

pepoluan commented Dec 30, 2020

pepoluan commented Jan 1, 2021

pepoluan commented Jan 1, 2021

pepoluan commented Jan 1, 2021 •

edited

Loading

pepoluan commented Jan 3, 2021

pepoluan Jan 3, 2021

pepoluan Jan 3, 2021

pepoluan Jan 3, 2021

pepoluan commented Jan 3, 2021

lgtm-com bot commented Jan 19, 2021

pepoluan commented Jan 19, 2021

pepoluan commented Jan 25, 2021 •

edited

Loading

pepoluan commented Jan 26, 2021 •

edited

Loading

Allow handle_EHLO to change the responses to EHLO #157

Allow handle_EHLO to change the responses to EHLO #157

Conversation

cnicodeme commented Dec 10, 2018 • edited by pepoluan Loading

waynew left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 29, 2020 • edited Loading

Codecov Report

pepoluan commented Dec 29, 2020 • edited Loading

pepoluan commented Dec 29, 2020

pepoluan commented Dec 29, 2020

waynew left a comment

Choose a reason for hiding this comment

pepoluan commented Dec 30, 2020 • edited Loading

lgtm-com bot commented Dec 30, 2020

pepoluan commented Dec 30, 2020

pepoluan commented Jan 1, 2021

pepoluan commented Jan 1, 2021

pepoluan commented Jan 1, 2021 • edited Loading

pepoluan commented Jan 3, 2021

pepoluan Jan 3, 2021

Choose a reason for hiding this comment

pepoluan Jan 3, 2021

Choose a reason for hiding this comment

pepoluan Jan 3, 2021

Choose a reason for hiding this comment

pepoluan commented Jan 3, 2021

lgtm-com bot commented Jan 19, 2021

pepoluan commented Jan 19, 2021

pepoluan commented Jan 25, 2021 • edited Loading

pepoluan commented Jan 26, 2021 • edited Loading

cnicodeme commented Dec 10, 2018 •

edited by pepoluan

Loading

codecov bot commented Dec 29, 2020 •

edited

Loading

pepoluan commented Dec 29, 2020 •

edited

Loading

pepoluan commented Dec 30, 2020 •

edited

Loading

pepoluan commented Jan 1, 2021 •

edited

Loading

pepoluan commented Jan 25, 2021 •

edited

Loading

pepoluan commented Jan 26, 2021 •

edited

Loading