various improvements to shell completions #5238

thaJeztah · 2024-07-05T14:44:04Z

follow-up to / stacked on install bash-completion in dev container #5232

cli/command/completion: add FileNames utility

This is just a convenience function to allow defining completion to
use the default (complete with filenames and directories).

cli/command/completion: add EnvVarNames utility

EnvVarNames offers completion for environment-variable names. This
completion can be used for "--env" and "--build-arg" flags, which
allow obtaining the value of the given environment-variable if present
in the local environment, so we only should complete the names of the
environment variables, and not their value. This also prevents the
completion script from printing values of environment variables
containing sensitive values.

For example;

export MY_VAR=hello
docker run --rm --env MY_VAR alpine printenv MY_VAR
hello

Before this patch:

docker run --env GO
GO111MODULE=auto        GOLANG_VERSION=1.21.12  GOPATH=/go              GOTOOLCHAIN=local

With this patch:

docker run --env GO<tab>
GO111MODULE     GOLANG_VERSION  GOPATH          GOTOOLCHAIN

cli/command/completion: add FromList utility

It's an alias for cobra.FixedCompletions but takes a variadic list
of strings, so that it's not needed to construct an array for this.

cli/command/container: provide flag-completion for "docker create"

"docker run" and "docker create" are mostly identical, so we can copy
the same completion functions,

We could possibly create a utility for this (similar to addFlags() which
configures both commands with the flags they share). I considered combining
his with addFlags(), but that utility is also used in various tests, in
which we don't need this feature, so keeping that for a future exercise.

cmd/docker: fix completion for --context

registerCompletionFuncForGlobalFlags was called from newDockerCommand,
at which time no context-store is initialized yet, so it would return
a nil value, probably resulting in store.Names to panic, but these
errors are not shown when running the completion. As a result, the flag
completion would fall back to completing from filenames.

This patch changes the function to dynamically get the context-store;
this fixes the problem mentioned above, because at the time the completion
function is invoked, the CLI is fully initialized, and does have a
context-store available.

A (non-exported) interface is defined to allow the function to accept
alternative implementations (not requiring a full command.DockerCLI).

Before this patch:

docker context create one
docker context create two

docker --context <TAB>
.DS_Store                   .idea/                      Makefile
.dockerignore               .mailmap                    build/
...

With this patch:

docker context create one
docker context create two

docker --context <TAB>
default  one      two

cli/context/store: Names(): fix panic when called with nil-interface

Before this, it would panic when a nil-interface was passed.

cli/command/container: add completion for --cap-add, --cap-drop

With this patch:

docker run --cap-add <TAB>
ALL                     CAP_KILL                CAP_SETUID
CAP_AUDIT_CONTROL       CAP_LEASE               CAP_SYSLOG
CAP_AUDIT_READ          CAP_LINUX_IMMUTABLE     CAP_SYS_ADMIN
CAP_AUDIT_WRITE         CAP_MAC_ADMIN           CAP_SYS_BOOT
CAP_BLOCK_SUSPEND       CAP_MAC_OVERRIDE        CAP_SYS_CHROOT
CAP_BPF                 CAP_MKNOD               CAP_SYS_MODULE
CAP_CHECKPOINT_RESTORE  CAP_NET_ADMIN           CAP_SYS_NICE
CAP_CHOWN               CAP_NET_BIND_SERVICE    CAP_SYS_PACCT
CAP_DAC_OVERRIDE        CAP_NET_BROADCAST       CAP_SYS_PTRACE
CAP_DAC_READ_SEARCH     CAP_NET_RAW             CAP_SYS_RAWIO
CAP_FOWNER              CAP_PERFMON             CAP_SYS_RESOURCE
CAP_FSETID              CAP_SETFCAP             CAP_SYS_TIME
CAP_IPC_LOCK            CAP_SETGID              CAP_SYS_TTY_CONFIG
CAP_IPC_OWNER           CAP_SETPCAP             CAP_WAKE_ALARM

cli/command/container: add completion for --restart

With this patch:

docker run --restart <TAB>
always  no  on-failure  unless-stopped

cli/command/container: add completion for --volumes-from

With this patch:

docker run --volumes-from amazing_nobel
amazing_cannon     boring_wozniak         determined_banzai
elegant_solomon    reverent_booth         amazing_nobel

cli/command/container: add completion for --stop-signal

With this patch:

docker run --stop-signal <TAB>
ABRT  IOT      RTMAX-4   RTMIN     RTMIN+11  TSTP
ALRM  KILL     RTMAX-5   RTMIN+1   RTMIN+12  TTIN
BUS   PIPE     RTMAX-6   RTMIN+2   RTMIN+13  TTOU
CHLD  POLL     RTMAX-7   RTMIN+3   RTMIN+14  URG
CLD   PROF     RTMAX-8   RTMIN+4   RTMIN+15  USR1
CONT  PWR      RTMAX-9   RTMIN+5   SEGV      USR2
FPE   QUIT     RTMAX-10  RTMIN+6   STKFLT    VTALRM
HUP   RTMAX    RTMAX-11  RTMIN+7   STOP      WINCH
ILL   RTMAX-1  RTMAX-12  RTMIN+8   SYS       XCPU
INT   RTMAX-2  RTMAX-13  RTMIN+9   TERM      XFSZ
IO    RTMAX-3  RTMAX-14  RTMIN+10  TRAP

- Description for the changelog

add and improve shell completions for various flags

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

thaJeztah · 2024-07-05T14:46:23Z

cli/command/container/completion.go

+// This list was based on the containerd pkg/cap package;
+// https://github.com/containerd/containerd/blob/v1.7.19/pkg/cap/cap_linux.go#L133-L181
+//
+// TODO(thaJeztah): add descriptions, and enable descriptions for our completion scripts (cobra.CompletionOptions.DisableDescriptions is currently set to "true")


Need to check why we disable it here;

cli/cmd/docker/docker.go

Lines 92 to 96 in 9bb1a62

CompletionOptions: cobra.CompletionOptions{

DisableDefaultCmd: false,

HiddenDefaultCmd: true,

DisableDescriptions: true,

},

And here;

cli/cli-plugins/plugin/plugin.go

Lines 158 to 162 in 9bb1a62

CompletionOptions: cobra.CompletionOptions{

DisableDefaultCmd: false,

HiddenDefaultCmd: true,

DisableDescriptions: true,

},

codecov-commenter · 2024-07-05T14:46:47Z

Codecov Report

Attention: Patch coverage is 58.13953% with 18 lines in your changes missing coverage. Please review.

Project coverage is 61.48%. Comparing base (ce4469a) to head (b1c0ddc).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5238      +/-   ##
==========================================
+ Coverage   61.44%   61.48%   +0.03%     
==========================================
  Files         298      299       +1     
  Lines       20824    20831       +7     
==========================================
+ Hits        12796    12808      +12     
  Misses       7113     7113              
+ Partials      915      910       -5

thaJeztah · 2024-07-05T15:16:46Z

Ah! Forgot to remove something I previously used. Also looks like we're running some deprecated linters

#16 0.209 level=warning msg="[lintersdb] The linter named \"megacheck\" is deprecated. It has been split into: gosimple, staticcheck, unused."
#16 0.209 level=warning msg="[lintersdb] The name \"vet\" is deprecated. The linter has been renamed to: govet."
#16 65.71 cli/command/completion/functions.go:123:2: ineffectual assignment to toComplete (ineffassign)
#16 65.71 	toComplete = strings.ToLower(toComplete)
#16 65.71 	^

Benehiko

This looks really good, will be a nice improvement to the CLI UX :)

cli/command/container/completion.go

thaJeztah · 2024-07-10T15:06:54Z

Thanks for reviewing! I'll have a look at fixing the typo when I'm near my computer.

Also; wrt the "disable descriptions" #5238 (comment) it looks like buildx DOES enable them for its completion, so maybe we should enable it in our code as well; there may be some commands that could use a slight touch-up for those as some have descriptions that are a bit confusing, so still best to do in a follow up

thaJeztah · 2024-07-16T13:21:33Z

Thanks for reviewing! I'll have a look at fixing the typo when I'm near my computer.

(I see I didn't comment, but the typo was fixed 😂)

- explicitly suppress unhandled errors - remove names for unused arguments Signed-off-by: Sebastiaan van Stijn <[email protected]>

This is just a convenience function to allow defining completion to use the default (complete with filenames and directories). Signed-off-by: Sebastiaan van Stijn <[email protected]>

EnvVarNames offers completion for environment-variable names. This completion can be used for "--env" and "--build-arg" flags, which allow obtaining the value of the given environment-variable if present in the local environment, so we only should complete the names of the environment variables, and not their value. This also prevents the completion script from printing values of environment variables containing sensitive values. For example; export MY_VAR=hello docker run --rm --env MY_VAR alpine printenv MY_VAR hello Before this patch: docker run --env GO GO111MODULE=auto GOLANG_VERSION=1.21.12 GOPATH=/go GOTOOLCHAIN=local With this patch: docker run --env GO<tab> GO111MODULE GOLANG_VERSION GOPATH GOTOOLCHAIN Signed-off-by: Sebastiaan van Stijn <[email protected]>

It's an alias for cobra.FixedCompletions but takes a variadic list of strings, so that it's not needed to construct an array for this. Signed-off-by: Sebastiaan van Stijn <[email protected]>

"docker run" and "docker create" are mostly identical, so we can copy the same completion functions, We could possibly create a utility for this (similar to `addFlags()` which configures both commands with the flags they share). I considered combining his with `addFlags()`, but that utility is also used in various tests, in which we don't need this feature, so keeping that for a future exercise. Signed-off-by: Sebastiaan van Stijn <[email protected]>

registerCompletionFuncForGlobalFlags was called from newDockerCommand, at which time no context-store is initialized yet, so it would return a nil value, probably resulting in `store.Names` to panic, but these errors are not shown when running the completion. As a result, the flag completion would fall back to completing from filenames. This patch changes the function to dynamically get the context-store; this fixes the problem mentioned above, because at the time the completion function is _invoked_, the CLI is fully initialized, and does have a context-store available. A (non-exported) interface is defined to allow the function to accept alternative implementations (not requiring a full command.DockerCLI). Before this patch: docker context create one docker context create two docker --context <TAB> .DS_Store .idea/ Makefile .dockerignore .mailmap build/ ... With this patch: docker context create one docker context create two docker --context <TAB> default one two Signed-off-by: Sebastiaan van Stijn <[email protected]>

Before this, it would panic when a nil-interface was passed. Signed-off-by: Sebastiaan van Stijn <[email protected]>

With this patch: docker run --cap-add <TAB> ALL CAP_KILL CAP_SETUID CAP_AUDIT_CONTROL CAP_LEASE CAP_SYSLOG CAP_AUDIT_READ CAP_LINUX_IMMUTABLE CAP_SYS_ADMIN CAP_AUDIT_WRITE CAP_MAC_ADMIN CAP_SYS_BOOT CAP_BLOCK_SUSPEND CAP_MAC_OVERRIDE CAP_SYS_CHROOT CAP_BPF CAP_MKNOD CAP_SYS_MODULE CAP_CHECKPOINT_RESTORE CAP_NET_ADMIN CAP_SYS_NICE CAP_CHOWN CAP_NET_BIND_SERVICE CAP_SYS_PACCT CAP_DAC_OVERRIDE CAP_NET_BROADCAST CAP_SYS_PTRACE CAP_DAC_READ_SEARCH CAP_NET_RAW CAP_SYS_RAWIO CAP_FOWNER CAP_PERFMON CAP_SYS_RESOURCE CAP_FSETID CAP_SETFCAP CAP_SYS_TIME CAP_IPC_LOCK CAP_SETGID CAP_SYS_TTY_CONFIG CAP_IPC_OWNER CAP_SETPCAP CAP_WAKE_ALARM Signed-off-by: Sebastiaan van Stijn <[email protected]>

With this patch: docker run --restart <TAB> always no on-failure unless-stopped Signed-off-by: Sebastiaan van Stijn <[email protected]>

With this patch: docker run --volumes-from amazing_nobel amazing_cannon boring_wozniak determined_banzai elegant_solomon reverent_booth amazing_nobel Signed-off-by: Sebastiaan van Stijn <[email protected]>

With this patch: docker run --stop-signal <TAB> ABRT IOT RTMAX-4 RTMIN RTMIN+11 TSTP ALRM KILL RTMAX-5 RTMIN+1 RTMIN+12 TTIN BUS PIPE RTMAX-6 RTMIN+2 RTMIN+13 TTOU CHLD POLL RTMAX-7 RTMIN+3 RTMIN+14 URG CLD PROF RTMAX-8 RTMIN+4 RTMIN+15 USR1 CONT PWR RTMAX-9 RTMIN+5 SEGV USR2 FPE QUIT RTMAX-10 RTMIN+6 STKFLT VTALRM HUP RTMAX RTMAX-11 RTMIN+7 STOP WINCH ILL RTMAX-1 RTMAX-12 RTMIN+8 SYS XCPU INT RTMAX-2 RTMAX-13 RTMIN+9 TERM XFSZ IO RTMAX-3 RTMAX-14 RTMIN+10 TRAP Signed-off-by: Sebastiaan van Stijn <[email protected]>

vvoland

LGTM

vvoland · 2024-07-17T08:39:24Z

cli/command/container/completion.go

+// https://github.com/containerd/containerd/blob/v1.7.19/pkg/cap/cap_linux.go#L133-L181
+//
+// TODO(thaJeztah): add descriptions, and enable descriptions for our completion scripts (cobra.CompletionOptions.DisableDescriptions is currently set to "true")
+var allLinuxCapabilities = []string{


Looks like containerd/pkg/cap maintains a list of these: https://github.com/containerd/containerd/blob/78d3e205a51ec101f775a43bee6f4fdd8fc6b22b/pkg/cap/cap_linux.go#L133-L187

Perhaps we could help extracting this from pkg to a separate repo that we could consume?

(not a blocker for this PR though)

Looks like containerd/pkg/cap maintains a list of these:

Hehe, yes, look at the comment above; that's where I took the list from 😄.

We could consider moving such things to a separate repo (or to be in a smaller module); one tricky bit there is that these are linux capabilities, so if we want to use them cross-platform (in our case, the CLI may be running on Windows), at least we'd have to make clear that they only apply to Linux, so we can't have that code be platform-specific at compile time (_linux.go e.g.).

Oh right, I didn't read the comment 🤦🏻 😄

Yeah, we can't use it in the current form, but would be nice to avoid having to maintain this list by ourselves in future.

Yeah agreed. For this one I decided that it's not critical for the list to be up-to-date (even more accurate; completion may provide capabilities that are not supported by the daemon host if it's running an older kernel version), but for completion that should be mostly "OK"; the user doesn't have to use the completion, and can still type something else.

For the --cap-add and --cap-rm cases, there's also the case that it would provide suggestions for capabilities that are either already set by default, or not present (both of which should be a no-op).

So the most accurate thing to do would be to have API endpoints for these (i.e., let the daemon provide options that can be used) that are specifically crafted for purpose of completion (as lightweight as possible), but even if they're lightweight, there may still be some latency when connected to a remove machine, so that's something we should explore and consider pros/cons.

thaJeztah · 2024-07-17T12:05:10Z

Looks like this test is flaky again (but different this time); I opened a tracking ticket; #5254

#20 60.67 === FAIL: cli/command/container TestInitTtySizeErrors (unknown)
#20 60.67 Error: write /dev/pts/0: file already closed
#20 60.67 panic: Fail in goroutine after TestRunAttachTermination has completed
#20 60.67 
#20 60.67 goroutine 216 [running]:
#20 60.67 testing.(*common).Fail(0xc00021eb60)
#20 60.67 	/usr/local/go/src/testing/testing.go:952 +0xd4
#20 60.67 testing.(*common).FailNow(0xc00021eb60)
#20 60.67 	/usr/local/go/src/testing/testing.go:975 +0x26
#20 60.67 github.com/docker/cli/vendor/gotest.tools/v3/assert.ErrorIs({0x1033010, 0xc00021eb60}, {0x102a300?, 0xc00049a0f0}, {0x102a3e0?, 0x16e0f30}, {0x0, 0x0, 0x0})
#20 60.67 	/go/src/github.com/docker/cli/vendor/gotest.tools/v3/assert/assert.go:311 +0x159
#20 60.67 github.com/docker/cli/cli/command/container.TestRunAttachTermination.func6()
#20 60.67 	/go/src/github.com/docker/cli/cli/command/container/run_test.go:85 +0x85
#20 60.67 created by github.com/docker/cli/cli/command/container.TestRunAttachTermination in goroutine 214
#20 60.67 	/go/src/github.com/docker/cli/cli/command/container/run_test.go:84 +0x486

thaJeztah added kind/enhancement status/2-code-review area/completion labels Jul 5, 2024

thaJeztah added this to the 28.0.0 milestone Jul 5, 2024

thaJeztah self-assigned this Jul 5, 2024

thaJeztah changed the title ~~Completion improvements~~ various improvements to shell completions Jul 5, 2024

thaJeztah commented Jul 5, 2024

View reviewed changes

thaJeztah force-pushed the completion_improvements branch 4 times, most recently from 233e5fc to 0644a0b Compare July 5, 2024 23:53

thaJeztah mentioned this pull request Jul 8, 2024

install bash-completion in dev container #5232

Merged

thaJeztah force-pushed the completion_improvements branch from 0644a0b to 21315da Compare July 8, 2024 17:22

Benehiko approved these changes Jul 10, 2024

View reviewed changes

cli/command/container/completion.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the completion_improvements branch from 21315da to 0e18564 Compare July 10, 2024 19:41

thaJeztah added 11 commits July 17, 2024 01:25

cli/command/container: NewRunCommand: slight cleanup of completion

eed0e5b

- explicitly suppress unhandled errors - remove names for unused arguments Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/command/completion: add FileNames utility

9207ff1

This is just a convenience function to allow defining completion to use the default (complete with filenames and directories). Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/command/completion: add FromList utility

5e7bcbe

It's an alias for cobra.FixedCompletions but takes a variadic list of strings, so that it's not needed to construct an array for this. Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/context/store: Names(): fix panic when called with nil-interface

e4dd8b1

Before this, it would panic when a nil-interface was passed. Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/command/container: add completion for --restart

7fe7223

With this patch: docker run --restart <TAB> always no on-failure unless-stopped Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/command/container: add completion for --volumes-from

d6f78cd

With this patch: docker run --volumes-from amazing_nobel amazing_cannon boring_wozniak determined_banzai elegant_solomon reverent_booth amazing_nobel Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the completion_improvements branch from 0e18564 to b1c0ddc Compare July 16, 2024 23:25

vvoland approved these changes Jul 17, 2024

View reviewed changes

thaJeztah mentioned this pull request Jul 17, 2024

Flaky test: TestInitTtySizeErrors / TestRunAttachTermination #5254

Open

thaJeztah merged commit 2da5f06 into docker:master Jul 17, 2024
88 checks passed

thaJeztah deleted the completion_improvements branch July 17, 2024 13:35

thaJeztah added the impact/changelog label Jul 17, 2024

thaJeztah mentioned this pull request Jul 18, 2024

[27.1 backport] assorted fixes and enhancements for shell-completion #5261

Merged

thaJeztah added the process/cherry-picked label Jul 19, 2024

thaJeztah mentioned this pull request Sep 26, 2024

cli/container: use github.com/moby/sys/capability for completions #5480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various improvements to shell completions #5238

various improvements to shell completions #5238

thaJeztah commented Jul 5, 2024 •

edited

Loading

thaJeztah Jul 5, 2024

codecov-commenter commented Jul 5, 2024 •

edited

Loading

thaJeztah commented Jul 5, 2024

Benehiko left a comment

thaJeztah commented Jul 10, 2024

thaJeztah commented Jul 16, 2024

vvoland left a comment

vvoland Jul 17, 2024

thaJeztah Jul 17, 2024

vvoland Jul 17, 2024

vvoland Jul 17, 2024

thaJeztah Jul 17, 2024

thaJeztah commented Jul 17, 2024

	CompletionOptions: cobra.CompletionOptions{
	DisableDefaultCmd: false,
	HiddenDefaultCmd: true,
	DisableDescriptions: true,
	},

various improvements to shell completions #5238

various improvements to shell completions #5238

Conversation

thaJeztah commented Jul 5, 2024 • edited Loading

cli/command/completion: add FileNames utility

cli/command/completion: add EnvVarNames utility

cli/command/completion: add FromList utility

cli/command/container: provide flag-completion for "docker create"

cmd/docker: fix completion for --context

cli/context/store: Names(): fix panic when called with nil-interface

cli/command/container: add completion for --cap-add, --cap-drop

cli/command/container: add completion for --restart

thaJeztah Jul 5, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jul 5, 2024 • edited Loading

Codecov Report

thaJeztah commented Jul 5, 2024

Benehiko left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 10, 2024

thaJeztah commented Jul 16, 2024

vvoland left a comment

Choose a reason for hiding this comment

vvoland Jul 17, 2024

Choose a reason for hiding this comment

thaJeztah Jul 17, 2024

Choose a reason for hiding this comment

vvoland Jul 17, 2024

Choose a reason for hiding this comment

vvoland Jul 17, 2024

Choose a reason for hiding this comment

thaJeztah Jul 17, 2024

Choose a reason for hiding this comment

thaJeztah commented Jul 17, 2024

thaJeztah commented Jul 5, 2024 •

edited

Loading

codecov-commenter commented Jul 5, 2024 •

edited

Loading