Enhance Dockerfile for local building #1002

chrisplo · 2017-10-10T17:21:50Z

Added a caching layer in the docker build for compiling all of the
vendor dependencies, drops compilation time to about 15s

Added support in Dockerfile for passing BUILD_VERSION and
NIGHTLY_RELEASE to build.sh

Add compile-with-docker target

Split out how we get the git commit SHA so it's reusable for docker
image tag

Added a caching layer in the docker build for compiling all of the vendor dependencies, drops compilation time to about 15s Added support in Dockerfile for passing BUILD_VERSION and USE_RELEASE to build.sh Add compile-with-docker target Split out how we get the git commit SHA so it's reusable for docker image tag

chrisplo · 2017-10-10T17:22:50Z

added "${BUILD_VERSION:-devBuild}-" to the tag, otherwise detail output below is the same:
examples:

BUILD_VERSION=1.2.3 make compile-with-docke
Successfully tagged netplugin:1.2.3-ecb31d87-unsupported

make compile-with-docker
Successfully tagged netplugin:devBuild-ecb31d87-unsupported

Detail with version info:

make compile-with-docker

scripts/build.sh
Version: 1.0.0-beta-10-10-2017.17-00-47.UTC
GitCommit: ecb31d8-unsupported
BuildTime: 10-10-2017.17-00-47.UTC

Successfully tagged netplugin:ecb31d87-unsupported

USE_RELEASE=y BUILD_VERSION=1.2.3 make compile-with-docker

Version: 1.2.3
GitCommit: ecb31d8-unsupported
BuildTime: 10-10-2017.17-01-36.UTC

Successfully tagged netplugin:ecb31d87-unsupported

USE_RELEASE=y make compile-with-docker

Version: 1.0.0-beta
GitCommit: ecb31d8-unsupported
BuildTime: 10-10-2017.17-02-52.UTC

Successfully tagged netplugin:ecb31d87-unsupported

BUILD_VERSION=1.2.3 make compile-with-docker

Version: 1.2.3-10-10-2017.16-58-15.UTC
GitCommit: ecb31d8-unsupported
BuildTime: 10-10-2017.16-58-15.UTC

Successfully tagged netplugin:ecb31d87-unsupported

chrisplo · 2017-10-10T17:36:38Z

make build was successfull as well

dseevr · 2017-10-10T17:37:06Z

Dockerfile


 ENTRYPOINT ["netplugin"]
 CMD ["--help"]

-COPY ./ /go/src/github.com/contiv/netplugin/
+# build the vendor dependencies


Since it's not obvious (apparently lol) why this is being done manually as a separate step, could you update the comment here to explain that this is for caching purposes because vendored code takes a long time to compile?

chrisplo · 2017-10-10T17:40:10Z

I pulled a compile target last minute as thought wasn't needed, need to restore, will add comments as well

chrisplo · 2017-10-10T17:49:09Z

rechecked make compile-with-docker and

$ docker run --entrypoint ls netplugin:devBuild-c18b3301-unsupported /etc/bash_completion.d/netctl
/etc/bash_completion.d/netctl

chrisplo · 2017-10-11T04:21:27Z

build PR

dseevr · 2017-10-12T04:19:45Z

Now that #999 is merged, you can remove that grep -v and such

dseevr · 2017-10-12T04:21:20Z

scripts/getGitCommit.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+


Can we get a

set -euo pipefail

here? I think we should include it in every new script by default.

good catch, thx

dseevr · 2017-10-12T04:21:45Z

scripts/getGitCommit.sh

+	if [ -n "$(git status --porcelain --untracked-files=no)" ]; then
+		GIT_COMMIT="$GIT_COMMIT-unsupported"
+	fi
+    echo $GIT_COMMIT


Inconsistent indentation on lines 8 and 9 vs. 4-7

Check out shfmt if you want an easy way to automatically tidy up scripts. We use this on auth_proxy and install

wth is are tabs doing in there :/

gkvijay · 2017-10-12T05:09:17Z

Dockerfile

+RUN GOPATH=/go/ \
+    BUILD_VERSION="${BUILD_VERSION}" \
+    USE_RELEASE="${USE_RELEASE}" \
+    make compile \


If you are compiling netplugin as part of docker build then you should add atleast checks here. Otherwise, we may not catching errors. If the idea is to make a quick build knowing that there are no code changes (for releases), then can Dockerfile only compile vendor directory and build the container. User can decide to skip tests when running the make using docker exec command

I have checks for docker based build as a backlog item but this isn't replacing other ways to build so it's not required (yet), and this PR is long enough already.

I was planning to have a separate Dockerfile that does the checks after compile which I think matches development flow better and will be faster to iterate. Also, will allow choice as to when to run the checks based on which target is being run in the Makefile.

Please add the development Dockerfile here (root directory) and move the one with no checks to a release directory. Please take care of it in a later PR

chrisplo · 2017-10-12T15:25:10Z

I will also check for changes due to PR1000 merging

…update

go-winio is now safe to include in compile strict bash checks and fix indents getGitCommit.sh switch docker build from USE_RELEASE to NIGHTLY_BUILD

dseevr · 2017-10-12T19:19:29Z

build pr

chrisplo · 2017-10-12T19:24:57Z

do you want me to pull apart the getGitCommit as separate PR @unclejack @dseevr ?

dseevr · 2017-10-12T19:25:39Z

It's required for this PR to be merged and function so it seems fine to me as it is

chrisplo · 2017-10-12T19:39:06Z

when squash merging this please use the PR description as the commit msg (instead of the first commit msg)

chrisplo · 2017-10-12T21:17:48Z

Makefile

+	docker build \
+		--build-arg NIGHTLY_RELEASE=${NIGHTLY_RELEASE} \
+		--build-arg BUILD_VERSION=${BUILD_VERSION} \
+		-t netplugin:$${BUILD_VERSION:-devBuild}-$$(./scripts/getGitCommit.sh) .


devbuild not devBuild

dseevr · 2017-10-12T21:28:39Z

build pr

dseevr · 2017-10-13T02:42:55Z

@unclejack PTAL and merge if it looks good

gkvijay · 2017-10-13T13:29:23Z

Dockerfile

+RUN GOPATH=/go/ \
+    BUILD_VERSION="${BUILD_VERSION}" \
+    USE_RELEASE="${USE_RELEASE}" \
+    make compile \


Please add the development Dockerfile here (root directory) and move the one with no checks to a release directory. Please take care of it in a later PR

unclejack · 2017-10-13T19:57:11Z

Dockerfile


+RUN GOPATH=/go/ \


This Dockerfile should call make deps at some point to add the tools used to check if the source is fine to the image.

The executed command should include the checks target to run it before the build. This should reject code which doesn't meet the requirements.

unclejack · 2017-10-13T20:03:10Z

Makefile

+# fully prepares code for pushing to branch, includes building binaries
+run-build: deps checks clean compile
+
+compile-with-docker:


Could you set up these tasks like this, please?
host-compile-with-docker: -what we have in this target
compile-with-docker: - a wrapper which builds this in the Vagrant VM (e.g.: vagrant ssh netplugin-node1 -c 'bash -lc "source /etc/profile.d/envvar.sh && cd /opt/gopath/src/github.com/contiv/netplugin && make host-compile-with-docker"')

It should be possible to invoke these targets on the host and on the VMs.

unclejack · 2017-10-13T20:04:53Z

@chrisplo: Can you get rid of the merge commit and integrate the changes into their corresponding commits, please?

unclejack · 2017-10-13T21:11:35Z

The merge commit is fine in this case. This particular PR will get squashed on merge.

dseevr reviewed Oct 10, 2017

View reviewed changes

add comments, add missing compile target

3b21d1b

dseevr requested changes Oct 12, 2017

View reviewed changes

gkvijay reviewed Oct 12, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into dockerfile_compile_…

e8b2ec1

…update

chrisplo force-pushed the dockerfile_compile_update branch 2 times, most recently from 3766099 to 27a0a57 Compare October 12, 2017 18:55

Review feedback

1097d2a

go-winio is now safe to include in compile strict bash checks and fix indents getGitCommit.sh switch docker build from USE_RELEASE to NIGHTLY_BUILD

chrisplo force-pushed the dockerfile_compile_update branch from 27a0a57 to 1097d2a Compare October 12, 2017 18:57

dseevr approved these changes Oct 12, 2017

View reviewed changes

chrisplo commented Oct 12, 2017

View reviewed changes

devbuild not devBuild

0272201

gkvijay approved these changes Oct 13, 2017

View reviewed changes

unclejack reviewed Oct 13, 2017

View reviewed changes

chrisplo merged commit 5732e53 into contiv:master Oct 13, 2017

chrisplo deleted the dockerfile_compile_update branch October 27, 2017 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Dockerfile for local building #1002

Enhance Dockerfile for local building #1002

chrisplo commented Oct 10, 2017 •

edited

chrisplo commented Oct 10, 2017

chrisplo commented Oct 10, 2017

dseevr Oct 10, 2017

chrisplo commented Oct 10, 2017

chrisplo commented Oct 10, 2017 •

edited

chrisplo commented Oct 11, 2017

dseevr commented Oct 12, 2017

dseevr Oct 12, 2017

chrisplo Oct 12, 2017

dseevr Oct 12, 2017

chrisplo Oct 12, 2017

gkvijay Oct 12, 2017 •

edited

chrisplo Oct 12, 2017

gkvijay Oct 13, 2017

chrisplo commented Oct 12, 2017

dseevr commented Oct 12, 2017

chrisplo commented Oct 12, 2017

dseevr commented Oct 12, 2017

chrisplo commented Oct 12, 2017 •

edited

chrisplo Oct 12, 2017

dseevr commented Oct 12, 2017

dseevr commented Oct 13, 2017

gkvijay Oct 13, 2017

unclejack Oct 13, 2017

unclejack Oct 13, 2017

unclejack commented Oct 13, 2017

unclejack commented Oct 13, 2017


		RUN GOPATH=/go/ \

Enhance Dockerfile for local building #1002

Enhance Dockerfile for local building #1002

Conversation

chrisplo commented Oct 10, 2017 • edited

chrisplo commented Oct 10, 2017

chrisplo commented Oct 10, 2017

Choose a reason for hiding this comment

chrisplo commented Oct 10, 2017

chrisplo commented Oct 10, 2017 • edited

chrisplo commented Oct 11, 2017

dseevr commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkvijay Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisplo commented Oct 12, 2017

dseevr commented Oct 12, 2017

chrisplo commented Oct 12, 2017

dseevr commented Oct 12, 2017

chrisplo commented Oct 12, 2017 • edited

Choose a reason for hiding this comment

dseevr commented Oct 12, 2017

dseevr commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclejack commented Oct 13, 2017

unclejack commented Oct 13, 2017

chrisplo commented Oct 10, 2017 •

edited

chrisplo commented Oct 10, 2017 •

edited

gkvijay Oct 12, 2017 •

edited

chrisplo commented Oct 12, 2017 •

edited