-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support for goproxy_server and go.env files #12747
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
Conversation
fffd129
to
5b8ebd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM, just optional nitpicks
if go_env | ||
T.must(go_env) | ||
File.write(T.must(go_env).name, T.must(go_env).content) | ||
go_env_path = Pathname.new(T.must(go_env).name).realpath.to_s | ||
ENV["GOENV"] = go_env_path | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory you could always set GOENV
to the repo without checking the file exists. The Go CLI will not error if it doesn't find a go.env there. Tried it locally seems to be ok with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think I prefer safeguarding since it's cheap, just in case the behavior ever changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This actually needs a bit more work, there are some places where we're explicitly passing in a |
0cd2a3e
to
cd07532
Compare
go_modules/spec/dependabot/go_modules/file_updater/go_mod_updater_spec.rb
Show resolved
Hide resolved
adb6f9c
to
a308639
Compare
a308639
to
4e5520e
Compare
def set_goproxy_variable | ||
return if go_env&.content&.include?("GOPROXY") | ||
|
||
goproxy_credentials = credentials.select { |cred| cred["type"] == "goproxy_server" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do these goproxy_server
credentials come from? In the go tests I see git_source
credentials being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would come in just like other credentials
goprivate testingI tested your branch against a test repo containing a vitess dependency using two different go.env configurations to validate the environment variable handling works as expected. I built a Docker image from this branch and ran the tests against this custom image to ensure the go.env functionality was properly integrated. When I set GOPRIVATE="*" in the go.env file, Dependabot behaved exactly as intended - it bypassed the Go package manager proxy and fetched directly from the GitHub source repository. This direct fetching naturally excluded the problematic incompatible versions that were causing issues in the original vitess problem. |
GOPROXY testingI successfully created a GO private registry through Jfrog. And used it as proxy here |
Hmm, yeah I think that is problematic because we inject |
@jurre i think the best solution here is probably Only inject GOPRIVATE when GOPROXY is not present, and do nothing when goproxy is present. that reduces complexity on customers about always setting goprivate |
I think that's right, let me run through the logic and think about potential edge cases for a bit tomorrow though |
16a37c9
to
c19a2da
Compare
c19a2da
to
fdd8331
Compare
In order for customers to leverage go proxy servers and/or to configure environment variables used by go to determine how to resolve dependencies, we'll now do two things: - If a `go.env` file is present at the root of the repository, we'll pull it in and tell the Go toolchain to use it. - If a credential for a goproxy_server is passed in, and no `go.env` exists, we export the URLs from the credentials as `GOPROXY`, allowing the toolchain to use it. By default we also append `direct` so that direct access to dependencies is still possible. This should give customers all the flexibility they need to configure how go mod accesses dependencies for their projects, with nice defaults. The approach I've taken here is to export these values at the file fetching step, from here on the toolchain should use them. Since every job runs in its own container, once the job finished the environment is wiped when done.
Co-authored-by: Jake Coffman <[email protected]>
This makes it easier to configure things correctly once, since the local variables would otherwise override what is configured via go.env
fdd8331
to
d6101c6
Compare
By default dependabot will set `GOPRIVATE=*`, which will cause none of the dependencies to go through the proxy that was configured. When `go.env` configures this value it is still respected. Co-authored-by: alhss <[email protected]>
d6101c6
to
ae9c361
Compare
Seeing a failure in CI now that seems not related to the changes here and I can reproduce on main, but taking a look at it nonetheless |
It was a flaky test |
What are you trying to accomplish?
In order for customers to leverage go proxy servers and/or to configure environment variables used by go to determine how to resolve dependencies, we'll now do two things:
go.env
file is present at the root of the repository, we'll pull it in and tell the Go toolchain to use it.go.env
exists, we export the URLs from the credentials asGOPROXY
, allowing the toolchain to use it. By default we also appenddirect
so that direct access to dependencies is still possible.This should give customers all the flexibility they need to configure how go mod accesses dependencies for their projects, with nice defaults.
Anything you want to highlight for special attention from reviewers?
The approach I've taken here is to export these values at the file fetching step, from here on the toolchain should use them.
Since every job runs in its own container, once the job finished the environment is wiped when done.
How will you know you've accomplished your goal?
Tests and manual verification
Checklist