Allow HOMEBREW_CURLRC to specify a path for curl --config
#15853
Conversation
There was a problem hiding this comment.
Great work here @clint-stripe! Excuse all the back-and-forth: just the first time we've had a config change like this so want to make sure we get it right first time 😎
Library/Homebrew/env_config.rb
Outdated
| description: "If set to a path, pass it with `--config` when invoking `curl`(1). " \ | ||
| "If set but _not_ a valid path, do not pass `--disable`, which disables the " \ | ||
| "use of `.curlrc`.", |
There was a problem hiding this comment.
This is great. Wondering if this is the best approach or if we want to do something like "if set to 1 or true"? @Homebrew/brew thoughts?
There was a problem hiding this comment.
That's a very fair question! The current handling of boolean: true is #present?, but it might be nice to enforce something stricter in this particular case.
There was a problem hiding this comment.
This is great. Wondering if this is the best approach or if we want to do something like "if set to
1ortrue"? @Homebrew/brew thoughts?
Starting with / is probably sufficient to invoke the path-based logic, given it is unlikely someone does that in order to use it as a boolean and we won't ever support relative paths.
There's potentially also an argument that we could just let CURL_HOME passthrough if HOMEBREW_CURLRC is set rather than the double-purpose, but it maybe doesn't matter much and which is safer is largely situational.
There was a problem hiding this comment.
Starting with
/is probably sufficient to invoke the path-based logic, given it is unlikely someone does that in order to use it as a boolean and we won't ever support relative paths.
Agreed, that works for me!
| it "doesn't return `--config` when HOMEBREW_CURLRC is unset" do | ||
| expect(curl_args(*args)).not_to include(a_string_starting_with("--config=")) | ||
| end | ||
|
|
||
| it "returns `--config` when HOMEBREW_CURLRC is a valid path" do | ||
| Tempfile.create do |tmpfile| | ||
| path = tmpfile.path | ||
| ENV["HOMEBREW_CURLRC"] = path | ||
| # We still expect --disable | ||
| expect(curl_args(*args).first).to eq("--disable") | ||
| expect(curl_args(*args).join(" ")).to include("--config #{path}") | ||
| end | ||
| ensure | ||
| ENV["HOMEBREW_CURLRC"] = nil | ||
| end |
Library/Homebrew/env_config.rb
Outdated
| description: "If set to a path, pass it with `--config` when invoking `curl`(1). " \ | ||
| "If set but _not_ a valid path, do not pass `--disable`, which disables the " \ | ||
| "use of `.curlrc`.", |
There was a problem hiding this comment.
| description: "If set to a path, pass it with `--config` when invoking `curl`(1). " \ | |
| "If set but _not_ a valid path, do not pass `--disable`, which disables the " \ | |
| "use of `.curlrc`.", | |
| description: "If set to a full path (i.e. beginning with `/`, pass it with `--config` when invoking `curl`(1). " \ | |
| "If set but _not_ a valid path, do not pass `--disable`, which disables the " \ | |
| "use of `.curlrc`.", |
note this will fail brew style: you'll need to fix up the strings locally. Once that's done, you've run brew generate-man-completions and (ideally if your git-fu is sufficient) squashed it down to a single commit and CI is 🟢: this is ready to merge! Sorry for all the back and forth.
clint-stripe
force-pushed
the
clint/pass_through_curl_home
branch
from
b06b619
to
a95f690
Compare
clint-stripe
changed the title
--config
|
Awesome, thank you so much for the quick reviews! (Also really appreciate I made that last change you suggested, fixed up the line lengths, and squashed. CI looks green! |
Co-authored-by: Mike McQuaid <[email protected]> Co-authored-by: Carlo Cabrera <[email protected]>
MikeMcQuaid
force-pushed
the
clint/pass_through_curl_home
branch
from
1f64cad
to
4ade935
Compare
There was a problem hiding this comment.
Thanks so much for your first contribution (hopefully of many)! Without people like you submitting PRs we couldn't run this project. You rock, @clint-stripe!
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?We're using a custom
HOMEBREW_ARTIFACT_DOMAINthat requires some additional flags in.curlrc. We would like to setHOMEBREW_CURLRCand the value forCURL_HOMEin/etc/homebrew/brew.envto support this without relying on the user's.curlrc.