Build checks in container #1012

chrisplo · 2017-10-16T03:02:22Z

Before the only option was to spin up a VM to run code checks, which is
expensive in terms of time.

Add Dockerfile-check to provide a container build for a runtime
environment that supports go tool vet, gofmt, and golint

Add script 'code_checks_in_docker.sh' intended to be run inside the
Dockerfile-check container that runs make checks which in turn runs a
variety of go code checks against packages passed in.

Checks are reordered so more severe issues (e.g. functional) are
resolved before less severe ones (e.g. formatting).

Some checks are upated to be simpler and faster because the command
accept multiple directories and recurse automatically.

Drive-by:

removed some trailing whitespace
found another binary to exclude in dockerignore
variables for go check commands not needed, they are only refernced
once and require a visual lookup to find what the command actually
is

Signed-off-by: Chris Plock chrisplo@cisco.com

unclejack · 2017-10-16T15:18:01Z

Makefile

+tar: compile-with-docker
+	docker run --rm netplugin-build:$(NETPLUGIN_CONTAINER_TAG) \
+		-C /go/src/github.com/contiv/netplugin netplugin-version \
+	| tar -x netplugin-version


Please use docker cp here. Piping can break if there's any Docker regression.

From the docs: docker cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|-

At this point there has never been a container, only an image, so there is no way I could use docker cp (without docker run, then docker cp, then docker rm)

@chrisplo: You can use docker create to create a container without running. You can remove it with docker rm.

Relying on the stdout isn't recommended. There have been regressions in the past with relying on piping from stdout and piping via stdin.

cool, I'll update with create/cp

actually did this in #1008, and removed this section as we don't depend on it for this PR

unclejack · 2017-10-16T15:18:29Z

Makefile

+	docker run --rm netplugin-build:$(NETPLUGIN_CONTAINER_TAG) \
+		-C /go/src/github.com/contiv/netplugin netplugin-version \
+	| tar -x netplugin-version
+	docker run --rm netplugin-build:$(NETPLUGIN_CONTAINER_TAG) \


This is the same as above - it should be done using docker cp.

did this in #1008, and removed this section as we don't depend on it for this PR

unclejack · 2017-10-16T15:19:22Z

scripts/code_checks.sh

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


Please keep using the Makefile checks for now. This script would get blown away soon.

unclejack · 2017-10-16T15:21:50Z

Dockerfile-check

+WORKDIR /go/src/github.com/contiv/netplugin/
+ENTRYPOINT ["/code_checks.sh"]
+
+RUN go get github.com/tools/godep github.com/aktau/github-release \


Please use https://github.com/contiv/netplugin/blob/master/scripts/deps by invoking the right Makefile target.

dseevr · 2017-10-16T22:11:39Z

Dockerfile-check

@@ -0,0 +1,15 @@
+ARG TAG=latest


General comments on this file:

We do something similar already in auth_proxy, take a look at Dockerfile.checks

In general, we want to COPY in the code to be checked as one of the last operations in the Dockerfile (for caching reasons). Bindmounting + SELinux = nightmare land, and it also prevents the use of docker-machine in general

COPY is at the end, though this ARG was leftover from an earlier incantation (for FROM) I'll remove

Right, you're COPYing in the script, that's fine...

What I'm saying is we need to COPY in all the code to be checked, e.g.,:

COPY ./ /go/src/github.com/contiv/netplugin

We shouldn't use a bindmount on line 95 in Makefile

dseevr · 2017-10-16T22:14:13Z

scripts/code_checks.sh

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


Unrelated to whatever script we end up using, these tools are slooooooow unless you exclude vendor. PTAL at https://github.com/contiv/auth_proxy/blob/6340df9b07a1723e2419069ebb5c0d5be6850a2e/scripts/checks_in_container.sh#L10-L11

agreed, I borrowed PKG_DIRS which already filtered out vendor
EXCLUDE_DIRS := bin docs Godeps scripts vagrant vendor install
PKG_DIRS := $(filter-out $(EXCLUDE_DIRS),$(subst /,,$(sort $(dir $(wildcard */)))))

dseevr · 2017-10-17T20:50:07Z

Dockerfile-check

+
+WORKDIR /go/src/github.com/contiv/netplugin/
+
+RUN go get github.com/tools/godep github.com/aktau/github-release \


Is github.com/aktau/github-release here intentionally? 😄

was copied, removed!

Before the only option was to spin up a VM to run code checks, which is expensive in terms of time. Add Dockerfile-check to provide a container build for a runtime environment that supports go tool vet, gofmt, and golint Add script 'code_checks_in_docker.sh' intended to be run inside the Dockerfile-check container that runs `make checks` which in turn runs a variety of go code checks against packages passed in. Checks are reordered so more severe issues (e.g. functional) are resolved before less severe ones (e.g. formatting). Some checks are upated to be simpler and faster because the command accept multiple directories and recurse automatically. Drive-by: * removed some trailing whitespace * added more exclusions in dockerignore * variables for go check commands not needed, they are only refernced once and require a visual lookup to find what the command actually is Signed-off-by: Chris Plock <chrisplo@cisco.com>

dseevr · 2017-10-17T21:25:05Z

@unclejack PTAL and merge if it looks good to you

chrisplo · 2017-10-17T21:33:34Z

build pr

rchirakk · 2017-10-17T23:56:55Z

LGTM, thx for fixing vet/lint/fmt.
another option for quick build/check is "make run-build"

chrisplo · 2017-10-18T05:16:32Z

build PR

chrisplo · 2017-10-18T16:57:20Z

build PR

chrisplo changed the title ~~Build checks in container, blocked on #1008 #1009~~ Build checks in container, blocked on #1008 #1007 Oct 16, 2017

chrisplo changed the title ~~Build checks in container, blocked on #1008 #1007~~ Build checks in container, blocked on #1008 #1007 #1009 Oct 16, 2017

unclejack mentioned this pull request Oct 16, 2017

use netplugin-build for compile container name #1009

Merged

unclejack reviewed Oct 16, 2017

View reviewed changes

chrisplo changed the title ~~Build checks in container, blocked on #1008 #1007 #1009~~ Build checks in container, blocked on #1008 #1009 Oct 16, 2017

chrisplo force-pushed the build_checks_in_container branch 2 times, most recently from 01da373 to 8b4fa20 Compare October 16, 2017 22:04

chrisplo changed the title ~~Build checks in container, blocked on #1008 #1009~~ Build checks in container Oct 16, 2017

dseevr reviewed Oct 16, 2017

View reviewed changes

chrisplo force-pushed the build_checks_in_container branch from 8b4fa20 to fda2c9a Compare October 17, 2017 20:44

dseevr reviewed Oct 17, 2017

View reviewed changes

chrisplo force-pushed the build_checks_in_container branch from fda2c9a to bc62324 Compare October 17, 2017 21:15

dseevr approved these changes Oct 17, 2017

View reviewed changes

dseevr requested a review from unclejack October 17, 2017 21:19

chrisplo merged commit e48a518 into contiv:master Oct 18, 2017

chrisplo deleted the build_checks_in_container branch October 18, 2017 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build checks in container #1012

Build checks in container #1012

chrisplo commented Oct 16, 2017 •

edited

Loading

unclejack Oct 16, 2017

chrisplo Oct 16, 2017 •

edited

Loading

unclejack Oct 16, 2017

chrisplo Oct 16, 2017

chrisplo Oct 17, 2017

unclejack Oct 16, 2017

chrisplo Oct 17, 2017

unclejack Oct 16, 2017

unclejack Oct 16, 2017

dseevr Oct 16, 2017

chrisplo Oct 16, 2017

dseevr Oct 16, 2017

dseevr Oct 16, 2017

chrisplo Oct 16, 2017

dseevr Oct 17, 2017

chrisplo Oct 17, 2017

dseevr commented Oct 17, 2017

chrisplo commented Oct 17, 2017

rchirakk commented Oct 17, 2017

chrisplo commented Oct 18, 2017

chrisplo commented Oct 18, 2017


		WORKDIR /go/src/github.com/contiv/netplugin/

		RUN go get github.com/tools/godep github.com/aktau/github-release \

Build checks in container #1012

Build checks in container #1012

Conversation

chrisplo commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

chrisplo Oct 16, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr commented Oct 17, 2017

chrisplo commented Oct 17, 2017

rchirakk commented Oct 17, 2017

chrisplo commented Oct 18, 2017

chrisplo commented Oct 18, 2017

chrisplo commented Oct 16, 2017 •

edited

Loading

chrisplo Oct 16, 2017 •

edited

Loading