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

Implement optional backends for python-jose as setup.py extras. #57

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

zejn
Copy link
Collaborator

@zejn zejn commented May 31, 2017

This allows installing running pip install python-jose[cryptography] to install with cryptography. Supported backends libraries are cryptography, pycrypto and cryptodome.

…allows

installing running pip install python-jose[cryptography] to install with
cryptography. Supported backends libraries are cryptography, pycrypto and
cryptodome.
@mpdavis
Copy link
Owner

mpdavis commented May 31, 2017

I like this approach much better.

Just a couple of issues:

  1. It appears to have broken our elliptic curve cryptography backend.
  2. Is there a way to determine a default extra dependency? From a quick search it doesn't appear that there is, which is unfortunate. I am always amazed that the amount of people that don't pin their dependencies.
  3. We will want to update the docs to reflect this change. This doesn't have to happen in this PR, this is mostly just a reminder to myself.

I will be cutting a 2.0.0 after this lands to try to affect as few people as possible.

@zejn
Copy link
Collaborator Author

zejn commented May 31, 2017

  1. Not quite, the breakage comes from cryptography 1.9, which was fixed in PR Fix for just released cryptography 1.9. #56, but this pull request's CI was run before that PR was merged. Not sure how to re-run the CI without another commit.
  2. I'm implementing a backend using pure python rsa module -- https://pypi.python.org/pypi/rsa/3.4.2 -- so you get basic functionality out of the box, albeit at a bit of a performance hit. I have it working mostly, only having issue with supporting appropriate PEM output.
    I'm not sure how one would go about implementing a "default" dependency. This would be possible if python packaging would have a "provides" field (e.g. like an apache2 deb package has "Provides: httpd"), but I'm not sure that even exists in python packaging, especially since the packages are provided by the upstream developers themselves and provides is a more of an ecosystem integration feature.
  3. Indeed.

@codecov-io
Copy link

Codecov Report

Merging #57 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   95.64%   95.51%   -0.14%     
==========================================
  Files          12       12              
  Lines         734      735       +1     
==========================================
  Hits          702      702              
- Misses         32       33       +1
Impacted Files Coverage Δ
jose/backends/pycrypto_backend.py 95.23% <0%> (-1.2%) ⬇️
jose/backends/cryptography_backend.py 100% <0%> (ø) ⬆️

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 7403dff...8931893. Read the comment docs.

@GautamGupta
Copy link

GautamGupta commented Jul 20, 2017

@mpdavis When do you plan to be able to cut a 2.0?

@zejn Is it possible for you to amend your commit and force push for the tests to re-run? edit: I think it's actually complaining on coverage and not failing tests.

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.

4 participants