python-consul icon indicating copy to clipboard operation
python-consul copied to clipboard

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

Open ShaheedHaque opened this issue 6 years ago • 2 comments

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 avatar Jul 11 '19 11:07 ShaheedHaque

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 avatar Jul 11 '19 20:07 ShaheedHaque

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).

ShaheedHaque avatar Jul 12 '19 09:07 ShaheedHaque