iframe-proxy

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

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

make build was successfull as well

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

 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

@@ -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

+	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

+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

+	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

+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

+ARG BUILD_VERSION=""
+ARG NIGHTLY_RELEASE=""

+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

+# 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

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

chrisplo commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisplo commented Oct 10, 2017

Uh oh!

chrisplo commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisplo commented Oct 10, 2017

Uh oh!

chrisplo commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisplo commented Oct 11, 2017

Uh oh!

dseevr commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkvijay Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisplo commented Oct 12, 2017

Uh oh!

dseevr commented Oct 12, 2017

Uh oh!

chrisplo commented Oct 12, 2017

Uh oh!

dseevr commented Oct 12, 2017

Uh oh!

chrisplo commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dseevr commented Oct 12, 2017

Uh oh!

dseevr commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unclejack commented Oct 13, 2017

Uh oh!

unclejack commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisplo commented Oct 10, 2017 •

edited

Loading

chrisplo commented Oct 10, 2017 •

edited

Loading

gkvijay Oct 12, 2017 •

edited

Loading

chrisplo commented Oct 12, 2017 •

edited

Loading