Build docker image in circleci pipeline #67
Conversation
alvaroaleman
requested review from
guusvw,
metalmatze and
mrIncompetent
as code owners
ghost
assigned
Makefile
Outdated
| @@ -20,6 +20,13 @@ machine-controller: $(shell find cmd pkg -name '*.go') vendor | |||
|
|
|||
| docker-image: machine-controller | |||
| docker build -t $(IMAGE_NAME) . | |||
| if git describe --tags $(shell git rev-parse HEAD)|grep -v -- '-g'; then \ | |||
| $(eval IMAGE_TAG = $(shell git describe --abbrev=0 --tags)) \ | |||
There was a problem hiding this comment.
You can get the tag from the circle ci environment variables: https://circleci.com/docs/2.0/env-vars/#build-details
There was a problem hiding this comment.
nevermind - i'm an idiot. Obviously those are not suitable for the Makefile 🤦♂️
alvaroaleman
changed the title
Makefile
Outdated
| @@ -19,7 +21,26 @@ machine-controller: $(shell find cmd pkg -name '*.go') vendor | |||
| github.com/kubermatic/machine-controller/cmd/controller | |||
|
|
|||
| docker-image: machine-controller | |||
| make docker-image-nodep | |||
|
|
|||
| docker-image-nodep: | |||
There was a problem hiding this comment.
Yeah, thats rather non-obvious. The reason is that make uses timestamps on files to determine if it has to rebuild something. CircleCI uses caches from any build, which means that sometimes it restores the vendor folder from an older build. The result is:
- Make sees
Gopkg.{toml,lock}are newer than the vender folder - -> Have to build the vendor target
- -> Vendor target is newer than the binary ->Have to rebuild the binary
To skip all that (which makes perfect sense locally btw), I added that -nodep target
Makefile
Outdated
| docker build -t $(IMAGE_NAME) . | ||
| if git describe --tags $(shell git rev-parse HEAD)|grep -v -- '-g'; then \ | ||
| $(eval IMAGE_TAG = $(shell git describe --abbrev=0 --tags)) \ |
There was a problem hiding this comment.
So there are 2 things for me here:
- I would rather have 1 task called
pushwhich builds the image and pushes it. - Why have 2 tasks for it? - I would rather use an environment variable for the tag. Parsing some output always makes me cringe
There was a problem hiding this comment.
I guess we could merge the targets, but I dislike relying on CI-Implementiation-Specific env vars
There was a problem hiding this comment.
Regarding the env var, i use TAG or something within the makefile and in the pipeline i would set ENV=CIRCLE_SHA1
.circleci/config.yml
Outdated
| - machine-controller | ||
|
|
||
| publish: | ||
| machine: true |
There was a problem hiding this comment.
why machine instead of
docker:
- image: docker:stable
?
There was a problem hiding this comment.
Because that's what I found in the docs: https://circleci.com/docs/2.0/building-docker-images/#example
.circleci/config.yml
Outdated
|
|
||
| publish: | ||
| machine: true | ||
| steps: |
There was a problem hiding this comment.
Specify:
working_directory: /go/src/github.com/kubermatic/machine-controller
On the publish level, so you dont have to do a change-directory
There was a problem hiding this comment.
Yeah I tried that, makes the build instantly fails because that dir doesn' exist
There was a problem hiding this comment.
Can you try again?
In https://github.com/kube-node/kube-machine/blob/master/.circleci/config.yml
We didn't had to do it
.circleci/config.yml
Outdated
| docker version && | ||
| set -u && | ||
| docker login -u "${DOCKERHUB_USER}" -p "${DOCKERHUB_PASS}" && | ||
| make push-nodep |
There was a problem hiding this comment.
why && here?
Why not simply:
- run: |
make docker-image-nodep
...
There was a problem hiding this comment.
Yeah, combined with a set -x that will do the same, I can change it
.circleci/config.yml
Outdated
| - run: > | ||
| cd /go/src/github.com/kubermatic/machine-controller && | ||
| make docker-image-nodep && | ||
| docker login --help && |
There was a problem hiding this comment.
Debugging ;) Will remove
.circleci/config.yml
Outdated
| machine: true | ||
| steps: | ||
| - run: > | ||
| sudo mkdir -p /go/src/github.com/kubermatic/machine-controller && |
There was a problem hiding this comment.
sudo should not be required. If its required, we a producing this binary with wrong permissions
There was a problem hiding this comment.
It is not required because of the binary, it is required to create the folders
This reverts commit 6e1fb0f.
|
@mrIncompetent PTAL, all comments should be addressed now. You can check a successfully push in th build for 900ecc5 |
What this PR does / why we need it:
Automatically publish a docker image via CircleCI
Which issue(s) this PR fixes:
Fixes #57
Special notes for your reviewer: