-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add goauth
binary for GOAUTH
environment support
#22
base: main
Are you sure you want to change the base?
Conversation
(for safety for the second call, but this should not happen as this requires the location to be missing, which would fail on the first call.)
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.
Really appreciate the contribution. Sorry if I am being a little nit picky here; mostly wanted to share some thoughts from the maintainability perspective.
Happy to discuss over the comments.
|
||
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>'" | ||
|
||
To support multiple locations, add the command multiple times to the GOAUTH variable (semicolon-separated). |
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.
Can we add an example for using this together with a non Artifact Registry repo? e.g.,
To support an Artifact Registry repo together with other repos, add all commands to the GOAUTH variable. For example:
export GOAUTH="GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>; another command"
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.
I am not against it, but your example won't work, see below. Please feel free to do some testing and verification on your side.
|
||
Add to your GOAUTH environment variable: | ||
|
||
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>'" |
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.
I wonder if we can leave out sh -c
. Without it the command should still work but not sure if sh -c
works on Windows?
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.
It won't work on Windows and I doubt sh -c is also needed.
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>'" | |
export GOAUTH="go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>" |
must work allright
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.
It won't work on Windows and I doubt sh -c is also needed.
must work allright
No, it does not. Please feel free to do some testing and verification.
I already checked these scenarios:
export GOAUTH="go run github.com/smartpricer/artifact-registry-go-tools/cmd/goauth@latest europe-west1"
#--> recursive call
export GOAUTH="GOPROXY=direct go run github.com/smartpricer/artifact-registry-go-tools/cmd/goauth@latest europe-west1"
#--> 401, does not work
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/smartpricer/artifact-registry-go-tools/cmd/goauth@latest europe-west1'"
#--> works fine
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.
Regarding the Windows support: Feel free to suggest an example for Windows, after testing 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.
Maybe the instructions must change to ask users to run go install first instead of go run
If you install the binary first like this:
GOPROXY=direct go install github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest
Then you can use the installed binary directly in GOAUTH without having to use GOPROXY
GOAUTH="goauth europe-west1"
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.
Even better would be for this to be distributed with the gcloud SDK like the docker Auth does.
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.
I think if going with @cagataygurturk , it needs to be renamed, e.g. go-auth-google-artifact-registry
or go-auth-gar
or sth. like this.
Besides that, I kind of agree with @owenhaynes , that this should become part of the Google Cloud SDK.
On our side, we solved this easily, because we run a mono repository, so we already have a place to put team-wide tooling.
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.
Even better would be for this to be distributed with the gcloud SDK like the docker Auth does.
I had the same thought - but the original rationale of implementing auth helpers in the same language as the repository format is that the users won't have a dependency on gcloud SDK in their build flow, and some customers do care about this mostly because gcloud is huge.
That being said I think implementing it within gcloud is indeed simpler; I'll go ahead and do that first to hopefully unblock folks. I would like to eventually add support for it in this auth tool, but I think it would require some design review among the team so I'll get back to it later.
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.
FYI after spending a lot of time debugging what's described in golang/go#71889 I finally have made it to work locally. Need some extra time adding tests and stuff and this should be available very soon.
For more details, see https://pkg.go.dev/cmd/go@master#hdr-GOAUTH_environment_variable` | ||
|
||
func main() { | ||
jsonKey := flag.String("json_key", "", "path to the json key of the service account used for this location. Leave empty to use the oauth token instead.") |
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.
I was wondering if we could reuse the same program entry point. The main motivation is to avoid duplicate code (the two flags here), or at least make it harder for future developers to miss one another when adding more features to goauth or netrc.
maybe
export GOAUTH="GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/auth@latest goauth ---location"
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.
I think it's better to have it separate, because the application behavior is dictated by Go and will have to be different from the auth
command behavior: Things like writing and reading from stdin/out and getting an additional commandline argument.
Even if they seem to be similar now, they probably will divert more in the future.
|
||
Add to your GOAUTH environment variable: | ||
|
||
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>'" |
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.
It won't work on Windows and I doubt sh -c is also needed.
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>'" | |
export GOAUTH="go run github.com/GoogleCloudPlatform/artifact-registry-go-tools/cmd/goauth@latest <location>" |
must work allright
fyi, I submitted a change to Go for adding environment variable support: https://go-review.googlesource.com/c/go/+/650055 This should make |
This adds support for the new
GOAUTH
environment variable, introduced by Go 1.24.Note: I have not tested the service account support, but it should work. Please feel free to contribute some testing ;) .
Fixes #21
How I tested (binary)
cmd/goauth
export GOAUTH="/home/felix/Projects/artifact-registry-go-tools/cmd/goauth/goauth europe-west1"
go get -u golang.org/x/net
How I tested (script)
export GOAUTH="sh -c 'GOPROXY=direct go run github.com/smartpricer/artifact-registry-go-tools/cmd/goauth@latest europe-west1'"
(obviously using the fork)go get -u golang.org/x/net