Adding a Dockerfile and making it easy to use it for dev by wk8 · Pull Request #2745 · moby/swarmkit

wk8 · 2018-09-10T20:47:12Z

Credit goes to dperny (#2687)

- What I did

Adds a Dockerfile for the swarmkit project, to easily get off the ground. Modifies the Makefile to make intelligent use of Docker.

Also made small clean up changes to the Makefile.

- How I did it

Modifies the Makefile to have two paths: containerized.mk, which builds the docker image and forwards any make targets to a container, and direct.mk, which encompasses the old Makefile's workflow.

By default, nothing will run inside a container. Set the environment variable DOCKER_SWARMKIT_USE_CONTAINER to use dockerized making.

Also leverages docker-sync for synchronizing code to the container if the DOCKER_SWARMKIT_USE_DOCKER_SYNC env variable is set; comes in handy on Macs, for example.

- How to test it

Set DOCKER_SWARMKIT_USE_CONTAINER and verify that your favorite make targets all work!

- Description for the changelog
Adding a Dockerfile and making it easy to use it for dev

GordonTheTurtle · 2018-09-10T20:47:14Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "wk8/make_in_container" git@github.com:wk8/swarmkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov · 2018-09-10T20:57:34Z

Codecov Report

Merging #2745 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2745      +/-   ##
==========================================
+ Coverage    61.7%   61.73%   +0.02%     
==========================================
  Files         134      134              
  Lines       21890    21888       -2     
==========================================
+ Hits        13507    13512       +5     
+ Misses       6931     6913      -18     
- Partials     1452     1463      +11

wk8 · 2018-09-10T21:14:22Z

+	@go test -parallel 8 ${RACE} -tags "${DOCKER_BUILDTAGS}" $(filter-out ${INTEGRATION_PACKAGE},${PACKAGES})
+
+.PHONY: integration-tests
+integration-tests: ## run integration tests


Renamed from integration as that's the name of a directory, so when running with the containerized Makefile, make wouldn't see that as a phony target and wouldn't do anything...

anshulpundir · 2018-09-10T22:16:45Z

BTW the commit message doesn't need to follow the same format as the PR description or to be quite as long :)

You can just put down a brief summary @wk8

dperny

several nits, but none of them are blockers. you may decline to fix any of these things if you want to. just let me know.

dperny · 2018-09-13T18:19:47Z

+
+# stop here. do we want to run everything inside of a container, or do we want
+# to run it directly on the host? if the user has set ANY non-empty value for
+# the variable DOCKER_SWARMKIT_USE_CONTAINER, then we do all of the making inside of


small nit: would you mind wrapping to 80 columns? we don't consistently follow that style and i don't think we always should, but in cases like comments where there's no ugliness caused by doing so, it makes things a bit cleaner.

dperny · 2018-09-13T18:20:34Z

+	@ if [ ! $$(docker images -q ${IMAGE_NAME}) ]; then $(MAKE) image; fi
+
+# internal target, starts the sync if needed
+# uses https://github.com/EugenMayer/docker-sync/blob/47363ee31b71810a60b05822b9c4bd2176951ce8/tasks/sync/sync.thor#L193-L196


(for example, i wouldn't expect this line to be wrapped to 80 columns, obviously)

dperny · 2018-09-13T18:23:08Z

+# which is not great, but that's all they expose so far to do this...
+.PHONY: ensure_sync_started
+ensure_sync_started:
+	@ kill -0 $$(cat .docker-sync/daemon.pid) 2&> /dev/null || docker-sync start


can you add a comment explaining in a nutshell what this does? something along the lines of:

# checks if the daemon pid in the .docker-sync directory maps # to the present daemon. if so, then...

...and so-on.

dperny · 2018-09-13T18:27:35Z

+	@go get -u github.com/gordonklaus/ineffassign
+	@go get -u github.com/client9/misspell/cmd/misspell
+	@go get -u github.com/lk4d4/vndr
+	@go get -u github.com/stevvooe/protobuild


nit: it may be handy, if we're going to use docker-sync, to include installation in the makefile, either here or in the other Makefile. Not necessarily as part of the setup target, if you don't think that's appropriate.

this is not a blocker, and if you decline to make this change, i'm unopposed to merging as-is.

We run make setup in the Dockerfile, so it's already baked into the image

does docker-sync not have a client-side component?

Oooh. Misunderstood you. It sure does. It comes as a gem. The idea here is that people who want to use it need to install it beforehand though. I don't want to go down the rabbithole of rvm and chruby and what not...

Credit goes to dperny (moby#2687) **- What I did** Adds a Dockerfile for the swarmkit project, to easily get off the ground. Modifies the Makefile to make intelligent use of Docker. Also made small clean up changes to the Makefile. **- How I did it** Modifies the Makefile to have two paths: containerized.mk, which builds the docker image and forwards any make targets to a container, and direct.mk, which encompasses the old Makefile's workflow. By default, nothing will run inside a container. Set the environment variable `DOCKER_SWARMKIT_USE_CONTAINER` to use dockerized making. Also leverages docker-sync for synchronizing code to the container if the `DOCKER_SWARMKIT_USE_DOCKER_SYNC` env variable is set; comes in handy on Macs, for example. **- How to test it** Set `DOCKER_SWARMKIT_USE_CONTAINER` and verify that your favorite make targets all work! Signed-off-by: Jean Rouge <jer329@cornell.edu>