Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

Port away from requests -> urllib3 for thread-safety. Resolves #144. #258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ShaheedHaque
Copy link
Contributor

@ShaheedHaque ShaheedHaque commented Jul 11, 2019

This is not yet ready to merge as I running some integration level tests with our software. The problems I reported in #144 were hard to reproduce, so I need to do a few runs before there is a reasonable chance that I can conclude all is well.

In the meantime, I would appreciate feedback.

It is worth noticing that the fix involves porting to urllib3>=1.25; that is because it is this version which introduces certificate checking as an option.

I also note in passing that there is a Python3.7 compatibility issue which make it hard for me to get a clean run:

/usr/lib/python3.7/ast.py:35: in parse
    return compile(source, filename, mode, PyCF_ONLY_AST)
E     File "/main/srhaque/kdedev/python-consul/tests/test_aio.py", line 77
E       fut = asyncio.async(put(), loop=loop)
E                         ^
E   SyntaxError: invalid syntax

so I have ignored that for now (these also show up as reserved word warning in 3.6).

@ShaheedHaque
Copy link
Contributor Author

After quite a bit of testing, the code seems to be behaving as well as the previous code, with no obvious regressions. The Travis failures seem to be caused by an issue running flake8 with what seems to be a malformed command, though I cannot work out what the problem is. So, plus or minus some help getting that sorted out, I believe the code is ready to merge.

[Sadly, this does not resolve my original problem, but I'll open a new issue for that]

@ShaheedHaque ShaheedHaque force-pushed the srh_make_consul_usage_threadsafe branch 2 times, most recently from 4bcf0c0 to a10cd7b Compare July 12, 2019 09:01
@ShaheedHaque ShaheedHaque force-pushed the srh_make_consul_usage_threadsafe branch from a10cd7b to 856d769 Compare July 12, 2019 09:04
@ShaheedHaque
Copy link
Contributor Author

ShaheedHaque commented Jul 12, 2019

The Travis issue turned out to be a W504 against setup.py which is now disabled. I added Python3.7 because that is what I need (including to test with), and fixed the syntax changes that required too, but Python3.7 does not even get started on Travis (it is not installed, and the install attempt fails).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant