Followup to #1564 (cli plugins) #1661
Labels
Comments
This was referenced Feb 18, 2019
|
noticed another thing; open a separate issue for that #1682 |
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 7, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 7, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 8, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 8, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 8, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 11, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 11, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 11, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 11, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 12, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 12, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 12, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 12, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 13, 2019
The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <ijc@docker.com>
ijc
pushed a commit
to ijc/docker-cli
that referenced
this issue
Mar 13, 2019
Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <ijc@docker.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
From #1564 (comment):
docker someplugin --version. The--version/-vflag is handled by the plugin root-command, and therefore shows thedockerversion information (Docker version 19.03.0-dev, build c8f1f1ee). We should fix this to be handled by theplugin.--version/-vpart of the pluging "API", so that a plugin can advertise its version (both when calling the plugin directly (docker-myplugin --version), and when calling through thedockercli (docker myplugin --version). Perhaps we should add a standard function and/or define a standard format (docker-myplugin version X, commit deadbeef (ACME, Inc.))~/.docker/pluginsor~/.docker/cli-plugins→ See issue Should cli plugins install to ~/.docker/plugins #1677 for discussion.docker --help: make plugin-command stand out from built-in commands, or make them appear as regular commands with a non-obtrusive indicator (*) → See PR Reformat plugin's vendor position and add version on --help #1675.docker info→ See PR Reformat the output of CLI plugins indocker system info#1689.displayutils.Ellipsis()instead of a custom implementation. Obsoleted by PR Reformat plugin's vendor position and add version on --help #1675.config.Path()to disallow directory traversalOthers:
docker help→ See PR Show plugins as Management commands #1684.config.jsonfor their use, see Configuration in design → See PR Add a field to the config file for plugin use. #1652.dial-stdioto connect to engine. From CLI Plugins Design #1534 (comment) → See PR cli-plugins: use system dial-stdio to contact the engine. #1654 .The text was updated successfully, but these errors were encountered: