Skip to content

Show plugins as Management commands - #1684

Merged
silvin-lubecki merged 1 commit into
docker:masterfrom
thaJeztah:plugin_subcommand_detection
Feb 26, 2019
Merged

Show plugins as Management commands#1684
silvin-lubecki merged 1 commit into
docker:masterfrom
thaJeztah:plugin_subcommand_detection

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Feb 21, 2019

Copy link
Copy Markdown
Member

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 #1661

Plugins 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:

Management Commands:
builder Manage builds
checkpoint Manage checkpoints
config Manage Docker configs
container Manage containers
context Manage contexts
engine Manage the docker engine
image Manage images
myplugin (ACME) Pluginize, by ACME

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@ijc@ulyssessouza WDYT? Thought I'd open this to discuss the best approach (slightly leaning towards the .IsTopLevelCommand boolean in the metadata)

@thaJeztah

thaJeztah commented Feb 21, 2019

Copy link
Copy Markdown
MemberAuthor

here's a docker-compose "plugin" to test the new behavior;

#!/usr/bin/env bashdocker_cli_plugin_metadata() {
if [ -z"$DOCKER_COMPOSE_VERSION" ];thenexport DOCKER_COMPOSE_VERSION="$(docker-compose --version | cut -d "" -f 3 | cut -d "," -f 1)"filocal 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-io

codecov-io commented Feb 21, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1684 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@ 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

@ijc

ijc commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

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 cli-plugins/plugin/ to automatically populate the array (and docs of course)

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

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

@thaJeztah
thaJeztahforce-pushed the plugin_subcommand_detection branch from 2e2a293 to d22a4b2CompareFebruary 22, 2019 13:54
@thaJeztahthaJeztah changed the title WIP detect plugin management commandsShow plugins as Management commandsFeb 22, 2019
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

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 (DOCKER_HIDE_LEGACY_COMMANDS=1 will hide them), or at least from the very beginning of Docker.

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

whoops, didn't look at tests yet; will have to update one

@ijc

ijc commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

Maybe we can get #1675 in first and rebase this onto it?

@thaJeztah
thaJeztahforce-pushed the plugin_subcommand_detection branch from d22a4b2 to 5270632CompareFebruary 22, 2019 14:35
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Yes, I want to have that one in first; was waiting for @silvin-lubecki to approve it

@thaJeztah
thaJeztahforce-pushed the plugin_subcommand_detection branch from 5270632 to 99b69c2CompareFebruary 22, 2019 14:53
@ijcijc mentioned this pull request Feb 25, 2019
11 tasks
@thaJeztah
thaJeztahforce-pushed the plugin_subcommand_detection branch 2 times, most recently from 335ae1d to 2c7b742CompareFebruary 25, 2019 11:17
Comment threadcli/cobra.go

{{- range operationSubCommands . }}
{{rpad (decoratedName .) (add .NamePadding 1)}}{{.Short}}{{ if isPlugin .}} {{vendorAndVersion .}}{{ end}}
{{rpad .Name .NamePadding }} {{.Short}}

Copy link
Copy Markdown
MemberAuthor

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

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@ijc@silvin-lubecki rebased; and updated the template for operationSubCommands (see comment above) PTAL

ijc
ijc approved these changes Feb 25, 2019
Comment threade2e/cli-plugins/help_test.go
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>
@thaJeztah
thaJeztahforce-pushed the plugin_subcommand_detection branch from 2c7b742 to f8c5f5dCompareFebruary 25, 2019 23:28
ijc
ijc approved these changes Feb 26, 2019

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@silvin-lubecki
silvin-lubecki merged commit 3174ca0 into docker:masterFeb 26, 2019
@GordonTheTurtleGordonTheTurtle added this to the 19.03.0 milestone Feb 26, 2019
@thaJeztah
thaJeztah deleted the plugin_subcommand_detection branch February 26, 2019 13:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah@codecov-io@ijc@silvin-lubecki@GordonTheTurtle