-
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
Show plugins as Management commands #1684
Show plugins as Management commands #1684
Conversation
@ijc @ulyssessouza WDYT? Thought I'd open this to discuss the best approach (slightly leaning towards the |
here's a #!/usr/bin/env bash
docker_cli_plugin_metadata() {
if [ -z "$DOCKER_COMPOSE_VERSION" ]; then
export DOCKER_COMPOSE_VERSION="$(docker-compose --version | cut -d " " -f 3 | cut -d "," -f 1)"
fi
local vendor="Docker"
local url="https://www.docker.com"
local description="Define and run multi-container applications"
cat <<-EOF
{"SchemaVersion":"0.1.0","Vendor":"${vendor}","Version":"${DOCKER_COMPOSE_VERSION}","ShortDescription":"${description}","URL":"${url}", "Subcommands":["build", "bundle", "up", "down"]}
EOF
}
case "$1" in
docker-cli-plugin-metadata)
docker_cli_plugin_metadata
;;
*)
exec "docker-$@"
;;
esac |
Codecov Report
@@ Coverage Diff @@
## master #1684 +/- ##
==========================================
- Coverage 56.12% 56.11% -0.01%
==========================================
Files 306 306
Lines 21025 21027 +2
==========================================
Hits 11800 11800
- Misses 8371 8373 +2
Partials 854 854 |
My initial thought was the boolean flag you mentioned, but listing the subcommands works too and I could see it being useful to completion perhaps (although given it doesn't work for sub-sub-commands maybe not?). Or an argument could be made that plugins are always management commands. If you go this route you should update |
perhaps the boolean would still be better (at least: for now); we haven't solved/designed the completion strategy yet, so adding subcommands because they "may" become handy might not be the best thing to do. If we agree that a cli by default is assumed to be a "management" command (which I think would be 99% of the cases), the boolean would only have to be set explicitly for a plugin that adds a new top-level command (which we overall "locked down"). If would still allow us to add a "subcommands" property to the metadata in future if there's a need for it |
2e2a293
to
d22a4b2
Compare
After some re-thinking, I updated this PR to unconditionally show plugins as management commands. There's currently not the intention to add more top-level commands, and the existing commands are either legacy ( |
whoops, didn't look at tests yet; will have to update one |
Maybe we can get #1675 in first and rebase this onto it? |
d22a4b2
to
5270632
Compare
Yes, I want to have that one in first; was waiting for @silvin-lubecki to approve it |
5270632
to
99b69c2
Compare
335ae1d
to
2c7b742
Compare
@@ -245,7 +248,7 @@ Management Commands: | |||
Commands: | |||
|
|||
{{- range operationSubCommands . }} | |||
{{rpad (decoratedName .) (add .NamePadding 1)}}{{.Short}}{{ if isPlugin .}} {{vendorAndVersion .}}{{ end}} | |||
{{rpad .Name .NamePadding }} {{.Short}} |
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.
Restored the original template for this line, as operationSubCommands
now no longer contains plugins
@ijc @silvin-lubecki rebased; and updated the template for |
Plugins are expected to be management commands ("docker <object> <verb>"). This patch modified the usage output to shown plugins in the "Management commands" section. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2c7b742
to
f8c5f5d
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.
LGTM 👍
a quick 5-minute attempt to make plugins with subcommands show as
Management
command, instead of as a top-level command. Ticks the ninth checkbox of #1661Plugins are expected to be management commands ("docker ").
This patch adds a dummy sub-command to the plugin stubs, so that they will
be shown in the "Management commands" section of the help/usage output: