-
Notifications
You must be signed in to change notification settings - Fork 589
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
Attempt to clarify use of Golang image, Modules and Mirror #517
Conversation
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.
Thanks for adding this! Just a few small comments. It'd be great to have an example in the directory's cloudbuild.yaml demonstrating it functionally, but if you don't feel like adding it in this PR then I can take a crack at it in a followup.
go/README.md
Outdated
@@ -4,27 +4,70 @@ This builder runs the `go` tool (`go build`, `go test`, etc.) | |||
after placing source in `/workspace` into the `GOPATH` before | |||
running the tool. | |||
|
|||
This functionality is not necessary if you're building using | |||
# Using `Golang` and [Go Modules](https://github.com/golang/go/wiki/Modules) |
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.
Docker image names must be lowercase, so I'd prefer to refer to it as golang
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.
Fixed: f7fbd8a
go/README.md
Outdated
in Go 1.11+, and you can **build with the standard | ||
[`golang`](https://hub.docker.com/_/golang) image on Dockerhub | ||
instead:** | ||
in Go 1.11+. You can **build** with the `golang` image (not a Builder) from [Dockerhub](hub.docker.com/library/golang) and Google's Container Registry [mirror](mirror.gcr.io/library/golang): |
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.
"(not a Builder)" is a bit misleading; any image can be used as a "builder" -- the term is fairly ill-defined -- including the golang
image.
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.
Fixed: f7fbd8a
go/README.md
Outdated
in Go 1.11+, and you can **build with the standard | ||
[`golang`](https://hub.docker.com/_/golang) image on Dockerhub | ||
instead:** | ||
in Go 1.11+. You can **build** with the `golang` image (not a Builder) from [Dockerhub](hub.docker.com/library/golang) and Google's Container Registry [mirror](mirror.gcr.io/library/golang): |
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.
We configure GCB to use the GCR mirror by default, so there's no need to explicitly request the mirrored version. Just referencing golang
will pull through the GCR mirror if available, and not if not.
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.
Interesting. How does this square with: https://issuetracker.google.com/issues/137170318
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.
That I don't know, I'd have to talk to the GCR team to understand more.
In general I think we should not ask users to request from mirror.gcr.io
directly, but instead just configure their Docker installation to use the mirror, or in the case of GCB, just rely on it already configuring that.
env: | ||
- GO111MODULE=on | ||
args: ['go','get','-u','github.com/golang/glog'] | ||
volumes: |
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.
You can now specify build-wide volumes and envs using .options.env
and .options.volumes
, to avoid having to repeat yourself.
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.
IDNKT. Will reflect.
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.
See f7fbd8a
…udPlatform#517) * Attempt to clarify Modules and Mirror * Reflect @imjasonh feedback
@imjasonh -- as discussed via email, a proposal to:
Golang
imageGolang
image's use of/go
and Cloud Build's use of/workspace
GOPROXY=https://proxy.golang.org
mirror.gcr.io/library/golang
-- will file a separate bug about the Golang version omission