Skip to content

Conversation

@wk8
Copy link
Contributor

@wk8 wk8 commented Nov 12, 2018

- What I did

This patch provides some way for non-Linux devs to work on this repo without
suffering too much from the notorious terrible I/O perf of mounted volumes (eg
on OS X).

- How I did it

We now allow to override the volume mounts and shell for the dev container
through the DOCKER_CLI_MOUNTS and DOCKER_CLI_SHELL env variables.
We also allow setting the dev container name through the
DOCKER_CLI_CONTAINER_NAME env var.

The motivation for allowing overriding the volume mounts is the same as for
moby/moby#37845, namely that I/O perf on native mounted
disks on non-Linux (notably Mac OS) is just terrible, thus making it a real
pain to develop: one has to choose between re-building the image after every
single change (eg to run a test) or just work directly inside the same
container (eg with vim, but even then one would have to re-configure their dev
container every time it gets destroyed - containers, after all, are not
supposed to be long-lived).

Allowing to override DOCKER_CLI_MOUNTS makes it easy for everyone
to decide what their volume/syncing strategy is; for example
one can choose to use docker-sync.

As for the shell, it's nice to be able to use bash instead of the more
bare-bones ash if preferred.

Finally, being able to name the container can come in handy for easier
scripting on the host.

This patch won't change anything for anyone who doesn't set these env variables
in their environment.

- How to verify it

The output of:

make -f docker.Makefile -n shell

should be unchanged compared to what it was before this patch; but:

DOCKER_CLI_MOUNTS='-v test:/test' make -f docker.Makefile -n shell

should replace the -v flag from the command above with just
-v test:/test, and finally:

DOCKER_CLI_MOUNTS=' ' make -f docker.Makefile -n shell

should be devoid of any -v flags (besides the socket one).

All the make targets should be unchanged when not setting any of the
new env vars.

- A picture of a cute animal (not mandatory but encouraged)

bunny

@codecov-io
Copy link

codecov-io commented Nov 12, 2018

Codecov Report

Merging #1511 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1511   +/-   ##
=======================================
  Coverage   55.32%   55.32%           
=======================================
  Files         283      283           
  Lines       19330    19330           
=======================================
  Hits        10694    10694           
  Misses       7939     7939           
  Partials      697      697

@wk8 wk8 force-pushed the wk8/small_makefile_tweaks branch from 869088e to 53b9f4b Compare November 12, 2018 13:52
@wk8 wk8 force-pushed the wk8/small_makefile_tweaks branch from 6de0220 to ebf42f7 Compare January 9, 2019 23:31
@wk8
Copy link
Contributor Author

wk8 commented Jan 9, 2019

Small review please? :)

…container

Through the `DOCKER_CLI_MOUNTS` and `DOCKER_CLI_SHELL` env variables. Also
allows setting the dev container name through the `DOCKER_CLI_CONTAINER_NAME`
env var.

The motivation for allowing overriding the volume mounts is the same as for
moby/moby#37845, namely that I/O perf on native mounted
disks on non-Linux (notably Mac OS) is just terrible, thus making it a real
pain to develop: one has to choose between re-building the image after every
single change (eg to run a test) or just work directly inside the same
container (eg with vim, but even then one would have to re-configure their dev
container every time it gets destroyed - containers, after all, are not
supposed to be long-lived).

Allowing to override DOCKER_CLI_MOUNTS makes it easy for everyone
to decide what their volume/syncing strategy is; for example
one can choose to use [docker-sync](https://github.com/EugenMayer/docker-sync).

As for the shell, it's nice to be able to use bash instead of the more
bare-bones `ash` if preferred.

Finally, being able to name the container can come in handy for easier
scripting on the host.

This patch won't change anything for anyone who doesn't set these env variables
in their environment.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
@wk8 wk8 force-pushed the wk8/small_makefile_tweaks branch from e75d976 to b039db9 Compare January 14, 2019 18:40
@mavenugo mavenugo requested a review from ijc February 15, 2019 20:22
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Sounds good to me, if it helps developing on the docker/cli repo 👍

@andrewhsu
Copy link
Contributor

SGTM as well!

@wk8
Copy link
Contributor Author

wk8 commented Feb 16, 2019

Thanks @andrewhsu and @silvin-lubecki for the review :) What's the process to get it merged at this point?

Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit 35c39d3 into docker:master Feb 21, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Feb 21, 2019
@wk8
Copy link
Contributor Author

wk8 commented Feb 21, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants