From 1d09f850503a7e18236d1857204040bd77384d8f 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 using evaluated messages. Keep PrintNextSteps as a deprecated wrapper to preserve existing callers and output. Signed-off-by: Nicolas Beck --- cli-plugins/hooks/hook_types.go | 4 + cli-plugins/hooks/printer.go | 49 ++++++++--- cli-plugins/hooks/printer_test.go | 78 ++++++++++++++++- cli-plugins/manager/hooks.go | 49 +++++------ cli-plugins/manager/hooks_test.go | 134 +++++++++++++++++++++++++++--- 5 files changed, 267 insertions(+), 47 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..0f519d9b9aec 100644 --- a/cli-plugins/hooks/printer.go +++ b/cli-plugins/hooks/printer.go @@ -7,17 +7,46 @@ 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 +// EvaluatedMessage contains the lines of a hook response after template evaluation. +type EvaluatedMessage struct { + Type ResponseType + Lines []string +} + +// 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 []EvaluatedMessage) { + grouped := make(map[ResponseType][]string) + for _, message := range messages { + grouped[message.Type] = append(grouped[message.Type], message.Lines...) } - _, _ = io.WriteString(out, whatsNext) - for _, msg := range messages { - _, _ = io.WriteString(out, indent) - _, _ = io.WriteString(out, msg) - _, _ = io.WriteString(out, "\n") + for _, responseType := range []ResponseType{GenericMessage, NextSteps} { + lines := grouped[responseType] + if len(lines) == 0 { + continue + } + + 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, []EvaluatedMessage{{Type: NextSteps, Lines: messages}}) +} diff --git a/cli-plugins/hooks/printer_test.go b/cli-plugins/hooks/printer_test.go index 8b1ca9234ed8..ab9d7ab17576 100644 --- a/cli-plugins/hooks/printer_test.go +++ b/cli-plugins/hooks/printer_test.go @@ -8,7 +8,78 @@ 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.EvaluatedMessage + expected string + }{ + {doc: "no messages"}, + { + doc: "no lines", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.GenericMessage}, + {Type: hooks.NextSteps, Lines: []string{}}, + }, + }, + { + doc: "generic message", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.GenericMessage, Lines: []string{"Session active"}}, + }, + expected: "\nSession active\n", + }, + { + doc: "preserve message formatting", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.GenericMessage, Lines: []string{"Session active", "", " Details"}}, + }, + expected: "\nSession active\n\n Details\n", + }, + { + doc: "next steps", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.NextSteps, Lines: []string{"Try another command"}}, + }, + expected: header + " Try another command\n", + }, + { + doc: "group interleaved types and preserve order within each type", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.NextSteps, Lines: []string{"First suggestion"}}, + {Type: hooks.GenericMessage, Lines: []string{"Session active"}}, + {Type: hooks.NextSteps, Lines: []string{"Second suggestion", "Third suggestion"}}, + {Type: hooks.GenericMessage, Lines: []string{"Another status"}}, + }, + expected: "\nSession active\nAnother status\n" + header + " First suggestion\n Second suggestion\n Third suggestion\n", + }, + { + doc: "unknown type", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.ResponseType(99), Lines: []string{"Ignored"}}, + }, + }, + { + doc: "unknown type does not affect known messages", + messages: []hooks.EvaluatedMessage{ + {Type: hooks.ResponseType(99), Lines: []string{"Ignored"}}, + {Type: hooks.GenericMessage, Lines: []string{"Session active"}}, + {Type: hooks.NextSteps, Lines: []string{"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) + 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 +105,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/manager/hooks.go b/cli-plugins/manager/hooks.go index 5a3d3ebe9191..75cfba7095a6 100644 --- a/cli-plugins/manager/hooks.go +++ b/cli-plugins/manager/hooks.go @@ -49,11 +49,13 @@ 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, subCommand, invokedCommand, flags, cmdErrorMessage) + hooks.PrintMessages(subCommand.ErrOrStderr(), messages) } -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, subCmd *cobra.Command, subCmdStr string, flags map[string]string, cmdErrorMessage string, +) []hooks.EvaluatedMessage { if ctx.Err() != nil { return nil } @@ -64,17 +66,17 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root } pluginDirs := getPluginDirs(cfg) - nextSteps := make([]string, 0, len(pluginsCfg)) + var collected []hooks.EvaluatedMessage - tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (messages []string, ok bool, err error) { + tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (hooks.EvaluatedMessage, error) { match, matched := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage) if !matched { - return nil, false, nil + return hooks.EvaluatedMessage{}, nil } p, err := getPlugin(pluginName, pluginDirs, rootCmd) if err != nil { - return nil, false, err + return hooks.EvaluatedMessage{}, err } resp, err := p.RunHook(ctx, hooks.Request{ @@ -83,29 +85,28 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root CommandError: cmdErrorMessage, }) if err != nil { - return nil, false, err + return hooks.EvaluatedMessage{}, 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) + return hooks.EvaluatedMessage{}, 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))) + if message.Type != hooks.NextSteps && message.Type != hooks.GenericMessage { + return hooks.EvaluatedMessage{}, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type))) } - messages, err = hooks.ParseTemplate(message.Template, subCmd) + lines, err := hooks.ParseTemplate(message.Template, subCmd) if err != nil { - return nil, false, err + return hooks.EvaluatedMessage{}, err } - return messages, true, nil + return hooks.EvaluatedMessage{Type: message.Type, Lines: lines}, 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,27 +115,27 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root }).Debug("Plugin hook invocation failed") continue } - if !ok { + if message.Lines == nil { continue } var appended bool - nextSteps, appended = appendNextSteps(nextSteps, messages) + collected, appended = appendMessages(collected, message) if !appended { logrus.WithFields(logrus.Fields{ "plugin": pluginName, }).Debug("Plugin responded with an empty hook message; ignoring") } } - return nextSteps + return collected } -// appendNextSteps appends the processed hook output to the nextSteps slice. +// appendMessages appends the processed hook output to the messages 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) { +func appendMessages(messages []hooks.EvaluatedMessage, processed hooks.EvaluatedMessage) ([]hooks.EvaluatedMessage, bool) { empty := true - for _, l := range processed { + for _, l := range processed.Lines { if strings.TrimSpace(l) != "" { empty = false break @@ -142,10 +143,10 @@ func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) { } if empty { - return nextSteps, false + return messages, false } - return append(nextSteps, processed...), true + return append(messages, processed), true } // 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..dbfd3830255a 100644 --- a/cli-plugins/manager/hooks_test.go +++ b/cli-plugins/manager/hooks_test.go @@ -2,8 +2,14 @@ package manager import ( "context" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" "testing" + "github.com/docker/cli/cli-plugins/hooks" "github.com/docker/cli/cli/config/configfile" "github.com/spf13/cobra" "gotest.tools/v3/assert" @@ -258,32 +264,36 @@ func TestMatchHookConfig(t *testing.T) { } } -func TestAppendNextSteps(t *testing.T) { +func TestAppendMessages(t *testing.T) { testCases := []struct { - processed []string - expectedOut []string + processed hooks.EvaluatedMessage + expectedOut []hooks.EvaluatedMessage }{ { - processed: []string{}, - expectedOut: []string{}, + processed: hooks.EvaluatedMessage{Type: hooks.GenericMessage}, + expectedOut: []hooks.EvaluatedMessage{}, }, { - processed: []string{"", ""}, - expectedOut: []string{}, + processed: hooks.EvaluatedMessage{Type: hooks.NextSteps, Lines: []string{"", ""}}, + expectedOut: []hooks.EvaluatedMessage{}, }, { - processed: []string{"Some hint", "", "Some other hint"}, - expectedOut: []string{"Some hint", "", "Some other hint"}, + processed: hooks.EvaluatedMessage{Type: hooks.GenericMessage, Lines: []string{"Some hint", "", "Some other hint"}}, + expectedOut: []hooks.EvaluatedMessage{ + {Type: hooks.GenericMessage, Lines: []string{"Some hint", "", "Some other hint"}}, + }, }, { - processed: []string{"Hint 1", "Hint 2"}, - expectedOut: []string{"Hint 1", "Hint 2"}, + processed: hooks.EvaluatedMessage{Type: hooks.NextSteps, Lines: []string{"Hint 1", "Hint 2"}}, + expectedOut: []hooks.EvaluatedMessage{ + {Type: hooks.NextSteps, Lines: []string{"Hint 1", "Hint 2"}}, + }, }, } for _, tc := range testCases { t.Run("", func(t *testing.T) { - got, appended := appendNextSteps([]string{}, tc.processed) + got, appended := appendMessages([]hooks.EvaluatedMessage{}, tc.processed) assert.Check(t, is.DeepEqual(got, tc.expectedOut)) assert.Check(t, is.Equal(appended, len(got) > 0)) }) @@ -373,3 +383,103 @@ func TestInvokeAndCollectHooksCancelledContext(t *testing.T) { ) 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") + } + }) + } +}