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

fixed right order for EHLO handler #280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hudecof
Copy link

@hudecof hudecof commented Aug 4, 2021

What do these changes do?

Fix in EHLO handler, where the 250 HELP response line, means without dash should be the last one.
My tetsing postix server do not accept nre EHLO extensions after 250 HELP

Are there changes in behavior for the user?

There is no change for user, just fix according the RFC https://datatracker.ietf.org/doc/html/rfc5321#section-4.1.1.1

Related issue number

None

Checklist

  • [x ] I think the code is well written

@waynew
Copy link
Collaborator

waynew commented Nov 1, 2021

I need to double check this - I think it's the right fix, I think I may have overridden this method within my own code for that same reason.

@waynew waynew added this to the 1.5 milestone Nov 22, 2022
@waynew
Copy link
Collaborator

waynew commented Nov 25, 2022

Confirmed that yes, this is the right fix.

However, we don't have a test failing for this case, so we really should create that test case. @hudecof is that something that you could tackle this week or next?

@hudecof hudecof closed this Nov 29, 2022
@hudecof
Copy link
Author

hudecof commented Nov 29, 2022

I did not want to close this ;)
I could try, to write the test, but bit sure about the time line ;(

@hudecof hudecof reopened this Nov 29, 2022
@pepoluan
Copy link
Collaborator

Running the GHCI to test correctness.

Until we create the test to ensure this won't happen again, I won't merge it though.

I hope I can find some time to write the necessary tests if you're occupied @hudecof

@pepoluan pepoluan added rfc/standards compliance testing Issues/PRs related to the test suite labels Dec 21, 2022
@pepoluan
Copy link
Collaborator

This still use the old GHCI definition file, resulting in very slow qa_docs (and usually ends up in failure)

If you can rebase this to the latest master, @hudecof , I can rerun the tests.

Though again, until tests are written to prevent same issue from happening, I won't merge this.

But at least we can be sure that the changes do not cause any regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-unit-tests rfc/standards compliance testing Issues/PRs related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants