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.

Sign up for GitHub

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

Jump to bottom

Adding proxy variables to FILTERED_ENV #3501

Merged
merged 4 commits into from
Nov 29, 2017
Merged

Adding proxy variables to FILTERED_ENV #3501

merged 4 commits into from
Nov 29, 2017

Conversation

barreyra
Copy link
Contributor

Fixes #3500

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

MikeMcQuaid
MikeMcQuaid reviewed Nov 29, 2017
View reviewed changes
bin/brew Outdated
@@ -89,6 +89,7 @@ then

FILTERED_ENV=()
for VAR in HOME SHELL PATH TERM LOGNAME USER CI TRAVIS http_proxy \
https_proxy ftp_proxy no_proxy \
Copy link
Member

Choose a reason for hiding this comment

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

What is no_proxy used for? Can you move http_proxy from the previous line, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no_proxy lets you list internal domains that should not use the proxy server. Patch updated.

Copy link
Member

Choose a reason for hiding this comment

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

Why would you use this with Homebrew?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if somebody had some private casks in their intranet, for instance?

Copy link
Member

Choose a reason for hiding this comment

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

@barreyra Which of these variables do you personally need for Homebrew to function? I don't want to whitelist everything that could be useful but just things that individuals need.

Copy link
Member

Choose a reason for hiding this comment

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

@barreyra I'd suggest just going for HTTPS_PROXY (#3500) and, if you personally use it, FTP_PROXY for now (both uppercase). If you also use ALL_PROXY or NO_PROXY they can be added too (but only if you personally need them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So not setting the http proxy?

What's the difference between the lower and uppercase versions of the variables? I've always used the lowercase versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answering my own question, from the curl manpage:

The environment variables can be specified in lower case or upper case. The lower case version has precedence. http_proxy is an exception as it is only available in lower case.

Why do you prefer the uppercase version?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you prefer the uppercase version?

That's the version that's documented in the man page if you're copy-pasting.

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'm fine going with this for now. I had added no_proxy because it was one of the asks in #3500

@MikeMcQuaid MikeMcQuaid merged commit 6cf38d0 into Homebrew:master Nov 29, 2017
@MikeMcQuaid
Copy link
Member

Thanks for your first contribution to Homebrew, @barreyra! Without people like you submitting PRs we couldn't run this project. You rock!

@barreyra
Copy link
Contributor Author

Thanks, but it's not my first commit, I added libbluray support to ffmpeg back in the day :)

Homebrew/homebrew-core@e451b7f
Homebrew/homebrew-core@d12c637

@MikeMcQuaid
Copy link
Member

First commit to Homebrew/brew then 😁

@Quesar Quesar mentioned this pull request Dec 18, 2017
Merged
5 tasks
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@barreyra @MikeMcQuaid