Update dockerfile #11
Conversation
| @@ -13,6 +13,6 @@ BUILD_SCRIPT="cd /src && "\ | |||
| "rm -rf build && mkdir build && "\ | |||
| "go build -o build/codeclimate-golint ." | |||
|
|
|||
| docker run -v "$SRC_ROOT:/src" yunspace/alpine-golang /bin/sh -c "$BUILD_SCRIPT" | |||
| docker run -v "$SRC_ROOT:/src" golang:latest /bin/sh -c "$BUILD_SCRIPT" | |||
There was a problem hiding this comment.
I noted this on the other PR after it merged, but should we also use golang:alpine here? It should make CI a bit faster (one image to pull instead of two), and also just seems preferable in terms of being consistent about which image we're using.
There was a problem hiding this comment.
The issue is that golang:alpine doesn't have git
Build script errors like so:
...
github.com/codeclimate/cc-engine-go (download)
go: missing Git command. See https://golang.org/s/gogetcmd
package github.com/codeclimate/cc-engine-go/engine: exec: "git": executable file not found in $PATH
github.com/golang/lint (download)
go: missing Git command. See https://golang.org/s/gogetcmd
package github.com/golang/lint: exec: "git": executable file not found in $PATH
There was a problem hiding this comment.
You could install git separately:
docker run -v "$SRC_ROOT:/src" golang:alpine /bin/sh -c "apk --update add git && $BUILD_SCRIPT"
There was a problem hiding this comment.
Hmm. I think installing a new package every time we build would probably cancel out any time savings from pulling a smaller image, so maybe this should stay as-is.
| @@ -1,6 +1,12 @@ | |||
| FROM alpine:3.2 | |||
| FROM golang:alpine | |||
|
|
|||
There was a problem hiding this comment.
I think we can keep the base image plain alpine here: go lint doesn't need the go base toolchain installed (gofmt & go vet do), so I think this is just making the image bigger.
There was a problem hiding this comment.
Does it matter that alpine:3.2 has go ~1.4 installed?
go lint doesn't need the go base toolchain installed (go lint & go vet do)
There was an issue with govet recently where the go version in image seemed to result in unexpected engine behavior. Are you suggesting that the go version doesn't matter here in the same way since golint is its own package?
There was a problem hiding this comment.
Are you suggesting that the go version doesn't matter here in the same way since golint is its own package?
Precisely. Go vet & Gofmt interact with base parts of the go toolchain, so having that toolchain installed is necessary, and which version is installed matters.
Go lint doesn't use the toolchain at all except to get compiled (which is what bin/build does). The currently shipped image doesn't even have go installed, and I don't think we need to change that.
There was a problem hiding this comment.
Cool sounds good. Updated
|
@wfleming Ready for another look! |
|
LGTM |
- Make compliant with SPEC - Update go image
ABaldwinHunter
force-pushed
the
abh-update-go-image
branch
from
b165b32 to
1fb4c9e
Compare
@codeclimate-community/cc-staff