Skip to content

Fix: run plugin hooks on command failure, not just success #6794

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.

Sign up for GitHub

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

Merged
thaJeztah merged 2 commits into from
Mar 4, 2026
+348 −22
Merged

Fix: run plugin hooks on command failure, not just success #6794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9bc1899
Fix: run plugin hooks on command failure, not just success
derekmisler Feb 11, 2026
830d05d
error-hooks approach
derekmisler Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions cli-plugins/manager/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ func RunCLICommandHooks(ctx context.Context, dockerCLI config.Provider, rootCmd,

// RunPluginHooks is the entrypoint for the hooks execution flow
// after a plugin command was just executed by the CLI.
func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string) {
func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string, cmdErrorMessage string) {
Copy link
Member

@thaJeztah thaJeztah Feb 11, 2026

Choose a reason for hiding this comment

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

Just so I don't forget; this function is exported (and may be in use elsewhere), so we probably can't change the signature.

Copy link
Contributor Author

@derekmisler derekmisler Feb 11, 2026

Choose a reason for hiding this comment

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

This was the only implementation site I could find: https://github.com/docker/cli/pull/6794/changes#diff-3dcd6325b70a5f5db393a3ecc08060b69007fcef371fb4dabf3164da95646fb6R488

Copy link
Member

@thaJeztah thaJeztah Feb 12, 2026

Choose a reason for hiding this comment

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

Ah, yes, maybe we're fine; at least nothing in a public repository AFAICS; https://grep.app/search?q=.RunPluginHooks

We should probably consider to move some of this to internal/, and make the "public" (cli-plugins/xxxx) wrappers for the internal implementation so that we have a better separation between "these are only to be used by the CLI itself" and "these are "ok(ish)" for external consumers". We've been painted in a corner too many times because things were meant for the CLI itself, but happened to be exported, and now ... got used by other projects.

commandName := strings.Join(args, " ")
flags := getNaiveFlags(args)

runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, "")
runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, cmdErrorMessage)
}

func runHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCommand *cobra.Command, invokedCommand string, flags map[string]string, cmdErrorMessage string) {
Expand All @@ -70,7 +70,7 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
pluginDirs := getPluginDirs(cfg)
nextSteps := make([]string, 0, len(pluginsCfg))
for pluginName, pluginCfg := range pluginsCfg {
match, ok := pluginMatch(pluginCfg, subCmdStr)
match, ok := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage)
if !ok {
continue
}
Expand Down Expand Up @@ -138,13 +138,34 @@ func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) {
// command being executed (such as 'image ls' – the root 'docker' is omitted)
// and, if the configuration includes a hook for the invoked command, returns
// the configured hook string.
func pluginMatch(pluginCfg map[string]string, subCmd string) (string, bool) {
configuredPluginHooks, ok := pluginCfg["hooks"]
if !ok || configuredPluginHooks == "" {
//
// Plugins can declare two types of hooks in their configuration:
// - "hooks": fires on every command invocation (success or failure)
// - "error-hooks": fires only when a command fails (cmdErrorMessage is non-empty)
func pluginMatch(pluginCfg map[string]string, subCmd string, cmdErrorMessage string) (string, bool) {
Copy link
Member

@laurazard laurazard Mar 4, 2026

Choose a reason for hiding this comment

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

Seems like this function doesn't need cmdErrorMessage, just to know whether it was an error or not.

// Check "hooks" first — these always fire regardless of command outcome.
if match, ok := matchHookConfig(pluginCfg["hooks"], subCmd); ok {
return match, true
}

// Check "error-hooks" — these only fire when there was an error.
if cmdErrorMessage != "" {
if match, ok := matchHookConfig(pluginCfg["error-hooks"], subCmd); ok {
return match, true
}
}

return "", false
}

// matchHookConfig checks if a comma-separated hook configuration string
// contains a prefix match for the given subcommand.
func matchHookConfig(configuredHooks string, subCmd string) (string, bool) {
if configuredHooks == "" {
return "", false
}

for hookCmd := range strings.SplitSeq(configuredPluginHooks, ",") {
for hookCmd := range strings.SplitSeq(configuredHooks, ",") {
if hookMatch(hookCmd, subCmd) {
return hookCmd, true
}
Expand Down
246 changes: 239 additions & 7 deletions cli-plugins/manager/hooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
package manager

import (
"context"
"testing"

"github.com/docker/cli/cli/config/configfile"
"github.com/spf13/cobra"
"gotest.tools/v3/assert"
is "gotest.tools/v3/assert/cmp"
)

type fakeConfigProvider struct {
cfg *configfile.ConfigFile
}

func (f *fakeConfigProvider) ConfigFile() *configfile.ConfigFile {
return f.cfg
}

func TestGetNaiveFlags(t *testing.T) {
testCases := []struct {
args []string
Expand Down Expand Up @@ -40,12 +51,15 @@ func TestGetNaiveFlags(t *testing.T) {

func TestPluginMatch(t *testing.T) {
testCases := []struct {
commandString string
pluginConfig map[string]string
expectedMatch string
expectedOk bool
doc string
commandString string
pluginConfig map[string]string
cmdErrorMessage string
expectedMatch string
expectedOk bool
}{
{
doc: "hooks prefix match",
commandString: "image ls",
pluginConfig: map[string]string{
"hooks": "image",
Expand All @@ -54,6 +68,7 @@ func TestPluginMatch(t *testing.T) {
expectedOk: true,
},
{
doc: "hooks no match",
commandString: "context ls",
pluginConfig: map[string]string{
"hooks": "build",
Expand All @@ -62,6 +77,7 @@ func TestPluginMatch(t *testing.T) {
expectedOk: false,
},
{
doc: "hooks exact match",
commandString: "context ls",
pluginConfig: map[string]string{
"hooks": "context ls",
Expand All @@ -70,6 +86,7 @@ func TestPluginMatch(t *testing.T) {
expectedOk: true,
},
{
doc: "hooks first match wins",
commandString: "image ls",
pluginConfig: map[string]string{
"hooks": "image ls,image",
Expand All @@ -78,6 +95,7 @@ func TestPluginMatch(t *testing.T) {
expectedOk: true,
},
{
doc: "hooks empty string",
commandString: "image ls",
pluginConfig: map[string]string{
"hooks": "",
Expand All @@ -86,6 +104,7 @@ func TestPluginMatch(t *testing.T) {
expectedOk: false,
},
{
doc: "hooks partial token no match",
commandString: "image inspect",
pluginConfig: map[string]string{
"hooks": "image i",
Expand All @@ -94,19 +113,148 @@ func TestPluginMatch(t *testing.T) {
expectedOk: false,
},
{
doc: "hooks prefix token match",
commandString: "image inspect",
pluginConfig: map[string]string{
"hooks": "image",
},
expectedMatch: "image",
expectedOk: true,
},
{
doc: "error-hooks match on error",
commandString: "build",
pluginConfig: map[string]string{
"error-hooks": "build",
},
cmdErrorMessage: "exit status 1",
expectedMatch: "build",
expectedOk: true,
},
{
doc: "error-hooks no match on success",
commandString: "build",
pluginConfig: map[string]string{
"error-hooks": "build",
},
cmdErrorMessage: "",
expectedMatch: "",
expectedOk: false,
},
{
doc: "error-hooks prefix match on error",
commandString: "compose up",
pluginConfig: map[string]string{
"error-hooks": "compose",
},
cmdErrorMessage: "exit status 1",
expectedMatch: "compose",
expectedOk: true,
},
{
doc: "error-hooks no match for wrong command",
commandString: "pull",
pluginConfig: map[string]string{
"error-hooks": "build",
},
cmdErrorMessage: "exit status 1",
expectedMatch: "",
expectedOk: false,
},
{
doc: "hooks takes precedence over error-hooks",
commandString: "build",
pluginConfig: map[string]string{
"hooks": "build",
"error-hooks": "build",
},
cmdErrorMessage: "exit status 1",
expectedMatch: "build",
expectedOk: true,
},
{
doc: "hooks fires on success even with error-hooks configured",
commandString: "build",
pluginConfig: map[string]string{
"hooks": "build",
"error-hooks": "build",
},
cmdErrorMessage: "",
expectedMatch: "build",
expectedOk: true,
},
{
doc: "error-hooks with multiple commands",
commandString: "compose up",
pluginConfig: map[string]string{
"error-hooks": "build,compose up,pull",
},
cmdErrorMessage: "exit status 1",
expectedMatch: "compose up",
expectedOk: true,
},
}

for _, tc := range testCases {
t.Run(tc.doc, func(t *testing.T) {
match, ok := pluginMatch(tc.pluginConfig, tc.commandString, tc.cmdErrorMessage)
assert.Equal(t, ok, tc.expectedOk)
assert.Equal(t, match, tc.expectedMatch)
})
}
}

func TestMatchHookConfig(t *testing.T) {
testCases := []struct {
doc string
configuredHooks string
subCmd string
expectedMatch string
expectedOk bool
}{
{
doc: "empty config",
configuredHooks: "",
subCmd: "build",
expectedMatch: "",
expectedOk: false,
},
{
doc: "exact match",
configuredHooks: "build",
subCmd: "build",
expectedMatch: "build",
expectedOk: true,
},
{
doc: "prefix match",
configuredHooks: "image",
subCmd: "image ls",
expectedMatch: "image",
expectedOk: true,
},
{
doc: "comma-separated match",
configuredHooks: "pull,build,push",
subCmd: "build",
expectedMatch: "build",
expectedOk: true,
},
{
doc: "no match",
configuredHooks: "pull,push",
subCmd: "build",
expectedMatch: "",
expectedOk: false,
},
}

for _, tc := range testCases {
match, ok := pluginMatch(tc.pluginConfig, tc.commandString)
assert.Equal(t, ok, tc.expectedOk)
assert.Equal(t, match, tc.expectedMatch)
t.Run(tc.doc, func(t *testing.T) {
match, ok := matchHookConfig(tc.configuredHooks, tc.subCmd)
assert.Equal(t, ok, tc.expectedOk)
assert.Equal(t, match, tc.expectedMatch)
})
Comment on lines +207 to +257
Copy link
Member

@laurazard laurazard Mar 4, 2026

Choose a reason for hiding this comment

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

I understand that this change includes a refactor of hooks.go to split pluginMatch into pluginMatch and matchHookConfig, but these tests are a bit confusing – doesn't this test cover a subset of what is now covered in TestPluginMatch?

}
}

Expand Down Expand Up @@ -141,3 +289,87 @@ func TestAppendNextSteps(t *testing.T) {
})
}
}

func TestRunPluginHooksPassesErrorMessage(t *testing.T) {
cfg := configfile.New("")
cfg.Plugins = map[string]map[string]string{
"test-plugin": {"hooks": "build"},
}
provider := &fakeConfigProvider{cfg: cfg}
root := &cobra.Command{Use: "docker"}
sub := &cobra.Command{Use: "build"}
root.AddCommand(sub)

// Should not panic with empty error message (success case)
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "")

// Should not panic with non-empty error message (failure case)
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1")
}

func TestRunPluginHooksErrorHooks(t *testing.T) {
cfg := configfile.New("")
cfg.Plugins = map[string]map[string]string{
"test-plugin": {"error-hooks": "build"},
}
provider := &fakeConfigProvider{cfg: cfg}
root := &cobra.Command{Use: "docker"}
sub := &cobra.Command{Use: "build"}
root.AddCommand(sub)

// Should not panic — error-hooks with error message
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1")

// Should not panic — error-hooks with no error (should be skipped)
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "")
}

func TestInvokeAndCollectHooksErrorHooksSkippedOnSuccess(t *testing.T) {
cfg := configfile.New("")
cfg.Plugins = map[string]map[string]string{
"nonexistent": {"error-hooks": "build"},
}
root := &cobra.Command{Use: "docker"}
sub := &cobra.Command{Use: "build"}
root.AddCommand(sub)

// On success, error-hooks should not match, so the plugin
// binary is never looked up and no results are returned.
result := invokeAndCollectHooks(
context.Background(), cfg, root, sub,
"build", map[string]string{}, "",
)
assert.Check(t, is.Len(result, 0))
}

func TestInvokeAndCollectHooksNoPlugins(t *testing.T) {
cfg := configfile.New("")
root := &cobra.Command{Use: "docker"}
sub := &cobra.Command{Use: "build"}
root.AddCommand(sub)

result := invokeAndCollectHooks(
context.Background(), cfg, root, sub,
"build", map[string]string{}, "some error",
)
assert.Check(t, is.Len(result, 0))
}

func TestInvokeAndCollectHooksCancelledContext(t *testing.T) {
cfg := configfile.New("")
cfg.Plugins = map[string]map[string]string{
"test-plugin": {"hooks": "build"},
}
root := &cobra.Command{Use: "docker"}
sub := &cobra.Command{Use: "build"}
root.AddCommand(sub)

ctx, cancel := context.WithCancel(context.Background())
cancel() // cancel immediately

result := invokeAndCollectHooks(
ctx, cfg, root, sub,
"build", map[string]string{}, "exit status 1",
)
assert.Check(t, is.Nil(result))
}
Loading
Loading