-
Notifications
You must be signed in to change notification settings - Fork 265
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
vendor requests_kerberos v0.12.0 #268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor nit on the setup.py extras_requires. Just wanted to verify that this is a direct copy of the master branch of requests-kerberos. Do you have any extra changes, apart from the license header?
6a05d5a
to
7b5751c
Compare
Pending upstream changes will cause significant breakage; just embed a working one. * updated setup.py kerberos extras_require to directly install pykerberos/winkerberos as necessary (and drop requests_kerberos) * updated .coverargerc to exclude vendor subpackage
7b5751c
to
2029019
Compare
Yep, no changes required to the original source since it was already using relative imports. I pasted the license inline instead of a separate file for maximum paranoia, but could be convinced to embed the contents of LICENSE (and other stuff if we want) from the original repo elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that, looks good. I don't have a strong opinion on the LICENSE.
Note: Not sure if you want to do it as part of this PR, but everything else except the version in setup.py should be set for a release. I was planning on 0.4.0 since there were new features added.
extras_require = dict(kerberos=['requests-kerberos>=0.10.0'], credssp=['requests-credssp>=1.0.0']), | ||
extras_require={ | ||
'credssp': ['requests-credssp>=1.0.0'], | ||
'kerberos:sys_platform=="win32"': ['winkerberos>=0.5.0'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://github.com/requests/requests-kerberos/blob/master/setup.py#L52 , we may need to include cryptography>=1.3
as well in this extra section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming in implicitly from requests-ntlm already, but we can add explicitly I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh if it is already defined, that works.
@badcure on the release thing- I usually do a standalone commit/PR for all release activities instead of mixing it in with other stuff. We should probably push a 0.4.0rc1 to GH/PyPI and let it soak for a few days... I can pretty easily do a full Ansible CI run against it once it's on PyPI, which will exercise a lot of the bits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, OK with the way it is now.
Pending upstream changes will cause significant breakage; just embed a working one. See requests/requests-kerberos#133 and requests/requests-kerberos#114 for more details.
Fixes #267