-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add HTTPS Proxy support #47
Add HTTPS Proxy support #47
Conversation
net-http version 0.5.0 finally introduced TLS proxy support: ruby/net-http#55 https://github.com/ruby/net-http/releases/tag/v0.5.0
a0b45fd
to
c8bfc3c
Compare
@richardmarbach Cool feature! To support this, perhaps we can make this gem depend on net-http version 0.5.0+? |
SSL proxies only supported in net-http 0.5.0 or above
c5e58e3
to
8612fa7
Compare
Hey @olleolleolle, thanks for checking so quickly! I've added the minimum runtime dependency for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @richardmarbach!
@@ -25,5 +25,5 @@ Gem::Specification.new do |spec| | |||
spec.files = Dir.glob('lib/**/*') + %w[README.md LICENSE.md] | |||
spec.require_paths = ['lib'] | |||
|
|||
spec.add_runtime_dependency 'net-http' | |||
spec.add_runtime_dependency 'net-http', '>= 0.5.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version limitation creates an issue when gems are installed at runtime. The standard version is lower than the required one, leading to two different versions being loaded simultaneously. This causes the following error:
Unable to activate faraday-net_http-3.4.0, because net-http-0.4.1 conflicts with net-http (>= 0.5.0) (Gem::ConflictError)
The workaround was to keep using the previous version of faraday-net-http
.
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olleolleolle this is probably due to the add_runtime_dependency
here.
I believe we have 2 options to address this:
- Switch to
add_dependency
- Remove the version dependency and update the code to support both
The reason we didn't explicitly depend on net-http
was that this would cause compatibility issues with the "stdlib" Net::HTTP
bundled with Ruby. However, I believe this was removed in Ruby 3.0 and since we support only Ruby 3.0+ it should be safe to make this dependency more concrete now 👍
Would appreciate if you could double-check my claim here, I'm mostly going by memory and a quick Google Search 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also what we state in the gemspec
, so I'd go with option 1 unless I'm missing something important?
net-http version 0.5.0 finally introduced TLS proxy support: ruby/net-http#55
https://github.com/ruby/net-http/releases/tag/v0.5.0
Since this change is breaking for older versions of net-http, I can also implement this in a backward compatible way.