install bash-completion in dev container #5232
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5232 +/- ##
=======================================
Coverage 61.48% 61.49%
=======================================
Files 298 298
Lines 20811 20811
=======================================
+ Hits 12795 12797 +2
+ Misses 7103 7102 -1
+ Partials 913 912 -1 |
Makefile
Outdated
| .PHONY: completion | ||
| completion: binary ## install the generated bash-completion script in the dev container | ||
| docker completion bash > /etc/bash_completion.d/docker |
There was a problem hiding this comment.
does this need to be a separate make command?
Wondering if we could include the completion scripts (for all shells) inside the dev container by default.
There was a problem hiding this comment.
I was considering that, but we currently only have Bash installed. I think
we can consider adding more shells (but need to look at how much extra
time (and space) that would take by default to start the dev-container).
But yes, we can already split targets to prepare for that; I pushed an
update that splits the bash part to a separate make target 👍 PTAL
It's not initialized, because there's no `docker` command installed by default, but at least this makes sure that the basics are present for testing. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Makefile
Outdated
| completion: /etc/bash_completion.d/docker | ||
| completion: ## generate and install the completion scripts | ||
|
|
||
| /etc/bash_completion.d/docker: FORCE ## generate and install the bash-completion script |
There was a problem hiding this comment.
I think we can just use .PHONY here:
| /etc/bash_completion.d/docker: FORCE ## generate and install the bash-completion script | |
| .PHONY: /etc/bash_completion.d/docker | |
| /etc/bash_completion.d/docker: |
There was a problem hiding this comment.
Docs seem to support using PHONY
Using ‘.PHONY’ is more explicit and more efficient. However, other versions of make do not support ‘.PHONY’; thus ‘FORCE’ appears in many makefiles.
https://www.gnu.org/software/make/manual/html_node/Force-Targets.html
There was a problem hiding this comment.
Ah, yeah, both should work here; I picked FORCE to make it explicit it's not a "fake" target, so an actual file, but would still have to be forced; I had some recollection hat was the preferred way somewhere, but can't find that back (maybe it was some discussion in another project).
| completion: ## generate and install the completion scripts | ||
|
|
||
| /etc/bash_completion.d/docker: FORCE ## generate and install the bash-completion script | ||
| docker completion bash > /etc/bash_completion.d/docker |
There was a problem hiding this comment.
We should also make sure that the directory exists:
| docker completion bash > /etc/bash_completion.d/docker | |
| mkdir -p /etc/bash_completion.d | |
| docker completion bash > /etc/bash_completion.d/docker |
There was a problem hiding this comment.
Good one; I wrote it with the dev-container in mind (which had it set up already), but doesn't hurt to add
Add a "completion" target to install the generated completion scripts inside the dev-container. As generating this script depends on the docker binary, it calls "make binary" first. Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
thx! I'll bring this one in, and rebase #5238 |
Dockerfile.dev: install bash-completion in dev container
It's not initialized, because there's no
dockercommand installedby default, but at least this makes sure that the basics are present
for testing.
Makefile: add completion target
Add a "completion" target to install the generated completion
scripts inside the dev-container. As generating this script
depends on the docker binary, it calls "make binary" first.
- A picture of a cute animal (not mandatory but encouraged)