Fix unknown $(MOUNTS) variable in makefile plugins target#1698
Fix unknown $(MOUNTS) variable in makefile plugins target#1698thaJeztah merged 1 commit intodocker:masterfrom
Conversation
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Codecov Report
@@ Coverage Diff @@
## master #1698 +/- ##
==========================================
- Coverage 56.12% 56.12% -0.01%
==========================================
Files 306 306
Lines 21030 21025 -5
==========================================
- Hits 11803 11800 -3
+ Misses 8373 8371 -2
Partials 854 854 |
|
|
||
| plugins: build_binary_native_image ## build the CLI plugin examples | ||
| docker run --rm $(ENVVARS) $(MOUNTS) $(BINARY_NATIVE_IMAGE_NAME) ./scripts/build/plugins | ||
| docker run --rm $(ENVVARS) $(DOCKER_CLI_MOUNTS) $(BINARY_NATIVE_IMAGE_NAME) ./scripts/build/plugins |
There was a problem hiding this comment.
could we do the reverse, and remove the DOCKER_CLI_ prefix from all of these? This is the makefile for the CLI repository, so not sure why we need the prefix
There was a problem hiding this comment.
I also noticed the $DOCKER_CLI_SHELL variable was added; alpine images now have /bin/sh set by default (previously they didn't, which was perhaps the reason this argument was added), but we install bash as well @wk8 was that variable added to switch to bash? If so, perhaps we should update the Dockerfile, and make it the default?
There was a problem hiding this comment.
@thaJeztah the DOCKER_CLI prefix is for the variables which can be overriden with env variables. Allows setting them in eg a shell profile (and that's also why they are nicely set apart - https://github.com/wk8/cli/blob/2178fea84dbea0b278d412b5f9cf16e3542d2650/docker.Makefile#L7-L11)
There was a problem hiding this comment.
shouldn't those be set after make though?
make FOO=bar BAR=baz <target>
There was a problem hiding this comment.
That's one way to do it but
export FOO=bar
make <target>
or
FOO=bar make <target>
work just the same. (It's just a feature of make to parse env-variables-like arguments as such).
There was a problem hiding this comment.
As to $DOCKER_CLI_SHELL, yup I've set mine to bash --login so that it also loads my shell profile - just made it ash in #1511 to make the change seamless, but I don't any reason why we couldn't switch to bash.
There was a problem hiding this comment.
I'd be +1 to change it to bash by default, but we could do that separate
|
Ooops. Sorry. In my defense the |
Seems I rebased over b039db9 ("Make it possible to override the volume mounts and shell for the dev container") at some point and failed to notice that some of the variable names had changed. In the meantime the underlying issue was fixed in docker#1698 but here we switch to using `$(DOCKER_RUN)`. This means that these rules now use `$(DOCKER_RUN_NAME_OPTION)` and thus obey the `$(DOCKER_CLI_CONTAINER_NAME)` variable. Signed-off-by: Ian Campbell <ijc@docker.com>
5 participants
- What I did
Mounts were broken for plugins as #1511 replaced
$(MOUNTS)by$(DOCKER_CLI_MOUNTS).- A picture of a cute animal (not mandatory but encouraged)
