Completion for events --filter

[albers]

Adds completion for docker events --filter.

There are different types of filters, requiring different completion logic.
Some filters have static values, others require API lookups.
In some cases, trailing spaces of intermediate completions have to be suppressed.

The completion logic is complex because cobra does not offer support for compound flag values.
The handler for the --filter flag serves as the central entrypoint to all completions.

How to verify:
in a dev container, run make completion, then trigger completions, e.g.

$ docker events --filter <TAB><TAB>
container=  event=      label=      node=       type=       
daemon=     image=      network=    scope=      volume= 
$ docker events --filter event=c<TAB><TAB>
checkpoint  commit      connect     copy        create
$ docker events --filter image=<TAB><TAB>
docker-cli-dev:latest  docker-cli-e2e:latest  registry:2

The completions should correspond to those of the legacy bash completion.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 70.37037% with 32 lines in your changes missing coverage. Please review.

Project coverage is 59.64%. Comparing base (21eea1e) to head (e1c5180).
Report is 183 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5538      +/-   ##
==========================================
- Coverage   60.67%   59.64%   -1.03%     
==========================================
  Files         345      346       +1     
  Lines       23491    29211    +5720     
==========================================
+ Hits        14252    17422    +3170     
- Misses       8266    10819    +2553     
+ Partials      973      970       -3     

[thaJeztah]

Thank you for working on this! It's really great to see you start doing so (and much appreciated!). I initially had some reservations on the Cobra completion (and the initial set of completion was fairly limited), but they start to shape up.

I like where this PR is going; I gave it a quick try, and it's already pretty nice;

root@docker-cli-dev$ docker events --filter
container=  daemon=     event=      image=      label=      network=    node=       scope=      type=       volume=
root@docker-cli-dev$ docker events --filter container=
empty            foo              hello2           kind_wing        loving_mccarthy
root@docker-cli-dev$ docker events --filter container=foo --filter network=
bridge           docker_gwbridge  host             ingress          none
root@docker-cli-dev$ docker events --filter container=foo --filter network=

Some things that I didn't give much thought yet;

• (honestly) our filter flags are a bit messy and behavior not always well-defined (how do combinations of filters .. combine?)
• Also some quirks like some filters allowing "exclusions" (foo!=bar) and others don't, and this may also differ between commands (:disappointed:)
• ☝️ we need to spend time documenting all filters and options

Also "FYI" we were discussion potentially adding (an) API endpoint(s) dedicated to completion; nothing set in stone yet, and still to be discussed further, but the idea was that such an endpoint could move some of the heavy-lifting to the daemon, and provide a more optimized response for purpose of completion (e.g. if all we need is "names", there's no need to collect all the other bits). Such endpoint(s) could be more lightweight and perhaps more flexible / opinionated as their purpose is clear.

That discussion was somewhat related to #5528, which is a really nice feature, but also had some implications; it would mean the CLI now had to integrate with a Docker Hub specific API (the endpoints used are not part of the OCI distribution spec), but also could complicate situations where the daemon is configured to use a proxy, is airgapped, or is using a registry mirror. Having an (extensible) endpoint defined in the API could mean that such actions could be handled on the daemon-side (which on (e.g.) Docker Desktop could mean a service handling this).

[thaJeztah]

Oh! Perhaps we should have some utility for this; we define lists of these now, but they are strongly typed (which should still be fine, as they're just a string under the hood);

cli/vendor/github.com/docker/docker/api/types/events/events.go

Lines 7 to 100 in 3590f94

	// List of known event types.
	const (
	BuilderEventType Type = "builder" // BuilderEventType is the event type that the builder generates.
	ConfigEventType Type = "config" // ConfigEventType is the event type that configs generate.
	ContainerEventType Type = "container" // ContainerEventType is the event type that containers generate.
	DaemonEventType Type = "daemon" // DaemonEventType is the event type that daemon generate.
	ImageEventType Type = "image" // ImageEventType is the event type that images generate.
	NetworkEventType Type = "network" // NetworkEventType is the event type that networks generate.
	NodeEventType Type = "node" // NodeEventType is the event type that nodes generate.
	PluginEventType Type = "plugin" // PluginEventType is the event type that plugins generate.
	SecretEventType Type = "secret" // SecretEventType is the event type that secrets generate.
	ServiceEventType Type = "service" // ServiceEventType is the event type that services generate.
	VolumeEventType Type = "volume" // VolumeEventType is the event type that volumes generate.
	)
	// Action is used for event-actions.
	type Action string
	const (
	ActionCreate Action = "create"
	ActionStart Action = "start"
	ActionRestart Action = "restart"
	ActionStop Action = "stop"
	ActionCheckpoint Action = "checkpoint"
	ActionPause Action = "pause"
	ActionUnPause Action = "unpause"
	ActionAttach Action = "attach"
	ActionDetach Action = "detach"
	ActionResize Action = "resize"
	ActionUpdate Action = "update"
	ActionRename Action = "rename"
	ActionKill Action = "kill"
	ActionDie Action = "die"
	ActionOOM Action = "oom"
	ActionDestroy Action = "destroy"
	ActionRemove Action = "remove"
	ActionCommit Action = "commit"
	ActionTop Action = "top"
	ActionCopy Action = "copy"
	ActionArchivePath Action = "archive-path"
	ActionExtractToDir Action = "extract-to-dir"
	ActionExport Action = "export"
	ActionImport Action = "import"
	ActionSave Action = "save"
	ActionLoad Action = "load"
	ActionTag Action = "tag"
	ActionUnTag Action = "untag"
	ActionPush Action = "push"
	ActionPull Action = "pull"
	ActionPrune Action = "prune"
	ActionDelete Action = "delete"
	ActionEnable Action = "enable"
	ActionDisable Action = "disable"
	ActionConnect Action = "connect"
	ActionDisconnect Action = "disconnect"
	ActionReload Action = "reload"
	ActionMount Action = "mount"
	ActionUnmount Action = "unmount"
	// ActionExecCreate is the prefix used for exec_create events. These
	// event-actions are commonly followed by a colon and space (": "),
	// and the command that's defined for the exec, for example:
	//
	// exec_create: /bin/sh -c 'echo hello'
	//
	// This is far from ideal; it's a compromise to allow filtering and
	// to preserve backward-compatibility.
	ActionExecCreate Action = "exec_create"
	// ActionExecStart is the prefix used for exec_create events. These
	// event-actions are commonly followed by a colon and space (": "),
	// and the command that's defined for the exec, for example:
	//
	// exec_start: /bin/sh -c 'echo hello'
	//
	// This is far from ideal; it's a compromise to allow filtering and
	// to preserve backward-compatibility.
	ActionExecStart Action = "exec_start"
	ActionExecDie Action = "exec_die"
	ActionExecDetach Action = "exec_detach"
	// ActionHealthStatus is the prefix to use for health_status events.
	//
	// Health-status events can either have a pre-defined status, in which
	// case the "health_status" action is followed by a colon, or can be
	// "free-form", in which case they're followed by the output of the
	// health-check output.
	//
	// This is far form ideal, and a compromise to allow filtering, and
	// to preserve backward-compatibility.
	ActionHealthStatus Action = "health_status"
	ActionHealthStatusRunning Action = "health_status: running"
	ActionHealthStatusHealthy Action = "health_status: healthy"
	ActionHealthStatusUnhealthy Action = "health_status: unhealthy"
	)

Considering some options for that;

We could create slice for all event.Type and event.Action

var Types = []Type{
	ContainerEventType,
	ImageEventType,
	// ...
}

var Actions = []Action{
	ActionCreate,
	ActionStart,
	// ...
}

And/or create a struct with a slice of actions per type, e.g. something like;

var PerType = map[Type][]Action{
	ContainerEventType: {
		ActionCreate, ActionAttach, ActionDelete,
	},
	ImageEventType: {
		ActionCreate, ActionDelete,
	},
}

Or a utility function for that can provide this;

func ForType(t Type) []Action {
	if string(t) == "all" {
		return AllActions
	}

	return PerType[t]
}

[albers]

This would be added in the moby codebase?

[thaJeztah]

Yes, correct; we vendor those types from the Moby repo.

Converting to a string (or slice of strings) would then probably be left to the consumer (so that's something we can do in this repository).

[thaJeztah]

But before those changes are made there, we could implement them locally here as non-exported / private utilities (and see what a good "shape" would be), then make the change in moby, and once those are vendored here, we can swap the local ones; similar to how #5480 swapped a local construct to using the new module to provide those

[albers]

Great. I'll make a suggestion. This might take some time, though.
Thanks for your support.

[albers]

done, PTAL.

[albers]

@thaJeztah I added completions for the remaining filters.
At that stage, the completeFilters was way to long and ugly, so I decided to introduce some helper functions.
These helper functions simplify error handling very much, as they just return empty arrays in case of API errors.
PTAL, IMHO the PR should be complete now.

[thaJeztah]

Nice!! I was wondering the other day if something like this would work. I wanted to experiment if completion would be possible for the "advanced" CSV flags, but didn't give it a try yet.

[albers]

@thaJeztah I added tests for those commands that call the API.
Please take a look before I clean up the commits and publish the PR.

[albers]

The PR is ready for review now.

[vvoland]

LGTM, thanks!

[thaJeztah]

This looks great! Sorry for the delay; I apparently started a review, and didn't submit 🙈

I left some suggestions for consideration.

[thaJeztah]

We could probably optimize this to prevent matching prefix multiple times;

// completeEventFilters provides completion for the filters that can be used with `--filter`.
func completeEventFilters(dockerCLI completion.APIClientProvider) completion.ValidArgsFn {
	return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		key, _, ok := strings.Cut(toComplete, "=")
		if !ok {
			return postfixWith("=", eventFilters), cobra.ShellCompDirectiveNoSpace
		}
		switch key {
		case "container":
			return prefixWith("container=", containerNames(dockerCLI, cmd, args, toComplete)), cobra.ShellCompDirectiveNoFileComp
		case "daemon":
			return prefixWith("daemon=", daemonNames(dockerCLI, cmd)), cobra.ShellCompDirectiveNoFileComp
		case "event":
			return prefixWith("event=", validEventNames()), cobra.ShellCompDirectiveNoFileComp
		case "image":
			return prefixWith("image=", imageNames(dockerCLI, cmd)), cobra.ShellCompDirectiveNoFileComp
		case "label":
			return nil, cobra.ShellCompDirectiveNoFileComp
		case "network":
			return prefixWith("network=", networkNames(dockerCLI, cmd)), cobra.ShellCompDirectiveNoFileComp
		case "node":
			return prefixWith("node=", nodeNames(dockerCLI, cmd)), cobra.ShellCompDirectiveNoFileComp
		case "scope":
			return prefixWith("scope=", []string{"local", "swarm"}), cobra.ShellCompDirectiveNoFileComp
		case "type":
			return prefixWith("type=", eventTypeNames()), cobra.ShellCompDirectiveNoFileComp
		case "volume":
			return prefixWith("volume=", volumeNames(dockerCLI, cmd)), cobra.ShellCompDirectiveNoFileComp
		default:
			return postfixWith("=", eventFilters), cobra.ShellCompDirectiveNoSpace
		}
	}
}

I was even contemplating if we should re-purpose the constants where suitable, but perhaps confusing, because not all filters refer to a type; i.e., was considering;

case string(events.ContainerEventType):

But not sure if that's a good thing to do

[albers]

That's much better, thank you. I did not go for the constants because I think this produces asymmetry that makes the code harder to comprehend.

[thaJeztah]

Yes, makes sense; I started to replace the strings with the consts, then realised not all of them were event-types 😂

[albers]

@thaJeztah Thanks for the review, I applied all of your suggestions. PTAL.

[thaJeztah]

@thaJeztah Thanks for the review, I applied all of your suggestions. PTAL.

Thank you! Changes LGTM, but it's ok to squash my suggestions with the first commit (we don't need the history of the "review comments applied"). Could you squash the first and last commit?

[albers]

Could you squash the first and last commit?

done.

[thaJeztah]

LGTM, thank you!!