praefect: print verbose output in check subcommand

When an admin runs the praefect startup checks, it would be useful to
print more information about each check. This change refactors some of
the code to allow that to happen, and also a -q flag that the user can
pass to silence this detailed outpt.

Changelog: changed

6 files changed, 141 insertions, 65 deletions

diff --git a/cmd/praefect/subcmd.go b/cmd/praefect/subcmd.go
index 05718b073..fc67c7471 100644
--- a/cmd/praefect/subcmd.go
+++ b/cmd/praefect/subcmd.go
@@ -23,12 +23,17 @@ type subcmd interface {
 	Exec(flags *flag.FlagSet, config config.Config) error
 }
 
-const defaultDialTimeout = 30 * time.Second
+const (
+	defaultDialTimeout        = 30 * time.Second
+	paramVirtualStorage       = "virtual-storage"
+	paramRelativePath         = "repository"
+	paramAuthoritativeStorage = "authoritative-storage"
+)
 
 var subcommands = map[string]subcmd{
 	sqlPingCmdName:                &sqlPingSubcommand{},
 	sqlMigrateCmdName:             &sqlMigrateSubcommand{},
-	dialNodesCmdName:              newDialNodesSubcommand(logger),
+	dialNodesCmdName:              newDialNodesSubcommand(os.Stdout),
 	sqlMigrateDownCmdName:         &sqlMigrateDownSubcommand{},
 	sqlMigrateStatusCmdName:       &sqlMigrateStatusSubcommand{},
 	datalossCmdName:               newDatalossSubcommand(),
diff --git a/cmd/praefect/subcmd_accept_dataloss.go b/cmd/praefect/subcmd_accept_dataloss.go
index 04b076743..944c55e89 100644
--- a/cmd/praefect/subcmd_accept_dataloss.go
+++ b/cmd/praefect/subcmd_accept_dataloss.go
@@ -10,10 +10,7 @@ import (
 )
 
 const (
-	acceptDatalossCmdName     = "accept-dataloss"
-	paramVirtualStorage       = "virtual-storage"
-	paramRelativePath         = "repository"
-	paramAuthoritativeStorage = "authoritative-storage"
+	acceptDatalossCmdName = "accept-dataloss"
 )
 
 type acceptDatalossSubcommand struct {
diff --git a/cmd/praefect/subcmd_check.go b/cmd/praefect/subcmd_check.go
index 246e6e0bf..5755b2008 100644
--- a/cmd/praefect/subcmd_check.go
+++ b/cmd/praefect/subcmd_check.go
@@ -18,6 +18,7 @@ const (
 
 type checkSubcommand struct {
 	w          io.Writer
+	quiet      bool
 	checkFuncs []praefect.CheckFunc
 }
 
@@ -30,6 +31,7 @@ func newCheckSubcommand(writer io.Writer, checkFuncs ...praefect.CheckFunc) *che
 
 func (cmd *checkSubcommand) FlagSet() *flag.FlagSet {
 	fs := flag.NewFlagSet(checkCmdName, flag.ExitOnError)
+	fs.BoolVar(&cmd.quiet, "q", false, "do not print out verbose output about each check")
 	fs.Usage = func() {
 		printfErr("Description:\n" +
 			"	This command runs startup checks for Praefect.")
@@ -44,7 +46,7 @@ var errFatalChecksFailed = errors.New("checks failed")
 func (cmd *checkSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) error {
 	var allChecks []*praefect.Check
 	for _, checkFunc := range cmd.checkFuncs {
-		allChecks = append(allChecks, checkFunc(cfg))
+		allChecks = append(allChecks, checkFunc(cfg, cmd.w, cmd.quiet))
 	}
 
 	bgContext := context.Background()
@@ -54,7 +56,8 @@ func (cmd *checkSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) error {
 		ctx, cancel := context.WithTimeout(bgContext, 5*time.Second)
 		defer cancel()
 
-		fmt.Fprintf(cmd.w, "Checking %s...", check.Name)
+		cmd.printCheckDetails(check)
+
 		if err := check.Run(ctx); err != nil {
 			failedChecks++
 			if check.Severity == praefect.Fatal {
@@ -81,3 +84,12 @@ func (cmd *checkSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) error {
 
 	return nil
 }
+
+func (cmd *checkSubcommand) printCheckDetails(check *praefect.Check) {
+	if cmd.quiet {
+		fmt.Fprintf(cmd.w, "Checking %s...", check.Name)
+		return
+	}
+
+	fmt.Fprintf(cmd.w, "Checking %s - %s [%s]\n", check.Name, check.Description, check.Severity)
+}
diff --git a/cmd/praefect/subcmd_check_test.go b/cmd/praefect/subcmd_check_test.go
index ec9108a54..ef6d489d5 100644
--- a/cmd/praefect/subcmd_check_test.go
+++ b/cmd/praefect/subcmd_check_test.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"flag"
+	"io"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -16,106 +17,166 @@ func TestCheckSubcommand_Exec(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
-		desc           string
-		checks         []praefect.CheckFunc
-		expectedOutput string
-		expectedError  error
+		desc                string
+		checks              []praefect.CheckFunc
+		expectedQuietOutput string
+		expectedOutput      string
+		expectedError       error
 	}{
 		{
 			desc: "all checks pass",
 			checks: []praefect.CheckFunc{
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 1",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 1",
+						Description: "checks a",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 2",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 2",
+						Description: "checks b",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 3",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 3",
+						Description: "checks c",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
 			},
-			expectedOutput: "Checking check 1...Passed\nChecking check 2...Passed\nChecking check 3...Passed\n\nAll checks passed.\n",
-			expectedError:  nil,
+			expectedOutput: `Checking check 1 - checks a [fatal]
+Passed
+Checking check 2 - checks b [fatal]
+Passed
+Checking check 3 - checks c [fatal]
+Passed
+
+All checks passed.
+`,
+			expectedQuietOutput: `Checking check 1...Passed
+Checking check 2...Passed
+Checking check 3...Passed
+
+All checks passed.
+`,
+			expectedError: nil,
 		},
 		{
 			desc: "a fatal check fails",
 			checks: []praefect.CheckFunc{
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 1",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 1",
+						Description: "checks a",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 2",
-						Run:      func(ctx context.Context) error { return errors.New("i failed") },
-						Severity: praefect.Fatal,
+						Name:        "check 2",
+						Description: "checks b",
+						Run:         func(ctx context.Context) error { return errors.New("i failed") },
+						Severity:    praefect.Fatal,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 3",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 3",
+						Description: "checks c",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
 			},
-			expectedOutput: "Checking check 1...Passed\nChecking check 2...Failed (fatal) error: i failed\nChecking check 3...Passed\n\n1 check(s) failed, at least one was fatal.\n",
-			expectedError:  errFatalChecksFailed,
+			expectedOutput: `Checking check 1 - checks a [fatal]
+Passed
+Checking check 2 - checks b [fatal]
+Failed (fatal) error: i failed
+Checking check 3 - checks c [fatal]
+Passed
+
+1 check(s) failed, at least one was fatal.
+`,
+			expectedQuietOutput: `Checking check 1...Passed
+Checking check 2...Failed (fatal) error: i failed
+Checking check 3...Passed
+
+1 check(s) failed, at least one was fatal.
+`,
+			expectedError: errFatalChecksFailed,
 		},
 		{
 			desc: "only warning checks fail",
 			checks: []praefect.CheckFunc{
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 1",
-						Run:      func(ctx context.Context) error { return nil },
-						Severity: praefect.Fatal,
+						Name:        "check 1",
+						Description: "checks a",
+						Run:         func(ctx context.Context) error { return nil },
+						Severity:    praefect.Fatal,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 2",
-						Run:      func(ctx context.Context) error { return errors.New("i failed but not too badly") },
-						Severity: praefect.Warning,
+						Name:        "check 2",
+						Description: "checks b",
+						Run:         func(ctx context.Context) error { return errors.New("i failed but not too badly") },
+						Severity:    praefect.Warning,
 					}
 				},
-				func(cfg config.Config) *praefect.Check {
+				func(cfg config.Config, w io.Writer, quiet bool) *praefect.Check {
 					return &praefect.Check{
-						Name:     "check 3",
-						Run:      func(ctx context.Context) error { return errors.New("i failed but not too badly") },
-						Severity: praefect.Warning,
+						Name:        "check 3",
+						Description: "checks c",
+						Run:         func(ctx context.Context) error { return errors.New("i failed but not too badly") },
+						Severity:    praefect.Warning,
 					}
 				},
 			},
-			expectedOutput: "Checking check 1...Passed\nChecking check 2...Failed (warning) error: i failed but not too badly\nChecking check 3...Failed (warning) error: i failed but not too badly\n\n2 check(s) failed, but none are fatal.\n",
-			expectedError:  nil,
+			expectedOutput: `Checking check 1 - checks a [fatal]
+Passed
+Checking check 2 - checks b [warning]
+Failed (warning) error: i failed but not too badly
+Checking check 3 - checks c [warning]
+Failed (warning) error: i failed but not too badly
+
+2 check(s) failed, but none are fatal.
+`,
+			expectedQuietOutput: `Checking check 1...Passed
+Checking check 2...Failed (warning) error: i failed but not too badly
+Checking check 3...Failed (warning) error: i failed but not too badly
+
+2 check(s) failed, but none are fatal.
+`,
+			expectedError: nil,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			var cfg config.Config
-			var stdout bytes.Buffer
 
-			checkCmd := checkSubcommand{w: &stdout, checkFuncs: tc.checks}
+			t.Run("quiet", func(t *testing.T) {
+				var stdout bytes.Buffer
+				checkCmd := checkSubcommand{w: &stdout, checkFuncs: tc.checks, quiet: true}
+				assert.Equal(t, tc.expectedError, checkCmd.Exec(flag.NewFlagSet("", flag.PanicOnError), cfg))
+				assert.Equal(t, tc.expectedQuietOutput, stdout.String())
+			})
 
-			assert.Equal(t, tc.expectedError, checkCmd.Exec(flag.NewFlagSet("", flag.PanicOnError), cfg))
-			assert.Equal(t, tc.expectedOutput, stdout.String())
+			t.Run("normal", func(t *testing.T) {
+				var stdout bytes.Buffer
+				checkCmd := checkSubcommand{w: &stdout, checkFuncs: tc.checks, quiet: false}
+				assert.Equal(t, tc.expectedError, checkCmd.Exec(flag.NewFlagSet("", flag.PanicOnError), cfg))
+				assert.Equal(t, tc.expectedOutput, stdout.String())
+			})
 		})
 	}
 }
diff --git a/cmd/praefect/subcmd_dial_nodes.go b/cmd/praefect/subcmd_dial_nodes.go
index 215a5496e..06d7719ec 100644
--- a/cmd/praefect/subcmd_dial_nodes.go
+++ b/cmd/praefect/subcmd_dial_nodes.go
@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"flag"
+	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/nodes"
@@ -10,12 +11,14 @@ import (
 
 const dialNodesCmdName = "dial-nodes"
 
-func newDialNodesSubcommand(p nodes.Printer) *dialNodesSubcommand {
-	return &dialNodesSubcommand{p}
+func newDialNodesSubcommand(w io.Writer) *dialNodesSubcommand {
+	return &dialNodesSubcommand{
+		w: w,
+	}
 }
 
 type dialNodesSubcommand struct {
-	p nodes.Printer
+	w io.Writer
 }
 
 func (s *dialNodesSubcommand) FlagSet() *flag.FlagSet {
@@ -24,5 +27,5 @@ func (s *dialNodesSubcommand) FlagSet() *flag.FlagSet {
 
 func (s *dialNodesSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error {
 	ctx := context.Background()
-	return nodes.PingAll(ctx, conf, s.p)
+	return nodes.PingAll(ctx, conf, nodes.NewTextPrinter(s.w), false)
 }
diff --git a/cmd/praefect/subcmd_dial_nodes_test.go b/cmd/praefect/subcmd_dial_nodes_test.go
index 9714b031e..9817b32ad 100644
--- a/cmd/praefect/subcmd_dial_nodes_test.go
+++ b/cmd/praefect/subcmd_dial_nodes_test.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/nodes"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -105,9 +104,8 @@ func TestSubCmdDialNodes(t *testing.T) {
 			tt.conf.SocketPath = ln.Addr().String()
 
 			output := &bytes.Buffer{}
-			p := nodes.NewTextPrinter(output)
 
-			cmd := newDialNodesSubcommand(p)
+			cmd := newDialNodesSubcommand(output)
 			require.NoError(t, cmd.Exec(nil, tt.conf))
 
 			require.Equal(t, tt.logs, output.String())