Support no_proxy via URI::Generic#find_proxy #670

Merged
merged 4 commits into from Mar 15, 2017

Conversation

Projects
None yet
4 participants
@authorNari
Contributor

authorNari commented Feb 24, 2017

Default proxy value is set via URI::Generic#find_proxy if it's defined.
Maintenance cost is lower using upstream implementation than a implementation is made by myself.

lib/faraday/connection.rb
+ return uri
+ else
+ return nil
+ end

This comment has been minimized.

@olleolleolle

olleolleolle Feb 27, 2017

Contributor

Code note: drop the return keyword on lines 451 and 453 (the if's the last expression of the method).

@olleolleolle

olleolleolle Feb 27, 2017

Contributor

Code note: drop the return keyword on lines 451 and 453 (the if's the last expression of the method).

This comment has been minimized.

@iMacTia

iMacTia Mar 7, 2017

Member

I agree. Moreover, the else branch is completely redundant. so the if can be simplified into:

if uri && !uri.empty?
  uri = 'http://' + uri if uri !~ /^http/i
  uri
end
@iMacTia

iMacTia Mar 7, 2017

Member

I agree. Moreover, the else branch is completely redundant. so the if can be simplified into:

if uri && !uri.empty?
  uri = 'http://' + uri if uri !~ /^http/i
  uri
end

This comment has been minimized.

@authorNari

authorNari Mar 8, 2017

Contributor

I think If the return value of a method is meaningful, return should be explicitly written. It's easy to read :)

But I think that the consistency of this project should be respected.
I deleted return dd13327

@authorNari

authorNari Mar 8, 2017

Contributor

I think If the return value of a method is meaningful, return should be explicitly written. It's easy to read :)

But I think that the consistency of this project should be respected.
I deleted return dd13327

@olleolleolle

I'm a big fan of this change.

@iMacTia

This comment has been minimized.

Show comment
Hide comment
@iMacTia

iMacTia Mar 3, 2017

Member

Hi @authorNari,

I can see many tests have been deleted in your PR.
Can I ask you why?

Member

iMacTia commented Mar 3, 2017

Hi @authorNari,

I can see many tests have been deleted in your PR.
Can I ask you why?

@authorNari

This comment has been minimized.

Show comment
Hide comment
@authorNari

authorNari Mar 6, 2017

Contributor

Hi @iMacTia

511a5b4 is reverted on this pull request so these tests are deleted.
These tests are depends on proxy_allowed? method.

Contributor

authorNari commented Mar 6, 2017

Hi @iMacTia

511a5b4 is reverted on this pull request so these tests are deleted.
These tests are depends on proxy_allowed? method.

@iMacTia

I see now, you've found another implementation to support no_proxy using a Ruby method!
Well, I totally agree we shouldn't reinvent the wheel and if we can use that method then I'm totally happy with it.
I just added a couple of comments to get a better picture.

- conn = Faraday::Connection.new
- assert_equal conn.proxy_allowed?('http://example.com'), false
- end
- end

This comment has been minimized.

@iMacTia

iMacTia Mar 7, 2017

Member

Please reintroduce all the deleted test as they're covering some important edge-cases.
This can be done without using the proxy_allowed? method:

  • If the test asserts assert_equal conn.proxy_allowed?('http://example.com'), false, then you can change it into assert_nil conn.proxy
  • If the test asserts assert_equal conn.proxy_allowed?('http://prefixedexample.com'), true than you can change it into assert_equal 'proxy.com', conn.proxy.host
@iMacTia

iMacTia Mar 7, 2017

Member

Please reintroduce all the deleted test as they're covering some important edge-cases.
This can be done without using the proxy_allowed? method:

  • If the test asserts assert_equal conn.proxy_allowed?('http://example.com'), false, then you can change it into assert_nil conn.proxy
  • If the test asserts assert_equal conn.proxy_allowed?('http://prefixedexample.com'), true than you can change it into assert_equal 'proxy.com', conn.proxy.host

This comment has been minimized.

@authorNari

authorNari Mar 8, 2017

Contributor

That make sense. done bbd89c1

@authorNari

authorNari Mar 8, 2017

Contributor

That make sense. done bbd89c1

- uri = 'http://' + uri unless uri.downcase.start_with?('http')
- uri
+ uri = nil
+ if URI.parse("").respond_to?(:find_proxy)

This comment has been minimized.

@iMacTia

iMacTia Mar 7, 2017

Member

According to the documentation, find_proxy is supported in Ruby 1.9.3 (https://ruby-doc.org/stdlib-1.9.3/libdoc/open-uri/rdoc/URI/Generic.html). Since we dropped the support for older ruby version, I think this method should always be available? Do you get any failing test if you remove this check?

@iMacTia

iMacTia Mar 7, 2017

Member

According to the documentation, find_proxy is supported in Ruby 1.9.3 (https://ruby-doc.org/stdlib-1.9.3/libdoc/open-uri/rdoc/URI/Generic.html). Since we dropped the support for older ruby version, I think this method should always be available? Do you get any failing test if you remove this check?

This comment has been minimized.

@authorNari

authorNari Mar 8, 2017

Contributor

Since we dropped the support for older ruby version, I think this method should always be available?

open-uri lib has find_proxy in >= Ruby 1.8.7. This method move to uri/generic lib in Ruby 2.0.0.
I think any library shouldn't require open-uri because this library overwrite Kernel#.open.
So this pull request doesn't support no_proxy in Ruby 1.9.3. 😢

@authorNari

authorNari Mar 8, 2017

Contributor

Since we dropped the support for older ruby version, I think this method should always be available?

open-uri lib has find_proxy in >= Ruby 1.8.7. This method move to uri/generic lib in Ruby 2.0.0.
I think any library shouldn't require open-uri because this library overwrite Kernel#.open.
So this pull request doesn't support no_proxy in Ruby 1.9.3. 😢

This comment has been minimized.

@iMacTia

iMacTia Mar 15, 2017

Member

Thanks for the detailed response @authorNari that makes totally sense.
I have nothing against this as the plan is to move away from Ruby1.9.3 as soon as possible, so this can be a good incentive!

@iMacTia

iMacTia Mar 15, 2017

Member

Thanks for the detailed response @authorNari that makes totally sense.
I have nothing against this as the plan is to move away from Ruby1.9.3 as soon as possible, so this can be a good incentive!

@iMacTia

This comment has been minimized.

Show comment
Hide comment
@iMacTia

iMacTia Mar 15, 2017

Member

Thank you @authorNari, outstanding work!
I'm grateful we could delegate to Ruby the management of the proxy/no_proxy ENV variables check.
Apologies if it took me some time to review and thanks again for the valuable contribution.

Member

iMacTia commented Mar 15, 2017

Thank you @authorNari, outstanding work!
I'm grateful we could delegate to Ruby the management of the proxy/no_proxy ENV variables check.
Apologies if it took me some time to review and thanks again for the valuable contribution.

@iMacTia iMacTia merged commit 3268aca into lostisland:master Mar 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iMacTia iMacTia added this to the v0.12.0 milestone Mar 15, 2017

@iMacTia iMacTia referenced this pull request Mar 15, 2017

Merged

Implement no_proxy feature #658

@@ -464,5 +442,14 @@ def set_authorization_header(header_type, *args)
header(*args)
headers[Faraday::Request::Authorization::KEY] = header
end
+
+ def find_default_proxy
+ warn 'no_proxy is unsupported' if ENV['no_proxy'] || ENV['NO_PROXY']

This comment has been minimized.

@somethingnew2-0

somethingnew2-0 Mar 15, 2017

Contributor

So no_proxy is unsupported when the url is not given upfront in the initializer, but later on in the request?

@somethingnew2-0

somethingnew2-0 Mar 15, 2017

Contributor

So no_proxy is unsupported when the url is not given upfront in the initializer, but later on in the request?

This comment has been minimized.

@iMacTia

iMacTia Mar 16, 2017

Member

You mean when you initialise the connection simply as conn = Faraday.new ?
That is actually true, I missed this detail as it's an unusual way to use it.
This could be easily fixed moving the #find_proxy call on the run_request or build_request methods.

@iMacTia

iMacTia Mar 16, 2017

Member

You mean when you initialise the connection simply as conn = Faraday.new ?
That is actually true, I missed this detail as it's an unusual way to use it.
This could be easily fixed moving the #find_proxy call on the run_request or build_request methods.

This comment has been minimized.

@somethingnew2-0

somethingnew2-0 Mar 22, 2017

Contributor

Yes, I agree it is a weird way to use it, but it is allowed. The find_proxy method should be moved to when the request is created in build_request.

@somethingnew2-0

somethingnew2-0 Mar 22, 2017

Contributor

Yes, I agree it is a weird way to use it, but it is allowed. The find_proxy method should be moved to when the request is created in build_request.

This comment has been minimized.

@iMacTia

iMacTia Mar 22, 2017

Member

Totally agree with you, I'll try to squeeze this small fix in release 0.12.0 👍
Otherwise will be good for 0.12.1 😄

@iMacTia

iMacTia Mar 22, 2017

Member

Totally agree with you, I'll try to squeeze this small fix in release 0.12.0 👍
Otherwise will be good for 0.12.1 😄

@authorNari authorNari deleted the authorNari:no_proxy_with_ruby2 branch Mar 17, 2017

tpatzig added a commit to sap-oc/cct that referenced this pull request Apr 12, 2017

@tpatzig tpatzig referenced this pull request in SUSE-Cloud/cct Apr 12, 2017

Open

increase faraday to 0.12.0.1 to support no_proxy #76

tpatzig added a commit to sap-oc/cct that referenced this pull request Apr 12, 2017

@tpatzig tpatzig referenced this pull request in SUSE-Cloud/cct Apr 12, 2017

Open

increase faraday to 0.12.0.1 to support no_proxy #77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment