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

fix #664 #665

Merged
merged 4 commits into from Jul 19, 2017

Conversation

Projects
None yet
2 participants
@reaperhulk
Member

reaperhulk commented Jul 19, 2017

No description provided.

reaperhulk added some commits Jul 19, 2017

fix #664
bytes and strings are different things.
@alex

This comment has been minimized.

Member

alex commented Jul 19, 2017

a) Why don't the tests catch this?
b) If these values are going to be unicode, please make _CRYPTOGRAPHY_MANYLINUX1_CA_DIR unicode.

@alex alex added this to the 17.2.0 milestone Jul 19, 2017

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Jul 19, 2017

Tests didn't catch this because to have it take the fallback path we fetched the values from cffi and then monkeypatched our constants (https://github.com/pyca/pyopenssl/blob/master/tests/test_ssl.py#L1114-L1142). So the comparison succeed since they were both bytes.

I think a better fix here is just to declare these as byte strings and not unicode. They're not really paths to us, just sentinel values.

@alex

This comment has been minimized.

Member

alex commented Jul 19, 2017

That's also fine with me.

@codecov

This comment has been minimized.

codecov bot commented Jul 19, 2017

Codecov Report

Merging #665 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #665   +/-   ##
=======================================
  Coverage   96.94%   96.94%           
=======================================
  Files          18       18           
  Lines        5737     5737           
  Branches      401      401           
=======================================
  Hits         5562     5562           
  Misses        117      117           
  Partials       58       58
Impacted Files Coverage Δ
src/OpenSSL/SSL.py 94.88% <100%> (ø) ⬆️

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 7f5610c...4041f61. Read the comment docs.

@alex

This comment has been minimized.

Member

alex commented Jul 19, 2017

trailing whitespace makes the flake8 fail

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Jul 19, 2017

Why can't GitHub's editor strip trailing whitespace like my vim config does.

@alex

This comment has been minimized.

Member

alex commented Jul 19, 2017

Send a feature request!

@alex

alex approved these changes Jul 19, 2017

@alex alex merged commit a92a1a7 into master Jul 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alex alex deleted the reaperhulk-patch-1 branch Jul 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment