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

Ssl version named constants #1890

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

javabrett
Copy link
Collaborator

@javabrett javabrett commented Oct 7, 2018

I found that I had this old contribution lying around without a matching PR.

Fixes #1454 , fixes #1114 . /cc @tilgovi .

Copy link
Collaborator

@sirkonst sirkonst left a comment

Choose a reason for hiding this comment

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

What about negative tests, with wrong setting?

@javabrett javabrett force-pushed the ssl-version-named-contants branch 10 times, most recently from 840ea89 to 06c6efa Compare October 11, 2018 04:19
@javabrett
Copy link
Collaborator Author

@sirkonst I added a commit which adds the negative tests, and tightens-up validation on the integer version of the constants (these were previously only checked for positive-integer, they now need to match a valid ssl.PROTOCOL_... value.

Removed Python 2.x tests and shuffled the remainder to match the ever-shifting sands of these constants across Python 3.x ssl.

@javabrett
Copy link
Collaborator Author

Also any +1's or -1's on #1680 appreciated.

@berkerpeksag
Copy link
Collaborator

Could you please squash all commits into one and add your name as the co-author of the patch in the commit message?

Co-Authored-By: Brett Randall <javabrett@gmail.com>

THANKS file needs to be updated as well.

docs/source/settings.rst Outdated Show resolved Hide resolved
docs/source/settings.rst Outdated Show resolved Hide resolved
docs/source/settings.rst Outdated Show resolved Hide resolved
docs/source/settings.rst Outdated Show resolved Hide resolved
docs/source/settings.rst Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
@javabrett
Copy link
Collaborator Author

I've returned to this with a rebase/squash, updated THANKS and amended the commit message. I'll add some new commits for the feedback and squash those later.

@javabrett
Copy link
Collaborator Author

Feel free to squash those last few commits before/during merge, or re-review and I will squash them.

@javabrett javabrett changed the title Ssl version named contants Ssl version named constants Oct 18, 2018
@benoitc
Copy link
Owner

benoitc commented Oct 29, 2018

ping @berkerpeksag ok to merge? what's missing?

gunicorn/config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
@berkerpeksag
Copy link
Collaborator

@javabrett you can squash these commits after you address my latest review comments. I'd do it myself before merge, but there is a bug in GitHub when a PR is authored by multiple authors and the merger use the "squash" option, so we may not able to attribute your work correctly in the squashed commit. It's much safer if you squash the commits yourself and add the following line to the commit message:

Co-Authored-By: Brett Randall <javabrett@gmail.com>

Thanks!

Fixes benoitc#1114

Co-Authored-By: Brett Randall <javabrett@gmail.com>
Signed-off-by: Brett Randall <javabrett@gmail.com>
@berkerpeksag
Copy link
Collaborator

@javabrett thank you for finishing this! I think this change will make Gunicorn much more user friendly.

@javabrett javabrett deleted the ssl-version-named-contants branch October 31, 2018 19:55
@javabrett javabrett mentioned this pull request Dec 11, 2018
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.

Add support for named constants in the --ssl-version flag
4 participants