praefect: migrate dial-nodes sub-command

The 'dial-nodes' sub-command migrated from the old
implementation to the new approach with usage of the
third party library. The invocation of the command remains
the same, but it has more descriptive representation when
help is invoked. Also, it is possible now to invoke help
only for the sub-command to see more details about it and
its usage.

Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/5001

5 files changed, 75 insertions, 44 deletions

diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index 6d453ab4c..a665d9445 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -26,13 +26,6 @@
 //
 //	praefect -config PATH_TO_CONFIG sql-migrate-status
 //
-// # Dial Nodes
-//
-// The subcommand "dial-nodes" helps diagnose connection problems to Gitaly or
-// Praefect. The subcommand works by sourcing the connection information from
-// the config file, and then dialing and health checking the remote nodes.
-//
-//	praefect -config PATH_TO_CONFIG dial-nodes
 
 package praefect
 
@@ -81,6 +74,7 @@ func NewApp() *cli.App {
 			newAcceptDatalossCommand(),
 			newCheckCommand(service.AllChecks()),
 			newDatalossCommand(),
+			newDialNodesCommand(),
 		},
 		Flags: []cli.Flag{
 			&cli.StringFlag{
diff --git a/internal/cli/praefect/main_test.go b/internal/cli/praefect/main_test.go
index cff3d62cd..dd8878236 100644
--- a/internal/cli/praefect/main_test.go
+++ b/internal/cli/praefect/main_test.go
@@ -3,8 +3,11 @@ package praefect
 import (
 	"errors"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
+	"github.com/pelletier/go-toml/v2"
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
 	"github.com/stretchr/testify/assert"
@@ -12,6 +15,7 @@ import (
 	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/bootstrap"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/bootstrap/starter"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
@@ -235,3 +239,13 @@ func TestExcludeDatabaseMetricsFromDefaultMetrics(t *testing.T) {
 		})
 	}
 }
+
+func writeConfigToFile(tb testing.TB, conf config.Config) string {
+	tb.Helper()
+	confData, err := toml.Marshal(conf)
+	require.NoError(tb, err)
+	tmpDir := testhelper.TempDir(tb)
+	confPath := filepath.Join(tmpDir, "config.toml")
+	require.NoError(tb, os.WriteFile(confPath, confData, perm.PublicFile))
+	return confPath
+}
diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go
index a594ae369..fa8bbed28 100644
--- a/internal/cli/praefect/subcmd.go
+++ b/internal/cli/praefect/subcmd.go
@@ -35,7 +35,6 @@ func subcommands(logger *logrus.Entry) map[string]subcmd {
 	return map[string]subcmd{
 		sqlPingCmdName:                &sqlPingSubcommand{},
 		sqlMigrateCmdName:             newSQLMigrateSubCommand(os.Stdout),
-		dialNodesCmdName:              newDialNodesSubcommand(os.Stdout),
 		sqlMigrateDownCmdName:         &sqlMigrateDownSubcommand{},
 		sqlMigrateStatusCmdName:       &sqlMigrateStatusSubcommand{},
 		setReplicationFactorCmdName:   newSetReplicatioFactorSubcommand(os.Stdout),
diff --git a/internal/cli/praefect/subcmd_dial_nodes.go b/internal/cli/praefect/subcmd_dial_nodes.go
index 3608a9be7..debf1f908 100644
--- a/internal/cli/praefect/subcmd_dial_nodes.go
+++ b/internal/cli/praefect/subcmd_dial_nodes.go
@@ -2,46 +2,51 @@ package praefect
 
 import (
 	"context"
-	"flag"
-	"io"
-	"time"
 
-	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/nodes"
 )
 
-const dialNodesCmdName = "dial-nodes"
-
-func newDialNodesSubcommand(w io.Writer) *dialNodesSubcommand {
-	return &dialNodesSubcommand{
-		w: w,
+func newDialNodesCommand() *cli.Command {
+	return &cli.Command{
+		Name:  "dial-nodes",
+		Usage: "checks connection with remote nodes",
+		Description: "The subcommand \"dial-nodes\" helps diagnose connection problems to Gitaly or\n" +
+			"Praefect. The subcommand works by sourcing the connection information from\n" +
+			"the config file, and then dialing and health checking the remote nodes.",
+		Action: dialNodesAction,
+		Flags: []cli.Flag{
+			&cli.DurationFlag{
+				Name:  "timeout",
+				Usage: "timeout for dialing gitaly nodes",
+				Value: 0,
+			},
+		},
+		Before: func(ctx *cli.Context) error {
+			if ctx.Args().Present() {
+				_ = cli.ShowSubcommandHelp(ctx)
+				return cli.Exit(unexpectedPositionalArgsError{Command: ctx.Command.Name}, 1)
+			}
+			return nil
+		},
 	}
 }
 
-type dialNodesSubcommand struct {
-	w       io.Writer
-	timeout time.Duration
-}
-
-func (s *dialNodesSubcommand) FlagSet() *flag.FlagSet {
-	fs := flag.NewFlagSet(dialNodesCmdName, flag.ExitOnError)
-	fs.DurationVar(&s.timeout, "timeout", 0, "timeout for dialing gitaly nodes")
-	fs.Usage = func() {
-		printfErr("Description:\n" +
-			"	This command attempts to reach all Gitaly nodes.\n")
-		fs.PrintDefaults()
+func dialNodesAction(ctx *cli.Context) error {
+	logger := log.Default()
+	conf, err := getConfig(logger, ctx.String(configFlagName))
+	if err != nil {
+		return err
 	}
 
-	return fs
-}
-
-func (s *dialNodesSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error {
-	if s.timeout == 0 {
-		s.timeout = defaultDialTimeout
+	timeout := ctx.Duration("timeout")
+	if timeout == 0 {
+		timeout = defaultDialTimeout
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), s.timeout)
+	timeCtx, cancel := context.WithTimeout(ctx.Context, timeout)
 	defer cancel()
 
-	return nodes.PingAll(ctx, conf, nodes.NewTextPrinter(s.w), false)
+	return nodes.PingAll(timeCtx, conf, nodes.NewTextPrinter(ctx.App.Writer), false)
 }
diff --git a/internal/cli/praefect/subcmd_dial_nodes_test.go b/internal/cli/praefect/subcmd_dial_nodes_test.go
index f511c8872..ffb977f74 100644
--- a/internal/cli/praefect/subcmd_dial_nodes_test.go
+++ b/internal/cli/praefect/subcmd_dial_nodes_test.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -23,6 +24,7 @@ func (m mockServerService) ServerInfo(ctx context.Context, r *gitalypb.ServerInf
 }
 
 func TestSubCmdDialNodes(t *testing.T) {
+	t.Parallel()
 	var resp *gitalypb.ServerInfoResponse
 	mockSvc := &mockServerService{
 		serverInfoFunc: func(_ context.Context, _ *gitalypb.ServerInfoRequest) (*gitalypb.ServerInfoResponse, error) {
@@ -46,14 +48,21 @@ func TestSubCmdDialNodes(t *testing.T) {
 
 	for _, tt := range []struct {
 		name   string
+		args   []string
 		conf   config.Config
 		resp   *gitalypb.ServerInfoResponse
 		logs   string
 		errMsg string
 	}{
 		{
+			name:   "positional arguments",
+			args:   []string{"positional-arg"},
+			errMsg: cli.Exit(unexpectedPositionalArgsError{Command: "dial-nodes"}, 1).Error(),
+		},
+		{
 			name: "2 virtuals, 2 storages, 1 node",
 			conf: config.Config{
+				SocketPath: ln.Addr().String(),
 				VirtualStorages: []*config.VirtualStorage{
 					{
 						Name: "default",
@@ -104,6 +113,7 @@ func TestSubCmdDialNodes(t *testing.T) {
 		{
 			name: "node unreachable",
 			conf: config.Config{
+				SocketPath: ln.Addr().String(),
 				VirtualStorages: []*config.VirtualStorage{
 					{
 						Name: "default",
@@ -123,17 +133,26 @@ func TestSubCmdDialNodes(t *testing.T) {
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			resp = tt.resp
-			tt.conf.SocketPath = ln.Addr().String()
-
-			output := &bytes.Buffer{}
-
-			cmd := dialNodesSubcommand{w: output, timeout: 1 * time.Second}
+			confPath := writeConfigToFile(t, tt.conf)
 
-			err := cmd.Exec(nil, tt.conf)
+			var stdout bytes.Buffer
+			app := cli.App{
+				Writer: &stdout,
+				Commands: []*cli.Command{
+					newDialNodesCommand(),
+				},
+				Flags: []cli.Flag{
+					&cli.StringFlag{
+						Name:  "config",
+						Value: confPath,
+					},
+				},
+			}
 
+			err := app.Run(append([]string{progname, "dial-nodes", "-timeout", time.Second.String()}, tt.args...))
 			if tt.errMsg == "" {
 				require.NoError(t, err)
-				require.Equal(t, tt.logs, output.String())
+				require.Equal(t, tt.logs, stdout.String())
 				return
 			}