Skip to content

Commit 01dbbf7

Browse files
derekmislerderekmisler
committed
error-hooks approach
Signed-off-by: Derek Misler <derek.misler@docker.com>
1 parent 19b66c8 commit 01dbbf7

File tree

2 files changed

+192
-18
lines changed

2 files changed

+192
-18
lines changed

‎cli-plugins/manager/hooks.go‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
7070
pluginDirs := getPluginDirs(cfg)
7171
nextSteps := make([]string, 0, len(pluginsCfg))
7272
for pluginName, pluginCfg := range pluginsCfg {
73-
match, ok := pluginMatch(pluginCfg, subCmdStr)
73+
match, ok := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage)
7474
if !ok {
7575
continue
7676
}
@@ -138,13 +138,34 @@ func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) {
138138
// command being executed (such as 'image ls' – the root 'docker' is omitted)
139139
// and, if the configuration includes a hook for the invoked command, returns
140140
// the configured hook string.
141-
func pluginMatch(pluginCfg map[string]string, subCmd string) (string, bool) {
142-
configuredPluginHooks, ok := pluginCfg["hooks"]
143-
if !ok || configuredPluginHooks == "" {
141+
//
142+
// Plugins can declare two types of hooks in their configuration:
143+
// - "hooks": fires on every command invocation (success or failure)
144+
// - "error-hooks": fires only when a command fails (cmdErrorMessage is non-empty)
145+
func pluginMatch(pluginCfg map[string]string, subCmd string, cmdErrorMessage string) (string, bool) {
146+
// Check "hooks" first — these always fire regardless of command outcome.
147+
if match, ok := matchHookConfig(pluginCfg["hooks"], subCmd); ok {
148+
return match, true
149+
}
150+
151+
// Check "error-hooks" — these only fire when there was an error.
152+
if cmdErrorMessage != "" {
153+
if match, ok := matchHookConfig(pluginCfg["error-hooks"], subCmd); ok {
154+
return match, true
155+
}
156+
}
157+
158+
return "", false
159+
}
160+
161+
// matchHookConfig checks if a comma-separated hook configuration string
162+
// contains a prefix match for the given subcommand.
163+
func matchHookConfig(configuredHooks string, subCmd string) (string, bool) {
164+
if configuredHooks == "" {
144165
return "", false
145166
}
146167

147-
for hookCmd := range strings.SplitSeq(configuredPluginHooks, ",") {
168+
for hookCmd := range strings.SplitSeq(configuredHooks, ",") {
148169
if hookMatch(hookCmd, subCmd) {
149170
return hookCmd, true
150171
}

‎cli-plugins/manager/hooks_test.go‎

Lines changed: 166 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,15 @@ func TestGetNaiveFlags(t *testing.T) {
5151

5252
func TestPluginMatch(t *testing.T) {
5353
testCases := []struct {
54-
commandString string
55-
pluginConfig map[string]string
56-
expectedMatch string
57-
expectedOk bool
54+
doc string
55+
commandString string
56+
pluginConfig map[string]string
57+
cmdErrorMessage string
58+
expectedMatch string
59+
expectedOk bool
5860
}{
5961
{
62+
doc: "hooks prefix match",
6063
commandString: "image ls",
6164
pluginConfig: map[string]string{
6265
"hooks": "image",
@@ -65,6 +68,7 @@ func TestPluginMatch(t *testing.T) {
6568
expectedOk: true,
6669
},
6770
{
71+
doc: "hooks no match",
6872
commandString: "context ls",
6973
pluginConfig: map[string]string{
7074
"hooks": "build",
@@ -73,6 +77,7 @@ func TestPluginMatch(t *testing.T) {
7377
expectedOk: false,
7478
},
7579
{
80+
doc: "hooks exact match",
7681
commandString: "context ls",
7782
pluginConfig: map[string]string{
7883
"hooks": "context ls",
@@ -81,6 +86,7 @@ func TestPluginMatch(t *testing.T) {
8186
expectedOk: true,
8287
},
8388
{
89+
doc: "hooks first match wins",
8490
commandString: "image ls",
8591
pluginConfig: map[string]string{
8692
"hooks": "image ls,image",
@@ -89,6 +95,7 @@ func TestPluginMatch(t *testing.T) {
8995
expectedOk: true,
9096
},
9197
{
98+
doc: "hooks empty string",
9299
commandString: "image ls",
93100
pluginConfig: map[string]string{
94101
"hooks": "",
@@ -97,6 +104,7 @@ func TestPluginMatch(t *testing.T) {
97104
expectedOk: false,
98105
},
99106
{
107+
doc: "hooks partial token no match",
100108
commandString: "image inspect",
101109
pluginConfig: map[string]string{
102110
"hooks": "image i",
@@ -105,19 +113,148 @@ func TestPluginMatch(t *testing.T) {
105113
expectedOk: false,
106114
},
107115
{
116+
doc: "hooks prefix token match",
108117
commandString: "image inspect",
109118
pluginConfig: map[string]string{
110119
"hooks": "image",
111120
},
112121
expectedMatch: "image",
113122
expectedOk: true,
114123
},
124+
{
125+
doc: "error-hooks match on error",
126+
commandString: "build",
127+
pluginConfig: map[string]string{
128+
"error-hooks": "build",
129+
},
130+
cmdErrorMessage: "exit status 1",
131+
expectedMatch: "build",
132+
expectedOk: true,
133+
},
134+
{
135+
doc: "error-hooks no match on success",
136+
commandString: "build",
137+
pluginConfig: map[string]string{
138+
"error-hooks": "build",
139+
},
140+
cmdErrorMessage: "",
141+
expectedMatch: "",
142+
expectedOk: false,
143+
},
144+
{
145+
doc: "error-hooks prefix match on error",
146+
commandString: "compose up",
147+
pluginConfig: map[string]string{
148+
"error-hooks": "compose",
149+
},
150+
cmdErrorMessage: "exit status 1",
151+
expectedMatch: "compose",
152+
expectedOk: true,
153+
},
154+
{
155+
doc: "error-hooks no match for wrong command",
156+
commandString: "pull",
157+
pluginConfig: map[string]string{
158+
"error-hooks": "build",
159+
},
160+
cmdErrorMessage: "exit status 1",
161+
expectedMatch: "",
162+
expectedOk: false,
163+
},
164+
{
165+
doc: "hooks takes precedence over error-hooks",
166+
commandString: "build",
167+
pluginConfig: map[string]string{
168+
"hooks": "build",
169+
"error-hooks": "build",
170+
},
171+
cmdErrorMessage: "exit status 1",
172+
expectedMatch: "build",
173+
expectedOk: true,
174+
},
175+
{
176+
doc: "hooks fires on success even with error-hooks configured",
177+
commandString: "build",
178+
pluginConfig: map[string]string{
179+
"hooks": "build",
180+
"error-hooks": "build",
181+
},
182+
cmdErrorMessage: "",
183+
expectedMatch: "build",
184+
expectedOk: true,
185+
},
186+
{
187+
doc: "error-hooks with multiple commands",
188+
commandString: "compose up",
189+
pluginConfig: map[string]string{
190+
"error-hooks": "build,compose up,pull",
191+
},
192+
cmdErrorMessage: "exit status 1",
193+
expectedMatch: "compose up",
194+
expectedOk: true,
195+
},
196+
}
197+
198+
for _, tc := range testCases {
199+
t.Run(tc.doc, func(t *testing.T) {
200+
match, ok := pluginMatch(tc.pluginConfig, tc.commandString, tc.cmdErrorMessage)
201+
assert.Equal(t, ok, tc.expectedOk)
202+
assert.Equal(t, match, tc.expectedMatch)
203+
})
204+
}
205+
}
206+
207+
func TestMatchHookConfig(t *testing.T) {
208+
testCases := []struct {
209+
doc string
210+
configuredHooks string
211+
subCmd string
212+
expectedMatch string
213+
expectedOk bool
214+
}{
215+
{
216+
doc: "empty config",
217+
configuredHooks: "",
218+
subCmd: "build",
219+
expectedMatch: "",
220+
expectedOk: false,
221+
},
222+
{
223+
doc: "exact match",
224+
configuredHooks: "build",
225+
subCmd: "build",
226+
expectedMatch: "build",
227+
expectedOk: true,
228+
},
229+
{
230+
doc: "prefix match",
231+
configuredHooks: "image",
232+
subCmd: "image ls",
233+
expectedMatch: "image",
234+
expectedOk: true,
235+
},
236+
{
237+
doc: "comma-separated match",
238+
configuredHooks: "pull,build,push",
239+
subCmd: "build",
240+
expectedMatch: "build",
241+
expectedOk: true,
242+
},
243+
{
244+
doc: "no match",
245+
configuredHooks: "pull,push",
246+
subCmd: "build",
247+
expectedMatch: "",
248+
expectedOk: false,
249+
},
115250
}
116251

117252
for _, tc := range testCases {
118-
match, ok := pluginMatch(tc.pluginConfig, tc.commandString)
119-
assert.Equal(t, ok, tc.expectedOk)
120-
assert.Equal(t, match, tc.expectedMatch)
253+
t.Run(tc.doc, func(t *testing.T) {
254+
match, ok := matchHookConfig(tc.configuredHooks, tc.subCmd)
255+
assert.Equal(t, ok, tc.expectedOk)
256+
assert.Equal(t, match, tc.expectedMatch)
257+
})
121258
}
122259
}
123260

@@ -170,21 +307,37 @@ func TestRunPluginHooksPassesErrorMessage(t *testing.T) {
170307
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1")
171308
}
172309

173-
func TestInvokeAndCollectHooksForwardsErrorMessage(t *testing.T) {
310+
func TestRunPluginHooksErrorHooks(t *testing.T) {
174311
cfg := configfile.New("")
175312
cfg.Plugins = map[string]map[string]string{
176-
"nonexistent": {"hooks": "build"},
313+
"test-plugin": {"error-hooks": "build"},
177314
}
315+
provider := &fakeConfigProvider{cfg: cfg}
178316
root := &cobra.Command{Use: "docker"}
179317
sub := &cobra.Command{Use: "build"}
180318
root.AddCommand(sub)
181319

182-
// Plugin binary doesn't exist — invokeAndCollectHooks skips it
183-
// gracefully and returns empty. Verifies the error message path
184-
// doesn't cause issues when forwarded through the call chain.
320+
// Should not panic — error-hooks with error message
321+
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1")
322+
323+
// Should not panic — error-hooks with no error (should be skipped)
324+
RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "")
325+
}
326+
327+
func TestInvokeAndCollectHooksErrorHooksSkippedOnSuccess(t *testing.T) {
328+
cfg := configfile.New("")
329+
cfg.Plugins = map[string]map[string]string{
330+
"nonexistent": {"error-hooks": "build"},
331+
}
332+
root := &cobra.Command{Use: "docker"}
333+
sub := &cobra.Command{Use: "build"}
334+
root.AddCommand(sub)
335+
336+
// On success, error-hooks should not match, so the plugin
337+
// binary is never looked up and no results are returned.
185338
result := invokeAndCollectHooks(
186339
context.Background(), cfg, root, sub,
187-
"build", map[string]string{}, "exit status 1",
340+
"build", map[string]string{}, "",
188341
)
189342
assert.Check(t, is.Len(result, 0))
190343
}

0 commit comments

Comments
 (0)