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

Expanded support for proxy config from environment variables #437

Merged
merged 4 commits into from
Apr 5, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Mar 7, 2018

Expands setting of config.proxy options to both ENV['http_proxy'] and ENV['https_proxy'] automatically.

@Cawllec Cawllec requested a review from kattrali March 7, 2018 16:08
@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 7, 2018

Docs PR incoming for this as well.

@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 29, 2018

@kattrali can I get a review on this for merging?

@Cawllec Cawllec requested review from a team and removed request for kattrali April 4, 2018 15:17
##
# Parses and sets proxy from a uri
def parse_proxy(uri)
proxy = URI.parse(uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any problems if uri was nil at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, however this would only occur if parse_proxy was called with nil, in which case URI would spit out a legible error i.e. **URI::InvalidURIError: bad URI(is not URI?):**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could catch and log that error, but I think failing loudly is more valuable in most cases.

@kattrali kattrali merged commit 1b9448e into master Apr 5, 2018
@kattrali kattrali deleted the cawllec/https-http-proxy-vars branch April 5, 2018 01:00
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.

3 participants