add docker image build support by underrun · Pull Request #588 · ksonnet/ksonnet

underrun · 2018-06-04T23:13:36Z

closes #291

rough edges around running with mounts and permissions, but bones are here. could use advice on creating an alias for this - had some trouble because it needs pwd. thoughts?

coveralls · 2018-06-04T23:18:43Z

Pull Request Test Coverage Report for Build 943

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 69.405%

Totals
Change from base Build 934:	0.01%
Covered Lines:	9385
Relevant Lines:	13522

💛 - Coveralls

shomron · 2018-06-05T16:45:03Z

Makefile

 	$(DOC_GEN_FILE)

+docker-image:
+	docker build -t ks --build-arg LD_FLAGS="$(LD_FLAGS)" .


We should probably tag according to the ksonnet version as well. Maybe git describe --tags with or without the v prefix?

if you look in the LD_FLAGS it grabs git rev-parse HEAD which gives you sha1 ... but do you mean for human consumption like having a ks images named ks-0.11.0 or something along those lines?

Yeah look how images are tagged on DockerHub (where these images should ultimately be pushed via Jenkins). I'd only include sha for non-release tags.

shomron · 2018-06-05T16:48:45Z

docs/docker-image-build.md

+In order to run via docker, the `ks` process needs access to a kubernetes config file, certificates, and the current working directory. We set these up using bind mounts:
+
+```bash
+docker run -e KUBECONFIG="${KUBECONFIG}" --mount type=bind,source="${KUBECONFIG}",target="${KUBECONFIG}" --mount type=bind,source="${HOME}/.minikube",target="${HOME}/.minikube" --mount type=bind,source="$(pwd)",target="$(pwd)" -w "$(pwd)" ks --help


This fails for people like me who lack a KUBECONFIG variable.
Let's supply a default:

${KUBECONFIG} -> ${KUBECONFIG:-${HOME}/.kube/config}

The minikube dependency is not always going to be correct - perhaps what we really give is a bash function that parses out the certificate paths from ${KUBECONFIG} and builds the arguments that way? Or is that overkill?

oh man i know - i actually had the exact line you suggest for KUBECONFIG with default ... and i had a CERTDIR with default to ${HOME}/.kube ... and i talked to @bryanl about parsing the cert file but apparently there are versions of the cert specification that are links on which actions need to be taken so ... that's exciting.

the ergonomics of this are why i didn't include something to add to .bash_aliases - it's way to user specific to do something simple. we can make this work intuitively but it might have diminishing returns.

Agreed. Maybe we attack this with documentation, describing what needs to be mounted logically and giving an example.

lol - yeah - this is the docs that i meant that to be - i'll add a couple other examples and note failure cases as well to help people navigate.

which builds a docker image for ks Signed-off-by: Derek Wilson <derek@heptio.com>

use lock file for apimachinery, add dep ensure to makefile, tag image, and expand docs around using `ks` from docker. Signed-off-by: Derek Wilson <derek@heptio.com>

alias changes so that it gets pwd and works more like stand alone ks build. Signed-off-by: Derek Wilson <derek@heptio.com>

bryanl · 2018-06-06T19:14:32Z

Makefile

-APIMACHINERY_VER := $(shell dep status | grep k8s.io/apimachinery | awk '{print $$3}')
+APIMACHINERY_VER := $(shell grep -B1 k8s.io/apimachinery Gopkg.lock | head -n1 | cut -d'"' -f2)
 REVISION=$(shell git rev-parse HEAD)
+GIT_TAG=$(shell git describe --tags)


Would it make sense to only set GIT_TAG if is needed?

so in Makefiles variables set with = are lazily evaluated ... so it really doesn't hurt anything to make it available here as it won't run git describe unless you are building a docker image. but i don't mind moving this into the target if that seems cleaner?

bryanl · 2018-06-06T19:16:14Z

Makefile

 docs:
 	$(DOC_GEN_FILE)

+docker-image: Gopkg.lock


This should be be added to .PHONY at the bottom of the file? (and also install should be as well)

bryanl · 2018-06-06T19:18:14Z

docs/docker-image-build.md

@@ -0,0 +1,29 @@
+# Build a `ks` docker image


@GuessWhoSamFoo What do you think about this text?

Nit: Referencing Docker outside the context of a CLI should be upper case.

because if you're debugging it shouldn't be in the docker image Signed-off-by: Derek Wilson <derek@heptio.com>

GuessWhoSamFoo · 2018-06-06T20:48:28Z

docs/docker-image-build.md

+
+This sets the $KUBECONFIG environment variable inside the container, mounts the config and the directory holding certificates (which can be found inside the kubeconfig), and mounts the current working directory so that the `ks` binary knows where to work.
+
+It is also possible to use as an alias:


Clarify the intention of setting an alias. e.g:

Optionally, set an alias to shorten the verbose command if using the docker image locally.

GuessWhoSamFoo · 2018-06-06T20:56:08Z

docs/docker-image-build.md

+
+# Running `ks` via docker
+
+In order to run via docker, the `ks` process needs access to a kubernetes config file, certificates, and the current working directory. We can set these up using mounts. Here's what it looks like running on a local cluster with minikube:


Instead of "We can set these up using mounts"

Pass the --mount flag to capture the path of these files.

Add targets to .PHONY and clarify and improve docs Signed-off-by: Derek Wilson <derek@heptio.com>

underrun requested review from bryanl and shomron June 4, 2018 23:13

shomron reviewed Jun 5, 2018

View reviewed changes

add docker-image make target

8a398ee

which builds a docker image for ks Signed-off-by: Derek Wilson <derek@heptio.com>

underrun force-pushed the master branch from a3ef921 to 8a398ee Compare June 5, 2018 16:52

Derek Wilson added 2 commits June 6, 2018 11:45

makefile skip dep status and add docs

b32e8e3

use lock file for apimachinery, add dep ensure to makefile, tag image, and expand docs around using `ks` from docker. Signed-off-by: Derek Wilson <derek@heptio.com>

add .dockerignore and fix alias

9975d92

alias changes so that it gets pwd and works more like stand alone ks build. Signed-off-by: Derek Wilson <derek@heptio.com>

bryanl suggested changes Jun 6, 2018

View reviewed changes

bryanl reviewed Jun 6, 2018

View reviewed changes

Shrink docker image by stripping symbols

c0e19cd

because if you're debugging it shouldn't be in the docker image Signed-off-by: Derek Wilson <derek@heptio.com>

GuessWhoSamFoo reviewed Jun 6, 2018

View reviewed changes

Imrpove docs and Makefile

c5c07a2

Add targets to .PHONY and clarify and improve docs Signed-off-by: Derek Wilson <derek@heptio.com>

bryanl approved these changes Jun 6, 2018

View reviewed changes

underrun merged commit 5522bf6 into ksonnet:master Jun 6, 2018


		This sets the $KUBECONFIG environment variable inside the container, mounts the config and the directory holding certificates (which can be found inside the kubeconfig), and mounts the current working directory so that the `ks` binary knows where to work.

		It is also possible to use as an alias:


		# Running `ks` via docker

		In order to run via docker, the `ks` process needs access to a kubernetes config file, certificates, and the current working directory. We can set these up using mounts. Here's what it looks like running on a local cluster with minikube:

Conversation

underrun commented Jun 4, 2018

Uh oh!

coveralls commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 943

💛 - Coveralls

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jun 4, 2018 •

edited

Loading