Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Dockerfile for local building #1002

Merged
merged 5 commits into from
Oct 13, 2017
Merged

Enhance Dockerfile for local building #1002

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c18b330
Enhance Dockerfile for local building
chrisplo Oct 10, 2017
3b21d1b
add comments, add missing compile target
chrisplo Oct 10, 2017
e8b2ec1
Merge remote-tracking branch 'origin/master' into dockerfile_compile_…
chrisplo Oct 12, 2017
1097d2a
Review feedback
chrisplo Oct 12, 2017
0272201
devbuild not devBuild
chrisplo Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,30 @@ FROM golang:1.7.6
#ENV https_proxy ""
ARG http_proxy
ARG https_proxy
ENV GOPATH=/go/ NET_CONTAINER_BUILD=1

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

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

COPY ./ /go/src/github.com/contiv/netplugin/
# by far, most of the compilation time is building vendor packages
# build the vendor dependencies as a separate docker caching layer
COPY ./vendor/ /go/src/github.com/contiv/netplugin/vendor/
# there is a bug in go-winio, remote the grep -v after this merges:
# https://github.com/contiv/netplugin/pull/999
RUN GOGC=1500 \
go install -ldflags "-s -w" \
$(go list ./vendor/... | grep -v go-winio)

WORKDIR /go/src/github.com/contiv/netplugin/
# build the netplugin binaries
COPY ./ /go/src/github.com/contiv/netplugin/

RUN make build
ARG BUILD_VERSION=""
ARG USE_RELEASE=""

RUN GOPATH=/go/ \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

BUILD_VERSION="${BUILD_VERSION}" \
USE_RELEASE="${USE_RELEASE}" \
make compile \
Copy link
Member

@gkvijay gkvijay Oct 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

&& cp scripts/contrib/completion/bash/netctl /etc/bash_completion.d/netctl \
&& netplugin -version
32 changes: 11 additions & 21 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

.PHONY: all all-CI build clean default unit-test release tar checks go-version gofmt-src golint-src govet-src
.PHONY: all all-CI build clean default unit-test release tar checks go-version gofmt-src \
golint-src govet-src run-build compile-with-docker

DEFAULT_DOCKER_VERSION := 1.12.6
SHELL := /bin/bash
Expand Down Expand Up @@ -86,23 +87,25 @@ endif

checks: go-version gofmt-src golint-src govet-src misspell-src

run-build: deps checks clean
compile:
cd $(GOPATH)/src/github.com/contiv/netplugin && \
USE_RELEASE=${USE_RELEASE} BUILD_VERSION=${BUILD_VERSION} \
TO_BUILD="${TO_BUILD}" VERSION_FILE=${VERSION_FILE} \
scripts/build.sh

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

compile-with-docker:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

build-docker-image: start
vagrant ssh netplugin-node1 -c 'bash -lc "source /etc/profile.d/envvar.sh && cd /opt/gopath/src/github.com/contiv/netplugin && make host-build-docker-image"'


ifdef NET_CONTAINER_BUILD
install-shell-completion:
cp scripts/contrib/completion/bash/netctl /etc/bash_completion.d/netctl
else
install-shell-completion:
sudo cp scripts/contrib/completion/bash/netctl /etc/bash_completion.d/netctl
endif

build: start ssh-build stop

Expand All @@ -116,12 +119,8 @@ update:

# setting CONTIV_NODES=<number> while calling 'make demo' can be used to bring
# up a cluster of <number> nodes. By default <number> = 1
ifdef NET_CONTAINER_BUILD
start:
else
start:
CONTIV_DOCKER_VERSION="$${CONTIV_DOCKER_VERSION:-$(DEFAULT_DOCKER_VERSION)}" CONTIV_NODE_OS=${CONTIV_NODE_OS} vagrant up
endif

# ===================================================================
#kubernetes demo targets
Expand Down Expand Up @@ -195,26 +194,17 @@ mesos-cni-destroy:
demo-ubuntu:
CONTIV_NODE_OS=ubuntu make demo

ifdef NET_CONTAINER_BUILD
stop:
else
stop:
CONTIV_NODES=$${CONTIV_NODES:-3} vagrant destroy -f
endif

demo: ssh-build
vagrant ssh netplugin-node1 -c 'bash -lc "source /etc/profile.d/envvar.sh && cd /opt/gopath/src/github.com/contiv/netplugin && make host-restart host-swarm-restart"'

ssh:
@vagrant ssh netplugin-node1 -c 'bash -lc "cd /opt/gopath/src/github.com/contiv/netplugin/ && bash"' || echo 'Please run "make demo"'

ifdef NET_CONTAINER_BUILD
ssh-build:
cd /go/src/github.com/contiv/netplugin && make run-build install-shell-completion
else
ssh-build: start
vagrant ssh netplugin-node1 -c 'bash -lc "source /etc/profile.d/envvar.sh && cd /opt/gopath/src/github.com/contiv/netplugin && make run-build install-shell-completion"'
endif

unit-test: stop clean
./scripts/unittests -vagrant
Expand Down
12 changes: 2 additions & 10 deletions scripts/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,7 @@ else
BUILD_VERSION="$VERSION"
fi

if command -v git &>/dev/null && git rev-parse &>/dev/null; then
GIT_COMMIT=$(git rev-parse --short HEAD)
if [ -n "$(git status --porcelain --untracked-files=no)" ]; then
GIT_COMMIT="$GIT_COMMIT-unsupported"
fi
else
echo >&2 'error: unable to determine the git revision'
exit 1
fi
GIT_COMMIT=$(./scripts/getGitCommit.sh)

echo $BUILD_VERSION >$VERSION_FILE

Expand All @@ -32,4 +24,4 @@ GOGC=1500 go install \
-X $PKG_NAME.buildTime=$BUILD_TIME \
-X $PKG_NAME.gitCommit=$GIT_COMMIT \
-s -w" \
-v $TO_BUILD
$TO_BUILD
13 changes: 13 additions & 0 deletions scripts/getGitCommit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a

set -euo pipefail

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thx

if command -v git &>/dev/null && git rev-parse &>/dev/null; then
GIT_COMMIT=$(git rev-parse --short HEAD)
if [ -n "$(git status --porcelain --untracked-files=no)" ]; then
GIT_COMMIT="$GIT_COMMIT-unsupported"
fi
echo $GIT_COMMIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wth is are tabs doing in there :/

exit 0
fi
echo >&2 'error: unable to determine the git revision'
exit 1