Update urllib3 handlers

[vEpiphyte]

update urllib3 handlers for urllib3/urllib3#1805 to close #688

This does have a problem of "what version of the urllib3 library is installed ?" since vcrpy does not have minimum version enforcement, so the PR as-is would not work with urllib3 < 1.25.9

[vEpiphyte]

I have not run the unit tests for this change.

[deronnax]

I did and your patch does not work, you forgot a couple of place. You can run the test suite with simply installing tox and then you can run the test suite in an single env (you can list them with tox --listenvs). E.g. tox -e py310-requests.

[hartwork]

so the PR as-is would not work with urllib3 < 1.25.9

@vEpiphyte given #688 (comment) I'm more optimistic about that, but it I think we need a version that supports both urllib3 1.x and 2.x.

If I'm reading CI logs right, we'll need a dedicated pull request to fix tox in here before this to have a chance to be green? Ongoing work at #690 now…

[hartwork]

I have a green CI now at #690 but it should be noted that some of the tests calling out to httpbin.org are flakey in the sense that the site sometimes returns status 502 and HTML output rather than JSON. I think midterm the tests should probably move away from talking to httpbin.org.

For now I think a good plan would be:

• merge Quick-fix the CI for recent tox and recent urllib3 #690 for a start
• rebase Update urllib3 handlers #689 here onto latest master after
• multiply everything in tox.ini that currently says "urllib3" times two:
  • "urllib3-1" with urllib3<2
  • "urllib3-2" with urllib3>=2

@vEpiphyte How much of that would you want to do yourself in here?
I could also try it out myself in a new pull request with your commit included via cherry-pick.

Are there other ideas?

[alisakr]

@vEpiphyte @hartwork The solution, which works for urllib3 >=2 AND urllib3<2, is to make the same change here to two other files, https://github.com/kevin1024/vcrpy/blob/master/vcr/stubs/boto3_stubs.py and https://github.com/kevin1024/vcrpy/blob/master/vcr/stubs/urllib3_stubs.py

[hartwork]

@alisakr thanks for the feedback! I'll wait for a reply from @vEpiphyte a bit more so we can coordinate a good way forward.

[alisakr]

@hartwork No problem! In the mean time, I see you landed your pull request to temporarily limit the urllib3 max version. #690 Should this be released now?
Last release was august https://github.com/kevin1024/vcrpy/releases

[hartwork]

@alisakr I don't have PyPI write permissions unfortunately and the quick-fix only helped CI, not users. Let's get both 1.x and 2.x supported and then ask someone with PyPI access to do a release right after. What do you think?

[therve]

I had a look and outside of the imports (the fix is unfortunately much more complex than what's done in the PR), there is also at least one change in the response breaking vcrpy: urllib3/urllib3#2649. If you want to support both urllib3 versions that might end up be messy,

[hartwork]

@therve thanks for the pointer! I think if we had a pull request that was complete in terms of support for urllib3 >=2, that would greatly help evaluating with the cost of supporting both at the same time. I imagine with some luck we could just look at it it, and "see" the answer.

[vEpiphyte]

Hi Everyone - I'm sorry for not giving more feedback on this. My immediate fire was put out by restricting requests (and avoiding a transitive urllib3 dependency change ), so I haven't had the time to get back to this.

I think we ( as vcrpy users / collaborators / maintainers / interested parties ) should make a decision regarded supported versions of the third party libraries like requests / boto3 ( see #693 ), since that would inform the correct way to fix the patching that is going on. I would like to say "let us just support the un-vendored urllib3 library which changed here urllib3/urllib3/pull/1805 " but I don't want to make that call in a vacuum simply because I was the one who put up a PR.

I believe that would make it much easier to maintain vcrpy moving forward and can be clearly documented. A developer who has to use a older version of one of those libraries which relies on a vendored or older version of urllib3 can always use a older version of vcrpy.

[jairhenrique]

@vEpiphyte I agree with you. The ecosystem needs to evolve and maintaining deprecated libraries in new versions is a difficult job that is often not necessary. Software updates are healthy software!

Can you help us by removing support for old versions?

[pquentin]

@jairhenrique Thanks! Do you agree that the minimum supported version should be those from #693?

One difficulty is that unlike other libraries we can’t tell pip about those supported versions, so what should we do if an older version of requests is actually in use? It seems to me that raising an exception like a RuntimeError telling users to downgrade vcrpy is the right thing to do.

[hartwork]

It seems to me that raising an exception like a RuntimeError telling users to downgrade vcrpy is the right thing to do.

Please take my vote for recommending an upgrade to the other side or to least mention both options.

[jairhenrique]

@jairhenrique Thanks! Do you agree that the minimum supported version should be those from #693?

One difficulty is that unlike other libraries we can’t tell pip about those supported versions, so what should we do if an older version of requests is actually in use? It seems to me that raising an exception like a RuntimeError telling users to downgrade vcrpy is the right thing to do.

Yes.

Downgrade vcr or upgrade project libraries.

[therve]

@therve thanks for the pointer! I think if we had a pull request that was complete in terms of support for urllib3 >=2, that would greatly help evaluating with the cost of supporting both at the same time. I imagine with some luck we could just look at it it, and "see" the answer.

Unfortunately I couldn't get something working. I'm somewhat getting lost in the patching about which method should return which object. Code is here if you're curious.

[vEpiphyte]

It sounds the consensus is "support the raw urllib3 library with the changes from 1.25.9" should be the way forward?

[shifqu]

I have created a PR (#697) which should provide backward compatibility as well as support urllib3>2.

[vEpiphyte]

Closing this in favor of #698 which is a much cleaner approach than what I had shotgunned together. Good discussion here though :)