-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Completion for events --filter
#5538
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5538 +/- ##
==========================================
- Coverage 60.67% 59.66% -1.01%
==========================================
Files 345 346 +1
Lines 23491 29206 +5715
==========================================
+ Hits 14252 17426 +3174
- Misses 8266 10807 +2541
Partials 973 973 |
2d60765
to
a09c204
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
cli/command/system/completion.go
Outdated
|
||
var ( | ||
eventFilters = []string{"container", "daemon", "event", "image", "label", "network", "node", "scope", "type", "volume"} | ||
eventNames = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be added in the moby codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I'll make a suggestion. This might take some time, though.
Thanks for your support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
cli/command/system/completion.go
Outdated
if strings.HasPrefix(toComplete, "container=") { | ||
// the pure container name list should be pulled out from ContainerNames. | ||
names, _ := completion.ContainerNames(dockerCLI, true)(cmd, args, toComplete) | ||
return prefixWith("container=", names), cobra.ShellCompDirectiveDefault | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cli/command/system/completion.go
Outdated
return nil, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
if strings.HasPrefix(toComplete, "network=") { | ||
names, err := completion.NetworkNames(dockerCLI, cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly on the fence on the completion
package now having both completion.NetworkNames
and completion.CompleteNetworks
. Or at least, I"m looking if we can reduce exported functions, unless needed.
Looking at the code in NetworkNames
, it's just a few lines, and fairly straightforward. I'd be fine with copying that code here as a un-exported function (for now). It may be good to include a reference to the "original" one to correlate them, e.g.;
// networkNames contacts the API to get a list of networks.
//
// This function is based on [completion.NetworkNames].
func NetworkNames(dockerCLI APIClientProvider, cmd *cobra.Command) ([]string, error) {
summaries, err := dockerCLI.Client().NetworkList(cmd.Context(), network.ListOptions{})
if err != nil {
return nil, err
}
var names []string
for _, nws := range summaries {
names = append(names, nws.Name)
}
return names, nil
}
That would still allow us to change our mind in future of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would still allow us to change our mind in future of course!
Expanding a bit on that (just to echo my train of thought ❤️); with cli-plugins, like compose, buildx, and others, we may need to think what to put "where" at some point. We didn't discuss that in-depth when we added the initial implementation of cobra completion, but sometimes it can be "complicated" to have the code live together with other code.
So, possibly at some point we may need to re-organise some of the functions to be exported (allowing them to be consumed by compose and buildx, etc), but without that meaning they come with a large number of dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly on the fence on the completion package now having both completion.NetworkNames and completion.CompleteNetworks.
I agree this feels a bit overkill for the desired effect.
Looking at the code in
NetworkNames
, it's just a few lines, and fairly straightforward. I'd be fine with copying that code here as a un-exported function (for now).
For NetworkNames
this would be a good solution. But for ContainerNames
, this would mean a lot of duplicated business logic.
What about keeping the hacky version from before the refactoring:
if strings.HasPrefix(toComplete, "container=") {
names, _ := completion.ContainerNames(dockerCLI, true)(cmd, args, toComplete)
return prefixWith("container=", names), cobra.ShellCompDirectiveDefault
}
if strings.HasPrefix(toComplete, "network=") {
names, _ := completion.NetworkNames(dockerCLI)(cmd, args, toComplete)
return prefixWith("network=", names), cobra.ShellCompDirectiveDefault
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about keeping the hacky version from before the refactoring:
Oh, I think that's definitely acceptable for now. Once we figure out alternatives, we can easily swap out those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted to the previous version and added error handling for network and container completion to avoid filenames being completed in case of an error in the api calls.
3306b98
to
3f3ff40
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b650f81
to
95c88b2
Compare
@thaJeztah I added tests for those commands that call the API. |
95c88b2
to
b0581cf
Compare
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
e49d3a8
to
2926d0f
Compare
The PR is ready for review now. |
events --filter
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.The completions should correspond to those of the legacy bash completion.