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

fixed response status check when making a request with a valid proxy … #709

Merged
merged 1 commit into from Nov 21, 2019

Conversation

jasquat
Copy link
Contributor

@jasquat jasquat commented Nov 21, 2019

All calls raise Excon::Errors::ProxyConnectionError when making a call with http_proxy or https_proxy set to a valid address in the environment.

It looks like the issue is that status is a subkey of response and not at the root of the hash:

{:expects=>200, :method=>"CONNECT", :response=>{:body=>"", :cookies=>[], :host=>nil, :headers=>{}, :path=>nil, :port=>nil, :status=>200, :status_line=>"HTTP/1.1 200 Connection established\r\n", :reason_phrase=>"Connection established", :remote_ip=>"[IP]", :local_port=>[PORT], :local_address=>"[IP]"}}

@geemus geemus merged commit e89bbb7 into excon:master Nov 21, 2019
1 check passed
@geemus
Copy link
Contributor

geemus commented Nov 21, 2019

Thanks for catching that and the quick fix.

@geemus
Copy link
Contributor

geemus commented Nov 21, 2019

Released in 0.69.1.

@jasquat
Copy link
Contributor Author

jasquat commented Nov 22, 2019

Thanks for catching that and the quick fix.

No problem. Thanks for the quick merge.

@geemus
Copy link
Contributor

geemus commented Nov 22, 2019

I really wish we had some better testing around this to catch these kinds of issues. But in my attempts in the past I never had much luck emulating this behavior in the test stack, and don't personally use proxies enough to feel confident about manually testing (and manual testing is brittle). If you have any thoughts/advise for how we could catch this better and/or avoid regressions I would definitely welcome the assistance.

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

Successfully merging this pull request may close these issues.

None yet

2 participants