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 the Python 3 incompatibility in crypto utils #478

Merged
merged 3 commits into from Sep 8, 2017

Conversation

Projects
None yet
2 participants
@jeremycline
Member

jeremycline commented Sep 6, 2017

Write the downloaded file as text since the file is opened in text mode.

fixes #476

jeremycline added some commits Sep 6, 2017

Add a base test module
This moves a few commonly defined constants into a base test module and
defines a simple base TestCase class that configures vcrpy.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Refactor the _load_remote_cert code to use requests retries
Requests supports using retries, so we should just use that rather than
trying to re-implement it.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@codecov

This comment has been minimized.

codecov bot commented Sep 6, 2017

Codecov Report

Merging #478 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #478   +/-   ##
========================================
  Coverage    58.82%   58.82%           
========================================
  Files           29       29           
  Lines         1831     1831           
  Branches       303      303           
========================================
  Hits          1077     1077           
  Misses         667      667           
  Partials        87       87
Impacted Files Coverage Δ
fedmsg/crypto/utils.py 58.46% <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 04d94be...8cb2c43. Read the comment docs.

@@ -98,7 +98,7 @@ def validate_policy(topic, signer, routing_policy, nitpicky=False):
return True
def _load_remote_cert(location, cache, cache_expiry, tries=0, **config):
def _load_remote_cert(location, cache, cache_expiry, tries=1, **config):

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 7, 2017

Member

Why this change?

The docblock says

tries (int): The number of times to attempt downloading the certificate.

but it sounds like it's actually the number of re-tries upon failure, based on the name of the parameter being handed to requests. It would be good to clarify which it is in the docblock.

with open(cache, 'w') as f:
f.write(response.content)
except requests.exceptions.ConnectionError:
if tries < 3:

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 7, 2017

Member

It does seem like the default number of tries is being changed in this patch, from 3 to 1? Maybe we should default tries to 3 now?

This comment has been minimized.

@jeremycline

jeremycline Sep 7, 2017

Member

Ah good catch, not sure what I was thinking

cassette_library_dir=os.path.join(FIXTURES_DIR, 'vcr'), record_mode='none')
self.vcr = my_vcr.use_cassette(self.id())
self.vcr.__enter__()
self.addCleanup(self.vcr.__exit__, None, None, None)

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 7, 2017

Member

Someone should make a competing library called beta...

This comment has been minimized.

self.cache_dir = tempfile.mkdtemp()
self.cache_file = os.path.join(self.cache_dir, 'ca.crt')
self.addCleanup(shutil.rmtree, self.cache_dir, True)

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 7, 2017

Member

Whoah I didn't know about addCleanup(). Nice!

Fix the Python 3 incompatibility in crypto utils
Write the downloaded file as text since the file is opened in text mode.

fixes #476

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the jeremycline:certs-encoding branch from a4f068e to 8cb2c43 Sep 7, 2017

@jeremycline jeremycline merged commit 2751845 into fedora-infra:develop Sep 8, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 58.82% (+0%) compared to 04d94be
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremycline jeremycline deleted the jeremycline:certs-encoding branch Sep 8, 2017

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