feat: wire ctx into plugin hooks #5039
Conversation
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5039 +/- ##
==========================================
- Coverage 61.08% 61.07% -0.02%
==========================================
Files 295 295
Lines 20660 20664 +4
==========================================
Hits 12621 12621
- Misses 7142 7146 +4
Partials 897 897 |
| commandName := strings.TrimPrefix(subCommand.CommandPath(), rootCmd.Name()+" ") | ||
| flags := getCommandFlags(subCommand) | ||
|
|
||
| runHooks(dockerCli, rootCmd, subCommand, commandName, flags, cmdErrorMessage) | ||
| runHooks(ctx, dockerCli, rootCmd, subCommand, commandName, flags, cmdErrorMessage) |
There was a problem hiding this comment.
nothing to change now, but these signatures are getting a bit rough 😅
There was a problem hiding this comment.
yeah I agree, but I didn't want to refactor it in this PR 😓
|
Looks great! (and reminds me I'd really like for us to add some e2e tests for hooks when we get a chance 🥲) |
oh yeah, saw this while adding these. I can do that in another PR once I have some time. I have a plan to utilize a pattern I used inside DD which re-uses the go test library to simulate external executables. |
|
We already have a bunch of "test" plugins in this repo that get compiled for end to end tests, so give those a look too! I don't know how the simulating external I guess what I mean is for e2e tests I'd prefer actual plugin binaries, but also we could/should look into other approaches for unit/integration tests. |
| func RunCLICommandHooks(ctx context.Context, dockerCli command.Cli, rootCmd, subCommand *cobra.Command, cmdErrorMessage string) { | ||
| commandName := strings.TrimPrefix(subCommand.CommandPath(), rootCmd.Name()+" ") | ||
| flags := getCommandFlags(subCommand) |
There was a problem hiding this comment.
@Benehiko @laurazard Sorry, late to the party, and not 100% sure what's best (things get somewhat confusing 😅)
Which context should be used in these cases? I see we already have the rootCmd and subCommand passed; both of which carry a context; should that context be the one to use? (rootCmd.Context() or subCommand.Context())
(If that's the one to use, then possibly the signature wouldn't have to be changed 🤔
There was a problem hiding this comment.
that's true, although adding the ctx parameter is way more explicit and the recommended way according to the Go documentation. The context stored inside the rootCommand and subCommand is the same as the one passed down. While I was looking at this I actually wanted to refactor the signature into an interface or using only the docker.Cli interface since passing down rootCmd and subCommand seems superfluous. But this is a topic for another time I think
In the spirit of wiring up
context.Contextfor graceful exit and early termination of tasks, I have done some more wiring up ofctxinside the plugin hooks. This allows early returns for when thectxis cancelled so that hooks aren't printed as well as cancelling the execution of the plugin throughexec.CommandContext.- What I did
Allow early cancellation of plugin hooks through
context.Contextand plugin execution cancellation throughexec.CommandContext.- How I did it
Wired up
ctxinside theRunCLICommandHooksandRunPluginHooks.- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)