From 296a6f5872d217e8f17953f3272093f98b91e92d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= <pawel.gronowski@docker.com>
Date: Fri, 17 May 2024 10:59:54 +0200
Subject: [PATCH] plugins/hooks: Don't show empty hooks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Don't show `Next steps:` with no messages at all when plugin returns an
unitialized value of `HookMessage` (zero-initialization sets its type to
NextSteps and empty template).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
---
 cli-plugins/manager/hooks.go      | 27 ++++++++++++++++++++++++-
 cli-plugins/manager/hooks_test.go | 33 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/cli-plugins/manager/hooks.go b/cli-plugins/manager/hooks.go
index a0f4d5ee46..5125c56d06 100644
--- a/cli-plugins/manager/hooks.go
+++ b/cli-plugins/manager/hooks.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/docker/cli/cli-plugins/hooks"
 	"github.com/docker/cli/cli/command"
+	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -100,11 +101,35 @@ func invokeAndCollectHooks(ctx context.Context, dockerCli command.Cli, rootCmd,
 		if err != nil {
 			continue
 		}
-		nextSteps = append(nextSteps, processedHook...)
+
+		var appended bool
+		nextSteps, appended = appendNextSteps(nextSteps, processedHook)
+		if !appended {
+			logrus.Debugf("Plugin %s responded with an empty hook message %q. Ignoring.", pluginName, string(hookReturn))
+		}
 	}
 	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
+}
+
 // pluginMatch takes a plugin configuration and a string representing the
 // command being executed (such as 'image ls' – the root 'docker' is omitted)
 // and, if the configuration includes a hook for the invoked command, returns
diff --git a/cli-plugins/manager/hooks_test.go b/cli-plugins/manager/hooks_test.go
index c532702f7b..3060a2e9f1 100644
--- a/cli-plugins/manager/hooks_test.go
+++ b/cli-plugins/manager/hooks_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 )
 
 func TestGetNaiveFlags(t *testing.T) {
@@ -108,3 +109,35 @@ func TestPluginMatch(t *testing.T) {
 		assert.Equal(t, match, tc.expectedMatch)
 	}
 }
+
+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))
+		})
+	}
+}