Merge pull request #7378 from thaJeztah/pr_7371_alt

cli-plugins: Add generic hook messages (alternative)

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

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

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) {

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

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",

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)
+			return hooks.Response{}, 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.Response{}, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type)))
 		}
 
-		messages, err = hooks.ParseTemplate(message.Template, subCmd)
-		if err != nil {
-			return nil, false, err
-		}
-
-		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")
-		}
+		collected = append(collected, message)
 	}
-	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
-	}
-
-	return append(nextSteps, processed...), true
+	return collected
 }
 
 // pluginMatch takes a plugin configuration and a string representing the

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