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.

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

add docker image build support #588

Merged
merged 5 commits into from Jun 6, 2018

Conversation

Projects
None yet
5 participants
@underrun
Collaborator

underrun commented Jun 4, 2018

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?

@underrun underrun requested review from bryanl and shomron Jun 4, 2018

@coveralls

This comment has been minimized.

coveralls commented Jun 4, 2018

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 Coverage Status
Change from base Build 934: 0.01%
Covered Lines: 9385
Relevant Lines: 13522

💛 - Coveralls
Makefile Outdated
@@ -45,6 +46,9 @@ ks:
docs:
$(DOC_GEN_FILE)
docker-image:
docker build -t ks --build-arg LD_FLAGS="$(LD_FLAGS)" .

This comment has been minimized.

@shomron

shomron Jun 5, 2018

Collaborator

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

This comment has been minimized.

@underrun

underrun Jun 5, 2018

Collaborator

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?

This comment has been minimized.

@shomron

shomron Jun 5, 2018

Collaborator

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.

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 comment has been minimized.

@shomron

shomron Jun 5, 2018

Collaborator

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

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

This comment has been minimized.

@shomron

shomron Jun 5, 2018

Collaborator

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?

This comment has been minimized.

@underrun

underrun Jun 5, 2018

Collaborator

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.

This comment has been minimized.

@shomron

shomron Jun 5, 2018

Collaborator

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

This comment has been minimized.

@underrun

underrun Jun 5, 2018

Collaborator

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.

underrun added some commits Jun 2, 2018

add docker-image make target
which builds a docker image for ks

Signed-off-by: Derek Wilson <derek@heptio.com>
makefile skip dep status and add docs
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
alias changes so that it gets pwd and works more like stand alone ks
build.

Signed-off-by: Derek Wilson <derek@heptio.com>
REVISION=$(shell git rev-parse HEAD)
GIT_TAG=$(shell git describe --tags)

This comment has been minimized.

@bryanl

bryanl Jun 6, 2018

Member

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

This comment has been minimized.

@underrun

underrun Jun 6, 2018

Collaborator

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?

$(GO) build -o $(KS_BIN) $(GO_FLAGS) ./cmd/ks
docs:
$(DOC_GEN_FILE)
docker-image: Gopkg.lock

This comment has been minimized.

@bryanl

bryanl Jun 6, 2018

Member

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

This comment has been minimized.

@underrun

underrun Jun 6, 2018

Collaborator

will do

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

This comment has been minimized.

@bryanl

bryanl Jun 6, 2018

Member

@GuessWhoSamFoo What do you think about this text?

This comment has been minimized.

@GuessWhoSamFoo

GuessWhoSamFoo Jun 6, 2018

Collaborator

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

Shrink docker image by stripping symbols
because if you're debugging it shouldn't be in the docker image

Signed-off-by: Derek Wilson <derek@heptio.com>
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:

This comment has been minimized.

@GuessWhoSamFoo

GuessWhoSamFoo Jun 6, 2018

Collaborator

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

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

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

This comment has been minimized.

@GuessWhoSamFoo

GuessWhoSamFoo Jun 6, 2018

Collaborator

Instead of "We can set these up using mounts"

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

Imrpove docs and Makefile
Add targets to .PHONY and clarify and improve docs

Signed-off-by: Derek Wilson <derek@heptio.com>
@bryanl

bryanl approved these changes Jun 6, 2018

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

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 69.405%
Details
signed-off-by Commit has Signed-off-by
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment