goproxytest: add overlay capability#304
Conversation
| } | ||
| if err := json.Unmarshal(eout.Bytes(), &goEnv); err != nil { | ||
| return fmt.Errorf("failed to unmarshal GOROOT and GOCACHE tags from go command out: %v\n%v", err, eout) | ||
| if err := goenv.Unmarshal(&goEnv); err != nil { |
There was a problem hiding this comment.
FWIW this is a mild regression; you're now asking for all env vars, even though before we only fetched three. You could add names ...string to keep this in place, meaning that if the caller doesn't give any, all are fetched at extra cost.
There was a problem hiding this comment.
Unfortunately that approach won't reduce the number of calls when two calls to goenv.Unmarshal both ask for distinct subsets of the variables. I've changed things so that we explicitly ask for all the env vars we might need - that's OK because this is an internal package and we know all the callers, and none of the variables are costly to calculate.
| We want to be able to use regular Go | ||
| modules as well as test-related ones. | ||
| The contents of this file are ignored; only | ||
| its presence is necessary. |
There was a problem hiding this comment.
wrap this at 80-100 columns as usual, i.e. two lines. no need for four lines AFAICT
| // the server when the test completes. | ||
| // | ||
| // If a file exists in the top level of the [GoModProxyDir] directory | ||
| // named "OVERLAY", then instead of replacing the Go proxy entirely, the |
There was a problem hiding this comment.
Given that GoModProxyDir is .gomodproxy, I find the name OVERLAY a bit inconsistent and unnecessarily loud. Almost like it means to be an env var when it's not. I'd personally just go with overlay. It can't possibly be confused with a modulepath_version name.
There was a problem hiding this comment.
I've gone with _enable_overlay as discussed online
| env.Defer(srv.Close) | ||
| // Add GOPROXY after calling the original setup | ||
| // so that it overrides any GOPROXY set there. | ||
| // Look for local test modules first, falling back to the existing download cache. |
There was a problem hiding this comment.
| // Look for local test modules first, falling back to the existing download cache. | |
| // Look for local test modules first, falling back to the existing module download cache. |
for the sake of consistent naming with the Go docs
| // be relying on modules that match them, but it seems safer | ||
| // to do this. | ||
| "GONOPROXY="+goEnv.GONOPROXY, | ||
| "GOPRIVATE="+goEnv.GOPRIVATE, |
There was a problem hiding this comment.
forwarding GOPRIVATE is unnecessary; go env already shows how it propagates to the others:
$ GOPRIVATE=foobar go env | grep foobar
GONOPROXY='foobar'
GONOSUMDB='foobar'
GOPRIVATE='foobar'
We weren't passing it along before, and we don't need to pass it along for this, so I'd keep omitting it.
rogpeppe
force-pushed
the
051-goproxytest-overlay
branch
2 times, most recently
from
91ab28f to
4dd8d2a
Compare
It's useful to have a hybrid mode where some modules are served from the txtar contents but others come from the regular Go sources. We make goproxytest capable of doing that: adding a file named `OVERLAY` to the top level `.gomodproxy` directory triggers that behavior. Also standardize the log messages produced by goproxytest to make it clear in all cases where the logs are coming from, and remove the "no archive" log because that's more common in overlay mode and doesn't provide much useful information.
rogpeppe
force-pushed
the
051-goproxytest-overlay
branch
from
4dd8d2a to
b37cb59
Compare
2 participants
It's useful to have a hybrid mode where some modules are served from the txtar contents but others come from the regular Go sources. We make goproxytest
capable of doing that: adding a file named
OVERLAYto the top level.gomodproxydirectory triggers that behavior.