Skip to content
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

KeyError when replaying proxy tunnel captured sessions (broken in #389, v2.0.0) #415

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Dec 22, 2018

This would appear as:

  File "/home/jking/pyvmomi/.eggs/vcrpy-2.0.1-py3.6.egg/vcr/request.py", line 61, in port
    port = {'https': 443, 'http': 80}[parse_uri.scheme]
KeyError: ''

Changes were made in #389 to allow for CONNECT (tunneling), and also OPTIONS (with path '*') would also work using the same logic. In these cases, the vcr.Request class will store the URI of the request as the contents of the path verbatim (not a uri).

When the cassette replays, there was code to parse the port which was throwing an uncaught exception. The code used to expect all uri to begin with either 'https' or 'http'. Now if the uri does not start with those, the port remains None instead of generating a KeyError.

Added a unit test to prove this as well.

This fixes #414

@jeking3
Copy link
Contributor Author

jeking3 commented Dec 22, 2018

Hmm, maybe then it's how pyvmomi is using vcrpy. I'll close this for now since it broke some things.

@jeking3
Copy link
Contributor Author

jeking3 commented Dec 22, 2018

This is ready for consideration. vmware/pyvmomi relies on vcrpy and is currently forced to use 1.x due to the issue introduced in 2.0.0. This resolves that issue.

@jeking3 jeking3 changed the title properly handle tunnel connect URL generation broken in #389 KeyError when replaying proxy tunnel captured sessions (broken in #389, v2.0.0) Dec 22, 2018
@jeking3
Copy link
Contributor Author

jeking3 commented Feb 5, 2019

@kevin1024 this is a regression from 1.x - is there any possibility you could review and merge this? Thanks.

@jeking3
Copy link
Contributor Author

jeking3 commented May 3, 2019

@kevin1024 this is a regression from 1.x - is there any possibility you could review and merge this? Thanks.

@jeking3
Copy link
Contributor Author

jeking3 commented Jun 26, 2019

@kevin1024 this is a regression from 1.x - is there any possibility you could review and merge this? Thanks.

@kevin1024
Copy link
Owner

Thanks for the poke. I wonder why the tests don’t seem to be running anymore.

@jeking3
Copy link
Contributor Author

jeking3 commented Jun 27, 2019

You may need to re-connect Travis CI and AppVeyor.

@jeking3
Copy link
Contributor Author

jeking3 commented Jun 27, 2019

FYI tests did run on this when it was first submitted:

https://travis-ci.org/kevin1024/vcrpy/builds/471426759

@arthurHamon2
Copy link
Collaborator

@jeking3 can you please rebase your branch with master in order to run travis CI ?
I'll be happy to merge your PR then :)

@jeking3
Copy link
Contributor Author

jeking3 commented Jun 28, 2019

All set, passed.

@arthurHamon2 arthurHamon2 merged commit 6be6f02 into kevin1024:master Jun 28, 2019
@jeking3 jeking3 deleted the issue-414 branch June 28, 2019 15:26
@jeking3
Copy link
Contributor Author

jeking3 commented Jun 28, 2019

Next hurdle - I have an upstream change on pyvmomi that depends on this being in a release. It is blocking pyvmomi from moving to v2 of vcrpy. Any idea when the next release will be?

See: vmware/pyvmomi#750

@neozenith
Copy link
Collaborator

@jeking3 follow #447 for discussing the next release. Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

Proxy tunnel connect was broken by #389
4 participants