Skip to content

Conversation

vinc
Copy link

@vinc vinc commented Mar 14, 2018

This PR adds support of HTTP_PROXY environment variable to the gem like in the JavaScript client: https://github.com/algolia/algoliasearch-client-javascript#proxy-support

If the environment variable is not set, the code behave as usual.

See https://github.com/nahi/httpclient/blob/v2.8.3/sample/thread.rb#L7

@coveralls
Copy link

coveralls commented Mar 14, 2018

Coverage Status

Coverage decreased (-5.05%) to 88.443% when pulling 3e354cc on vinc:feature-http-proxy into deb5046 on algolia:master.

@vinc
Copy link
Author

vinc commented Mar 14, 2018

Any tips on how to test this feature?

@julienbourdeau
Copy link

Good question :D I was about to ask you ^^ I'll think about it, it would be great to add tests.

@vinc
Copy link
Author

vinc commented Mar 14, 2018

I'll see if I can find some inspiration from this: https://github.com/nahi/httpclient/blob/v3.2.8/test/helper.rb#L69

thread_hosts_key = read ? "algolia_search_hosts_#{application_id}" : "algolia_hosts_#{application_id}"
Thread.current[thread_hosts_key] ||= (read ? search_hosts : hosts).each_with_index.map do |host, i|
client = HTTPClient.new
client = HTTPClient.new(ENV["HTTP_PROXY"])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed because the httpclient library already read this variables:

Any chance you could tell us what happens in this load_environment method in your case @vinc ? Maybe you have a REQUEST_METHOD variable forcing the code to look for cgi_http_proxy ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed yesterday our fork seemed to fix the issue we had on our production server, but we switched back to the original gem after reading your comment and everything is still working.

Therefor the issue we were having seems to have been totally unrelated!

So we can close this PR, and I apologize for your time spent on this :3

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem @vinc thanks for contributing!

@raphi raphi closed this Mar 16, 2018
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.

5 participants