-
Notifications
You must be signed in to change notification settings - Fork 48
Add circle CI config to build builder #85
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.
looks good overall, thanks for this! The only thing I'm 👎 is assuming things are on $GOPATH
outside of the build containers.
command: docker login -u $DOCKER_USER -p $DOCKER_PASS | ||
- run: | ||
name: Build builder image | ||
command: docker build -t titusoss/titus-executor-builder hack/builder |
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.
make push-builder
would be a bit better so we don't forget to change both places if we need to change how we build and push this 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.
Since this runs in the "image: docker:17.05.0-ce-git" container, and that doesn't have make, we can't do that, unless we have another image based on that image that has the builder.
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.
gotcha. This is fine then.
.circleci/config.yml
Outdated
- store_artifacts: | ||
path: /go/src/github.com/Netflix/titus-executor/build/distributions | ||
|
||
# test: |
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.
delete commented out section
@@ -36,11 +36,15 @@ tini/src: | |||
.PHONY: build | |||
build: tini/src | $(clean) $(builder) | |||
mkdir -p $(PWD)/build/distributions | |||
$(DOCKER_RUN) -v $(PWD):/src -v $(PWD)/build/distributions:/dist -u $(UID):$(GID) \ | |||
$(DOCKER_RUN) -v $(PWD):$(PWD) -u $(UID):$(GID) \ |
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.
this assumes the current local directory is in the GOPATH
. I'd rather we don't assume that and put things in the GOPATH the docker image expects.
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.
This assumes that the current working directory is the path to the Titus executor project, and that the path to the titus executor project is in the GOPATH. If the GOPATH inside-and-outside of the container are different, Golang gets confused. Part of this is to do with caching, and part of it, I do not understand.
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.
This makes assumptions about the local dev environment of whoever is running these make
tasks. Everything runs in a container, no need to have a GOPATH
configured or a strict requirement on the filesystem layout for the project. One can't just checkout the project and run the make
targets anymore, there needs to be a GOPATH
, etc, etc, even when the project isn't being compiled outside of the builder containers.
Unnecessary additional complexity on whoever tries to build the project locally without a Go dev environment IMHO.
What do you mean by "looks good overall, thanks for this! The only thing I'm 👎 is assuming things are on $GOPATH outside of the build containers." -- As in, the fact we prime the ci-builder container with govendor, and gometalinter, or the hack/builder container with gox, goimports, etc? |
No description provided.