Skip to content
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

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 21, 2019

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
Member Author

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

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 21, 2019

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

#!/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-io
Copy link

codecov-io commented Feb 21, 2019

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
Copy link
Contributor

ijc commented Feb 22, 2019

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
Member Author

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 thaJeztah force-pushed the plugin_subcommand_detection branch from 2e2a293 to d22a4b2 Compare February 22, 2019 13:54
@thaJeztah thaJeztah changed the title WIP detect plugin management commands Show plugins as Management commands Feb 22, 2019
@thaJeztah
Copy link
Member Author

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
Member Author

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

@ijc
Copy link
Contributor

ijc commented Feb 22, 2019

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

@thaJeztah thaJeztah force-pushed the plugin_subcommand_detection branch from d22a4b2 to 5270632 Compare February 22, 2019 14:35
@thaJeztah
Copy link
Member Author

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

@thaJeztah thaJeztah force-pushed the plugin_subcommand_detection branch from 5270632 to 99b69c2 Compare February 22, 2019 14:53
@ijc ijc mentioned this pull request Feb 25, 2019
11 tasks
@thaJeztah thaJeztah force-pushed the plugin_subcommand_detection branch 2 times, most recently from 335ae1d to 2c7b742 Compare February 25, 2019 11:17
@@ -245,7 +248,7 @@ Management Commands:
Commands:

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

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
Member Author

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

e2e/cli-plugins/help_test.go Show resolved Hide resolved
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 thaJeztah force-pushed the plugin_subcommand_detection branch from 2c7b742 to f8c5f5d Compare February 25, 2019 23:28
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

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:master Feb 26, 2019
@GordonTheTurtle GordonTheTurtle 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 free to 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