Skip to content
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

Adds Git #345

Closed
wants to merge 3 commits into from
Closed

Adds Git #345

wants to merge 3 commits into from

Conversation

Starttoaster
Copy link

It appears the upstream alpine image stopped baking git in their image which is essential for go get. This PR simply re-adds functionality that was lost from upstream alpine.

@tianon
Copy link
Member

tianon commented Oct 27, 2020

I'm not sure where you're seeing that Alpine used to include git by default? Our golang:alpine image has never included git, and that's intentional (#250 (comment); #119, #140, #153, #157, #166, #198, #209, #230, #239, #241, #251, #328).

@Starttoaster
Copy link
Author

I'm not sure where you're seeing that Alpine used to include git by default? Our golang:alpine image has never included git, and that's intentional (#250 (comment); #119, #140, #153, #157, #166, #198, #209, #230, #239, #241, #251, #328).

Huh, interesting. We use golang:alpine in a lot of our CI jobs and we only very recently began noticing them failing due to git not being installed. Using the exact same CI templates from other projects that also were not installing git, their go gets were succeeding. But I guess you would be an authoritative source to say I'm imagining that. 😄

@tianon
Copy link
Member

tianon commented Oct 27, 2020

Are the working projects using go.mod? I believe it has the ability to download specific versions from proxy.golang.org without git installed.

@klmitch
Copy link

klmitch commented Oct 27, 2020

Are the working projects using go.mod? I believe it has the ability to download specific versions from proxy.golang.org without git installed.

Yes, they are; we're exclusively using go.mod-style code. One of our failing CI jobs is actually executing just go vet and is encountering the following error:

go vet -mod=readonly ./...
go: gitlab.com/tmobile/cdp/libraries/cdp-error@v0.1.0: git init --bare in /go/pkg/mod/cache/vcs/5284ea28f371dd764ab19f2614118ef1a885457d03dbd84605c2c2d10e430d11: exec: "git": executable file not found in $PATH

This is pulling a local dependency, obviously, so that may explain why we're seeing this.

@tianon
Copy link
Member

tianon commented Oct 27, 2020

Yeah, that sounds like it's probably the case! For running go vet, you probably want to use non-Alpine images generally, but in this specific case I think you can run your own instance of proxy.golang.org which could then include your internal dependencies? (Haven't looking into the details of that myself)

@Starttoaster
Copy link
Author

Are the working projects using go.mod? I believe it has the ability to download specific versions from proxy.golang.org without git installed.

That they are. Hm, I'll have to play with this a bit to confirm. But this still strikes me as a basic functionality add of the Go language. I mean, if the argument is that we can just install git in our own 2 liner Dockerfile that FROM's this image because adding git is undue burden from this project's side of things (or from inside the CI job), then couldn't the argument be made that installing Go is also undue burden? 😄 Not trying to come across as crass, just trying to understand the philosophy there.

@Starttoaster
Copy link
Author

Also, sorry for creating a dupe PR 😄

@klmitch
Copy link

klmitch commented Oct 27, 2020

We tend to prefer using alpine-based images for our CI, because they are smaller, and thus load faster. We've not run into this particular issue before because we don't have very much go code within the company yet, but the image not having a known dependency of go definitely violated the principle of least surprise for me. We have ways of working around this internally--basically, adding apk add git--, but that workaround is also going to slow down our CI a little. We could, I suppose, build our own variant of the upstream golang image, but we really don't want to try to maintain such images ourselves, which is why we haven't done that and indeed have resisted requests to do that from our customers. (It might be worth mentioning that we're from the CI team of a very large company...)

@tianon
Copy link
Member

tianon commented Oct 27, 2020

The point is not "undue burden" but rather "unnecessary size" (which then cannot be reclaimed by image users).

I think #250 (comment) phrases this better than I could rephrase again. 😅

(That note about Alpine being entirely unsupported/unmaintained in the Go ecosystem has also since been documented in our image description via docker-library/docs#1749)

@klmitch
Copy link

klmitch commented Oct 27, 2020

The point is not "undue burden" but rather "unnecessary size" (which then cannot be reclaimed by image users).

True; what this means is that you prioritize what is necessary, which you have done. I'm simply arguing that git is necessary in some cases :)

(That note about Alpine being entirely unsupported/unmaintained in the Go ecosystem has also since been documented in our image description via docker-library/docs#1749)

Interesting. I do have to point out that that comment is not accurate; the alpine image does indeed include both bash and gcc; it's just missing git.

@Starttoaster
Copy link
Author

Starttoaster commented Oct 27, 2020

The point is not "undue burden" but rather "unnecessary size" (which then cannot be reclaimed by image users).

I think #250 (comment) phrases this better than I could rephrase again. sweat_smile

(That note about Alpine being entirely unsupported/unmaintained in the Go ecosystem has also since been documented in our image description via docker-library/docs#1749)

That's fair.. FTR, I still think this is a pretty simple value add, even if it adds a few MB to the image. But I will defer to your opinion that since git may not, under every circumstance, be necessary (even if IMO common) then it should not be added.

@Starttoaster
Copy link
Author

@tianon just wanted to revist this PR since I see it's still open. Wanted to make the point that for the debian variants of this image, the template in this repo is specifying FROM buildpack-deps:{{ env.variant }}-scm. The scm suffix to that tag implies that it's recognized that git is a basic and assumed addition to any Docker image containing Go.

Looking at the documentation for that buildpack-deps image: https://hub.docker.com/_/buildpack-deps

This variant is based on curl, but also adds various source control management tools. As of this writing, the current list of included tools is bzr, git, hg, and svn.

Pulling some sample image tags to my local, I can see that golang:1 (debian based) is 839MB whereas golang:1-alpine is 299MB. This rises to 317MB if you add git, but it is still much more lightweight than the debian variants. Noting that your debian variants include git, as well as several other SCM tools like svn, I'm not sure why the position is still being held that git should not be in the Alpine based golang image? Seems to me like your images set the precedent that Debian requires git, where Alpine doesn't...because?

@tianon
Copy link
Member

tianon commented Nov 30, 2020

The default (Debian) image tags target convenience -- see, for example, python:3.8 which is FROM buildpack-deps:buster vs python:3.8-slim which is instead FROM debian:buster-slim. If we were to add "slim" variants of the Debian images here, they would also not include Git.

In contrast, the Alpine image tags almost always target size (because that's typically the main reason folks choose Alpine in the first place), and for many uses of Go, git is definitely not required (even less so when using Go Modules with a properly configured GOPROXY).

(In other words, I definitely still stand by #250 (comment).)

@Starttoaster
Copy link
Author

In contrast, the Alpine image tags almost always target size

This is a fair point. Though there's something like a 500MB difference between the alpine and debian image variants... Asking someone that needs git to have their CI download 500 extra MB because they want git is a pretty tall order. On the flipside, installing git for every CI job that needs it is also tedious and a tall order in its own way.

To counterpoint, is it always necessary for a golang image to contain ca-certificates and openssl? Bash?

@tianon
Copy link
Member

tianon commented Nov 30, 2020

To counterpoint, is it always necessary for a golang image to contain ca-certificates and openssl? Bash?

openssl and bash are only added temporarily for the purposes of the Go build/install process -- they are removed as soon as it's finished.

In the case of ca-certificates, it's definitely necessary in a much higher percentage of use cases than git is (since it'll be used any time Go talks to GOPROXY or any time an application reaches out to the internet), and is only ~512kB (although technically, most of it is already included in alpine:3.12 itself, so we could probably do some testing around that and remove that too, but I don't know that the benefit in doing so is strong and I certainly won't be exploring it).

@tianon
Copy link
Member

tianon commented Nov 30, 2020

Also, we're suggesting that users who want a supported installation of Go download those additional 500MB, not just folks who want git (because Go on Alpine is explicitly unsupported to the point that the upstream Go developers disabled the buildbot workers they had testing it because nobody was working on fixing it so it was just permanently failing).

@yosifkit
Copy link
Member

yosifkit commented Sep 7, 2021

Closing old issue; since our goal for alpine based images is small size, we do not want to add git to it.

This is documented on the Docker Hub description about the alpine variants:

To minimize image size, additional related tools (such as git, gcc, or bash) are not included in Alpine-based images. Using this image as a base, add the things you need in your own Dockerfile (see the alpine image description for examples of how to install packages if you are unfamiliar). See also docker-library/golang#250 (comment) for a longer explanation.

@yosifkit yosifkit closed this Sep 7, 2021
@yosifkit yosifkit mentioned this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants