Make it possible to override the volume mounts and shell for the dev container#1511
Merged
cpuguy83 merged 1 commit intodocker:masterfrom Feb 21, 2019
Merged
Conversation
82f41a5 to
869088e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1511 +/- ##
=======================================
Coverage 55.32% 55.32%
=======================================
Files 283 283
Lines 19330 19330
=======================================
Hits 10694 10694
Misses 7939 7939
Partials 697 697 |
869088e to
53b9f4b
Compare
6de0220 to
ebf42f7
Compare
Contributor
Author
|
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>
e75d976 to
b039db9
Compare
silvin-lubecki
approved these changes
Feb 15, 2019
Contributor
silvin-lubecki
left a comment
There was a problem hiding this comment.
Sounds good to me, if it helps developing on the docker/cli repo 👍
Contributor
|
SGTM as well! |
Contributor
Author
|
Thanks @andrewhsu and @silvin-lubecki for the review :) What's the process to get it merged at this point? |
Contributor
Author
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- 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_MOUNTSandDOCKER_CLI_SHELLenv variables.We also allow setting the dev container name through the
DOCKER_CLI_CONTAINER_NAMEenv 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
ashif 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:
should be unchanged compared to what it was before this patch; but:
should replace the
-vflag from the command above with just-v test:/test, and finally:should be devoid of any
-vflags (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)