Skip to content
This repository was 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
ShaheedHaque wants to merge 3 commits intopython-consul:masterfrom
ShaheedHaque:srh_make_consul_usage_threadsafe
Open

Port away from requests -> urllib3 for thread-safety. Resolves #144.#258
ShaheedHaque wants to merge 3 commits intopython-consul:masterfrom
ShaheedHaque:srh_make_consul_usage_threadsafe

Conversation

@ShaheedHaque
Copy link
Contributor

@ShaheedHaque ShaheedHaque commented Jul 11, 2019 •
edited
Loading

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

ShaheedHaque commented Jul 11, 2019

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 mentioned this pull request Jul 11, 2019
Closed
@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 •
edited
Loading

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 ShaheedHaque mentioned this pull request Nov 29, 2019
Closed
16 tasks
@cablehead cablehead force-pushed the master branch 2 times, most recently from 636f367 to a91daae Compare April 15, 2024 01:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ShaheedHaque