From de8f4032adab4a18658a6309a1e676b5f82afec3 Mon Sep 17 00:00:00 2001 From: Nicolas Beck Date: Fri, 9 Oct 2026 15:50:53 +0000 Subject: [PATCH] cli-plugins: Add generic hook messages Allow plugins to return informational messages without the next-steps heading or indentation. Invoke each hook once and render generic messages before the shared next-steps block. Centralize grouping and formatting in PrintMessages, with a callback for template evaluation and error handling. This avoids the need for an intermediate EvaluatedMessage type and simplifies hook collection to return the original responses. Skip messages containing only whitespace while preserving empty lines within non-empty messages. Keep PrintNextSteps as a deprecated wrapper to preserve existing callers and output. Update the printer tests and remove the obsolete appendMessages helper and its tests. Co-authored-by: Nicolas Beck Signed-off-by: Nicolas Beck Signed-off-by: Sebastiaan van Stijn --- cli-plugins/hooks/hook_types.go | 4 + cli-plugins/hooks/printer.go | 45 +++++++-- cli-plugins/hooks/printer_test.go | 85 ++++++++++++++++- cli-plugins/hooks/template.go | 12 ++- cli-plugins/hooks/template_test.go | 12 ++- cli-plugins/manager/hooks.go | 70 +++++--------- cli-plugins/manager/hooks_test.go | 143 ++++++++++++++++++++++------- 7 files changed, 276 insertions(+), 95 deletions(-) diff --git a/cli-plugins/hooks/hook_types.go b/cli-plugins/hooks/hook_types.go index 03ee7294935a..9cda993e186f 100644 --- a/cli-plugins/hooks/hook_types.go +++ b/cli-plugins/hooks/hook_types.go @@ -30,7 +30,11 @@ package hooks type ResponseType int const ( + // NextSteps renders suggestions under a shared "What's next:" header. NextSteps ResponseType = 0 + // GenericMessage renders before NextSteps, without a header or indentation. + // CLI versions that do not recognize this type ignore the response. + GenericMessage ResponseType = 1 ) // Request is the type representing the information diff --git a/cli-plugins/hooks/printer.go b/cli-plugins/hooks/printer.go index c9b0bd9bcd9f..ff19762690d7 100644 --- a/cli-plugins/hooks/printer.go +++ b/cli-plugins/hooks/printer.go @@ -7,17 +7,42 @@ const ( indent = " " ) -// PrintNextSteps renders list of [NextSteps] messages and writes them -// to out. It is a no-op if messages is empty. -func PrintNextSteps(out io.Writer, messages []string) { - if len(messages) == 0 { - return +// PrintMessages groups messages by type and writes them to out. Generic messages +// appear first, without a header or indentation, followed by next steps under a +// shared "What's next:" header. Order within each type is preserved. +// Messages with no lines or an unknown type produce no output. +func PrintMessages(out io.Writer, messages []Response, evaluate func(Response) []string) { + grouped := make(map[ResponseType][]string) + for _, message := range messages { + lines := evaluate(message) + grouped[message.Type] = append(grouped[message.Type], lines...) } + for _, responseType := range []ResponseType{GenericMessage, NextSteps} { + lines := grouped[responseType] + if len(lines) == 0 { + continue + } - _, _ = io.WriteString(out, whatsNext) - for _, msg := range messages { - _, _ = io.WriteString(out, indent) - _, _ = io.WriteString(out, msg) - _, _ = io.WriteString(out, "\n") + header, prefix := "\n", "" + if responseType == NextSteps { + header, prefix = whatsNext, indent + } + + _, _ = io.WriteString(out, header) + for _, line := range lines { + _, _ = io.WriteString(out, prefix) + _, _ = io.WriteString(out, line) + _, _ = io.WriteString(out, "\n") + } } } + +// PrintNextSteps renders list of [NextSteps] messages and writes them +// to out. It is a no-op if messages is empty. +// +// Deprecated: use [PrintMessages] instead. +func PrintNextSteps(out io.Writer, messages []string) { + PrintMessages(out, []Response{{Type: NextSteps}}, func(Response) []string { + return messages + }) +} diff --git a/cli-plugins/hooks/printer_test.go b/cli-plugins/hooks/printer_test.go index 8b1ca9234ed8..5a1d33c3312b 100644 --- a/cli-plugins/hooks/printer_test.go +++ b/cli-plugins/hooks/printer_test.go @@ -8,7 +8,85 @@ import ( "gotest.tools/v3/assert" ) -func TestPrintHookMessages(t *testing.T) { +func TestPrintMessages(t *testing.T) { + const header = "\n\x1b[1mWhat's next:\x1b[0m\n" + + for _, tc := range []struct { + doc string + messages []hooks.Response + expected string + }{ + {doc: "no messages"}, + { + doc: "no lines", + messages: []hooks.Response{ + {Type: hooks.GenericMessage}, + {Type: hooks.NextSteps}, + }, + }, + { + doc: "generic message", + messages: []hooks.Response{ + {Type: hooks.GenericMessage, Template: "Session active"}, + }, + expected: "\nSession active\n", + }, + { + doc: "preserve message formatting", + messages: []hooks.Response{ + {Type: hooks.GenericMessage, Template: "Session active\n\n Details"}, + }, + expected: "\nSession active\n\n Details\n", + }, + { + doc: "next steps", + messages: []hooks.Response{ + {Type: hooks.NextSteps, Template: "Try another command"}, + }, + expected: header + " Try another command\n", + }, + { + doc: "group interleaved types and preserve order within each type", + messages: []hooks.Response{ + {Type: hooks.NextSteps, Template: "First suggestion"}, + {Type: hooks.GenericMessage, Template: "Session active"}, + {Type: hooks.NextSteps, Template: "Second suggestion\nThird suggestion"}, + {Type: hooks.GenericMessage, Template: "Another status"}, + }, + expected: "\nSession active\nAnother status\n" + header + " First suggestion\n Second suggestion\n Third suggestion\n", + }, + { + doc: "unknown type", + messages: []hooks.Response{ + {Type: hooks.ResponseType(99), Template: "Ignored"}, + }, + }, + { + doc: "unknown type does not affect known messages", + messages: []hooks.Response{ + {Type: hooks.ResponseType(99), Template: "Ignored"}, + {Type: hooks.GenericMessage, Template: "Session active"}, + {Type: hooks.NextSteps, Template: "Try another command"}, + }, + expected: "\nSession active\n" + header + " Try another command\n", + }, + } { + t.Run(tc.doc, func(t *testing.T) { + var w strings.Builder + hooks.PrintMessages(&w, tc.messages, func(message hooks.Response) []string { + if message.Template == "" { + return nil + } + lines, err := hooks.ParseTemplate(message.Template, nil) + assert.NilError(t, err) + return lines + }) + assert.Equal(t, w.String(), tc.expected) + }) + } +} + +func TestPrintNextSteps(t *testing.T) { const header = "\n\x1b[1mWhat's next:\x1b[0m\n" tests := []struct { @@ -34,6 +112,11 @@ func TestPrintHookMessages(t *testing.T) { " Foo\n" + " bar\n", }, + { + doc: "preserve empty and indented lines", + messages: []string{"", " Details", ""}, + expectedOutput: header + " \n Details\n \n", + }, } for _, tc := range tests { t.Run(tc.doc, func(t *testing.T) { diff --git a/cli-plugins/hooks/template.go b/cli-plugins/hooks/template.go index 99abb9acf828..d0057da56b85 100644 --- a/cli-plugins/hooks/template.go +++ b/cli-plugins/hooks/template.go @@ -7,6 +7,7 @@ import ( "bytes" "errors" "fmt" + "slices" "strings" "text/template" @@ -43,7 +44,16 @@ func ParseTemplate(hookTemplate string, cmd *cobra.Command) ([]string, error) { if n := strings.Count(out, "\n"); n > maxMessages { return nil, fmt.Errorf("hook template contains too many messages (%d): maximum is %d", n, maxMessages) } - return strings.SplitN(out, "\n", maxMessages), nil + lines := strings.SplitN(out, "\n", maxMessages) + + // Skip messages containing only whitespace, but preserve empty lines + // in messages that have content (e.g., for paragraph spacing). + if !slices.ContainsFunc(lines, func(line string) bool { + return strings.TrimSpace(line) != "" + }) { + return nil, nil + } + return lines, nil } var ErrHookTemplateParse = errors.New("failed to parse hook template") diff --git a/cli-plugins/hooks/template_test.go b/cli-plugins/hooks/template_test.go index 28b290696f1d..81d6c9175cfe 100644 --- a/cli-plugins/hooks/template_test.go +++ b/cli-plugins/hooks/template_test.go @@ -33,7 +33,17 @@ func TestParseTemplate(t *testing.T) { { doc: "empty template", template: "", - expectedOutput: []string{""}, + expectedOutput: nil, + }, + { + doc: "only empty lines", + template: "\n\n", + expectedOutput: nil, + }, + { + doc: "whitespace-only lines", + template: " \n\t\n ", + expectedOutput: nil, }, { doc: "plain message", diff --git a/cli-plugins/manager/hooks.go b/cli-plugins/manager/hooks.go index 5a3d3ebe9191..ffa70374e48c 100644 --- a/cli-plugins/manager/hooks.go +++ b/cli-plugins/manager/hooks.go @@ -49,11 +49,18 @@ func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, sub } func runHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCommand *cobra.Command, invokedCommand string, flags map[string]string, cmdErrorMessage string) { - nextSteps := invokeAndCollectHooks(ctx, cfg, rootCmd, subCommand, invokedCommand, flags, cmdErrorMessage) - hooks.PrintNextSteps(subCommand.ErrOrStderr(), nextSteps) + messages := invokeAndCollectHooks(ctx, cfg, rootCmd, invokedCommand, flags, cmdErrorMessage) + hooks.PrintMessages(subCommand.ErrOrStderr(), messages, func(message hooks.Response) []string { + lines, err := hooks.ParseTemplate(message.Template, subCommand) + if err != nil { + logrus.WithError(err).Debug("Failed to evaluate hook template") + return nil + } + return lines + }) } -func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCmd *cobra.Command, subCmdStr string, flags map[string]string, cmdErrorMessage string) []string { +func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd *cobra.Command, subCmdStr string, flags map[string]string, cmdErrorMessage string) []hooks.Response { if ctx.Err() != nil { return nil } @@ -64,17 +71,17 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root } pluginDirs := getPluginDirs(cfg) - nextSteps := make([]string, 0, len(pluginsCfg)) + var collected []hooks.Response - tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (messages []string, ok bool, err error) { + tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (hooks.Response, error) { match, matched := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage) if !matched { - return nil, false, nil + return hooks.Response{}, nil } p, err := getPlugin(pluginName, pluginDirs, rootCmd) if err != nil { - return nil, false, err + return hooks.Response{}, err } resp, err := p.RunHook(ctx, hooks.Request{ @@ -83,29 +90,23 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root CommandError: cmdErrorMessage, }) if err != nil { - return nil, false, err + return hooks.Response{}, err } var message hooks.Response if err := json.Unmarshal(resp, &message); err != nil { - return nil, false, fmt.Errorf("failed to unmarshal hook response (%q): %w", string(resp), err) - } - - // currently the only hook type - if message.Type != hooks.NextSteps { - return nil, false, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type))) + return hooks.Response{}, fmt.Errorf("failed to unmarshal hook response (%q): %w", string(resp), err) } - messages, err = hooks.ParseTemplate(message.Template, subCmd) - if err != nil { - return nil, false, err + if message.Type != hooks.NextSteps && message.Type != hooks.GenericMessage { + return hooks.Response{}, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type))) } - return messages, true, nil + return message, nil } for pluginName, pluginCfg := range pluginsCfg { - messages, ok, err := tryInvokeHook(pluginName, pluginCfg) + message, err := tryInvokeHook(pluginName, pluginCfg) if err != nil { // skip misbehaving plugins, but don't halt execution logrus.WithFields(logrus.Fields{ @@ -114,38 +115,13 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root }).Debug("Plugin hook invocation failed") continue } - if !ok { + if message.Template == "" { continue } - var appended bool - nextSteps, appended = appendNextSteps(nextSteps, messages) - if !appended { - logrus.WithFields(logrus.Fields{ - "plugin": pluginName, - }).Debug("Plugin responded with an empty hook message; ignoring") - } - } - return nextSteps -} - -// appendNextSteps appends the processed hook output to the nextSteps slice. -// If the processed hook output is empty, it is not appended. -// Empty lines are not stripped if there's at least one non-empty line. -func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) { - empty := true - for _, l := range processed { - if strings.TrimSpace(l) != "" { - empty = false - break - } - } - - if empty { - return nextSteps, false + collected = append(collected, message) } - - return append(nextSteps, processed...), true + return collected } // pluginMatch takes a plugin configuration and a string representing the diff --git a/cli-plugins/manager/hooks_test.go b/cli-plugins/manager/hooks_test.go index e39a1383118f..6f2f376e238f 100644 --- a/cli-plugins/manager/hooks_test.go +++ b/cli-plugins/manager/hooks_test.go @@ -2,6 +2,11 @@ package manager import ( "context" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" "testing" "github.com/docker/cli/cli/config/configfile" @@ -258,38 +263,6 @@ func TestMatchHookConfig(t *testing.T) { } } -func TestAppendNextSteps(t *testing.T) { - testCases := []struct { - processed []string - expectedOut []string - }{ - { - processed: []string{}, - expectedOut: []string{}, - }, - { - processed: []string{"", ""}, - expectedOut: []string{}, - }, - { - processed: []string{"Some hint", "", "Some other hint"}, - expectedOut: []string{"Some hint", "", "Some other hint"}, - }, - { - processed: []string{"Hint 1", "Hint 2"}, - expectedOut: []string{"Hint 1", "Hint 2"}, - }, - } - - for _, tc := range testCases { - t.Run("", func(t *testing.T) { - got, appended := appendNextSteps([]string{}, tc.processed) - assert.Check(t, is.DeepEqual(got, tc.expectedOut)) - assert.Check(t, is.Equal(appended, len(got) > 0)) - }) - } -} - func TestRunPluginHooksPassesErrorMessage(t *testing.T) { cfg := configfile.New("") cfg.Plugins = map[string]map[string]string{ @@ -336,7 +309,7 @@ func TestInvokeAndCollectHooksErrorHooksSkippedOnSuccess(t *testing.T) { // 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, + context.Background(), cfg, root, "build", map[string]string{}, "", ) assert.Check(t, is.Len(result, 0)) @@ -349,7 +322,7 @@ func TestInvokeAndCollectHooksNoPlugins(t *testing.T) { root.AddCommand(sub) result := invokeAndCollectHooks( - context.Background(), cfg, root, sub, + context.Background(), cfg, root, "build", map[string]string{}, "some error", ) assert.Check(t, is.Len(result, 0)) @@ -368,8 +341,108 @@ func TestInvokeAndCollectHooksCancelledContext(t *testing.T) { cancel() // cancel immediately result := invokeAndCollectHooks( - ctx, cfg, root, sub, + ctx, cfg, root, "build", map[string]string{}, "exit status 1", ) assert.Check(t, is.Nil(result)) } + +func TestRunHooksMessages(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test plugins require a POSIX shell") + } + + const header = "\n\x1b[1mWhat's next:\x1b[0m\n" + for _, tc := range []struct { + doc string + responses map[string]string + expected string + }{ + { + doc: "generic message preserves formatting and expands templates", + responses: map[string]string{ + "status": `{"Type":1,"Template":"Status for {{command}}\n\n Details"}`, + }, + expected: "\nStatus for build\n\n Details\n", + }, + { + doc: "omitted type defaults to next steps", + responses: map[string]string{ + "suggestion": `{"Template":"Try another command"}`, + }, + expected: header + " Try another command\n", + }, + { + doc: "generic messages precede shared next steps header", + responses: map[string]string{ + "statusone": `{"Type":1,"Template":"Session active"}`, + "statustwo": `{"Type":1,"Template":"Session active"}`, + "suggestionone": `{"Type":0,"Template":"Try another command"}`, + "suggestiontwo": `{"Type":0,"Template":"Try another command"}`, + }, + expected: "\nSession active\nSession active\n" + header + " Try another command\n Try another command\n", + }, + { + doc: "empty responses produce no output", + responses: map[string]string{ + "status": `{"Type":1,"Template":" \n\t"}`, + "suggestion": `{"Type":0,"Template":""}`, + }, + }, + { + doc: "empty generic message does not affect next steps", + responses: map[string]string{ + "status": `{"Type":1,"Template":""}`, + "suggestion": `{"Type":0,"Template":"Try another command"}`, + }, + expected: header + " Try another command\n", + }, + { + doc: "invalid responses do not affect other plugins", + responses: map[string]string{ + "unknown": `{"Type":99,"Template":"Ignored"}`, + "invalid": `{"Type":1,"Template":"{{"}`, + "status": `{"Type":1,"Template":"Session active"}`, + "suggestion": `{"Type":0,"Template":"Try another command"}`, + }, + expected: "\nSession active\n" + header + " Try another command\n", + }, + } { + t.Run(tc.doc, func(t *testing.T) { + pluginDir := t.TempDir() + cfg := configfile.New("") + cfg.CLIPluginsExtraDirs = []string{pluginDir} + cfg.Plugins = make(map[string]map[string]string) + for name, response := range tc.responses { + plugin := fmt.Sprintf(`#!/bin/sh +case "$1" in +docker-cli-plugin-metadata) + printf '%%s\n' '{"SchemaVersion":"0.1.0","Vendor":"test"}' + ;; +*) + test "$2" = docker-cli-plugin-hooks || exit 1 + printf 'called\n' >> "$0.calls" + printf '%%s\n' '%s' + ;; +esac +`, response) + assert.NilError(t, os.WriteFile(filepath.Join(pluginDir, "docker-"+name), []byte(plugin), 0o755)) + cfg.Plugins[name] = map[string]string{"hooks": "build"} + } + + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + var output strings.Builder + sub.SetErr(&output) + + runHooks(context.Background(), cfg, root, sub, "build", nil, "") + assert.Equal(t, output.String(), tc.expected) + for name := range tc.responses { + calls, err := os.ReadFile(filepath.Join(pluginDir, "docker-"+name+".calls")) + assert.NilError(t, err) + assert.Equal(t, string(calls), "called\n", "each plugin hook must run exactly once") + } + }) + } +}