Fix regression for CLI plugins using PersistentPreRunE #1737
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 14, 2019 14:20
I regressed this in d4ced2e ("allow plugins to have argument which match a top-level flag.") by unconditionally overwriting any `PersistentRunE` that the user may have supplied. We need to ensure two things: 1. That the user can use `PersistentRunE` (or `PersistentRun`) for their own purposes. 2. That our initialisation always runs, even if the user has used `PersistentRun*`, since that will shadow the root. To do this add a `PersistentRunE` to the helloworld plugin which logs (covers 1 above) and then use it when calling the `apiversion` subcommand (which covers 2 since that uses the client) Signed-off-by: Ian Campbell <ijc@docker.com>
I got a bit carried away in d4ced2e ("allow plugins to have argument which match a top-level flag.") and broke the ability of a plugin to use the `PersistentPreRun(E)` hook on its top-level command (by unconditionally overwriting it) and also broke the plugin framework if a plugin's subcommand used those hooks (because they would shadow the root one). This could result in either `dockerCli.Client()` returning `nil` or whatever initialisation the plugin hoped to do not occuring. This change revert the relevant bits and reinstates the requirement that a plugin calls `plugin.PersistentPreRunE` if it uses that hook itself. It is at least a bit nicer now since we avoid the need for the global struct since the interesting state is now encapsulated in `tcmd` (and the closure). In principal this could be done even more simply (by calling `tcmd.Initialize` statically between `tcmd.HandleGlobalFlags` and `cmd.Execute`) however this has the downside of _always_ initialising the cli (and therefore dialing the daemon) even for the `docker-cli-plugin-metadata` command but also for the `help foo` and `foo --help` commands (Cobra short-circuits the hooks in this case). Signed-off-by: Ian Campbell <ijc@docker.com>
Codecov Report
@@ Coverage Diff @@
## master #1737 +/- ##
=======================================
Coverage 56.04% 56.04%
=======================================
Files 306 306
Lines 21050 21050
=======================================
Hits 11798 11798
Misses 8399 8399
Partials 853 853 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I got a bit carried away in d4ced2e ("allow plugins to have argument which match a top-level flag.") and broke the ability of a plugin to use the
PersistentPreRun(E)hook on its top-level command (by unconditionally overwriting it) and also broke the plugin framework if a plugin's subcommand used those hooks (because they would shadow the root one). This could result in eitherdockerCli.Client()returningnilor whatever initialisation the plugin hoped to do not occuring.Unfortunately my carriedawayness extended to thinking the e2e test was no longer necessary (although I'm not completely sure it would have caught this case in fact). So first introduce a new (and better) e2e test case for this, then fix the issue which ends up being easier looking cleaner than the original code (so this isn't a simple partial revert).