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") + } + }) + } +}