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

Use pycryptodome, allowing the use of pycrypto on GAE #64

Closed
wants to merge 2 commits into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Sep 1, 2017

@mpdavis, is this method of GAE detection satisfactory?
I'm not an appengine user myself, so not ready to test, but I am interested in #43 getting merged.

Replaces / closes #43

@codecov-io
Copy link

codecov-io commented Sep 1, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   94.53%   94.41%   -0.12%     
==========================================
  Files          12       12              
  Lines         841      841              
==========================================
- Hits          795      794       -1     
- Misses         46       47       +1
Impacted Files Coverage Δ
jose/backends/pycrypto_backend.py 96.15% <0%> (-0.97%) ⬇️

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 4e5cdbb...f1f46bf. Read the comment docs.

@sirosen
Copy link
Contributor Author

sirosen commented Sep 1, 2017

I can mock over the google module and explicitly test this, to make codecov happier, but it seems that this integration is very shouty about unimportant changes in coverage.

@mpdavis, maybe you want to set a threshold in codecov.yaml? The doc for that file says it can be set to a %. Anything that decreases coverage by <1%, for example, should not be marked as failure, IMO.

mattias-lidman and others added 2 commits September 11, 2017 16:06
Continue to push for `pycryptodome` as the default, but add some GAE
detection for intallation requirements. If installing on GAE, should
continue to use pycrypto.

Closes mpdavis#43
@sirosen
Copy link
Contributor Author

sirosen commented Sep 12, 2017

I've just validated that this installs correctly on GAE.
Also, I think this closes #41 ?

EDIT: I'm not actually sure whether or not my GAE validation was sufficient, being a novice in its use. I was able to install in the Google Cloud Shell, which I thought was the same environment as GAE, but I now notice that it actually installed pycryptodome, so maybe not...

@sirosen
Copy link
Contributor Author

sirosen commented Sep 27, 2017

@mpdavis, a coworker just pointed out to me that pycrypto now fails to install on macOS 10.13
That elevates the importance of changing the default crypo lib off of pycrypto.

If this detection isn't satisfactory, I'm happy to swap it out for something else, but not being a GAE user it's hard to be confident in my testing.

@mpdavis
Copy link
Owner

mpdavis commented Sep 27, 2017

That certainly would push us towards getting off of pycrypto as a default.

I can look into testing this and getting it in. We will need some way to handle this.

@mpdavis
Copy link
Owner

mpdavis commented Jan 16, 2018

#57

@mpdavis mpdavis closed this Jan 16, 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.

4 participants