praefect: Migrate sql-migrate sub-command

Migration of the 'sql-migrate' sub-command 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

4 files changed, 87 insertions, 52 deletions

diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index 6c159b6b8..23a6c7818 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -10,17 +10,6 @@
 //
 //	praefect -config PATH_TO_CONFIG sql-ping
 //
-// # SQL Migrate
-//
-// The subcommand "sql-migrate" will apply any outstanding SQL migrations.
-//
-//	praefect -config PATH_TO_CONFIG sql-migrate [-ignore-unknown=true|false]
-//
-// By default, the migration will ignore any unknown migrations that are
-// not known by the Praefect binary.
-//
-// "-ignore-unknown=false" will disable this behavior.
-//
 // The subcommand "sql-migrate-status" will show which SQL migrations have
 // been applied and which ones have not:
 //
@@ -81,6 +70,7 @@ func NewApp() *cli.App {
 			newTrackRepositoryCommand(),
 			newVerifyCommand(),
 			newMetadataCommand(),
+			newSQLMigrateCommand(),
 		},
 		Flags: []cli.Flag{
 			&cli.StringFlag{
diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go
index 1c0011736..74ddeddf1 100644
--- a/internal/cli/praefect/subcmd.go
+++ b/internal/cli/praefect/subcmd.go
@@ -34,7 +34,6 @@ const (
 func subcommands(logger *logrus.Entry) map[string]subcmd {
 	return map[string]subcmd{
 		sqlPingCmdName:              &sqlPingSubcommand{},
-		sqlMigrateCmdName:           newSQLMigrateSubCommand(os.Stdout),
 		sqlMigrateDownCmdName:       &sqlMigrateDownSubcommand{},
 		sqlMigrateStatusCmdName:     &sqlMigrateStatusSubcommand{},
 		setReplicationFactorCmdName: newSetReplicatioFactorSubcommand(os.Stdout),
diff --git a/internal/cli/praefect/subcmd_sql_migrate.go b/internal/cli/praefect/subcmd_sql_migrate.go
index 57020bb7b..46d941113 100644
--- a/internal/cli/praefect/subcmd_sql_migrate.go
+++ b/internal/cli/praefect/subcmd_sql_migrate.go
@@ -1,13 +1,12 @@
 package praefect
 
 import (
-	"flag"
 	"fmt"
-	"io"
 	"time"
 
 	migrate "github.com/rubenv/sql-migrate"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/config"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore/glsql"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore/migrations"
 )
@@ -17,25 +16,42 @@ const (
 	timeFmt           = "2006-01-02T15:04:05"
 )
 
-type sqlMigrateSubcommand struct {
-	w             io.Writer
-	ignoreUnknown bool
-	verbose       bool
-}
-
-func newSQLMigrateSubCommand(writer io.Writer) *sqlMigrateSubcommand {
-	return &sqlMigrateSubcommand{w: writer}
-}
-
-func (cmd *sqlMigrateSubcommand) FlagSet() *flag.FlagSet {
-	flags := flag.NewFlagSet(sqlMigrateCmdName, flag.ExitOnError)
-	flags.BoolVar(&cmd.ignoreUnknown, "ignore-unknown", true, "ignore unknown migrations (default is true)")
-	flags.BoolVar(&cmd.verbose, "verbose", false, "show text of migration query (default is false)")
-	return flags
+func newSQLMigrateCommand() *cli.Command {
+	return &cli.Command{
+		Name:  sqlMigrateCmdName,
+		Usage: "apply outstanding SQL migrations",
+		Description: "The sql-migrate subcommand applies outstanding migrations to the configured database.\n" +
+			"The subcommand doesn't fail if database has migrations unknown to the version of Praefect you're using.\n" +
+			"To make the subcommand fail on unknown migrations, use the 'ignore-unknown' flag.",
+		HideHelpCommand: true,
+		Action:          sqlMigrateAction,
+		Flags: []cli.Flag{
+			&cli.BoolFlag{
+				Name:  "ignore-unknown",
+				Usage: "ignore unknown migrations",
+				Value: true,
+			},
+			&cli.BoolFlag{
+				Name:  "verbose",
+				Usage: "show text of migration query",
+			},
+		},
+		Before: func(ctx *cli.Context) error {
+			if ctx.Args().Present() {
+				_ = cli.ShowSubcommandHelp(ctx)
+				return cli.Exit(unexpectedPositionalArgsError{Command: ctx.Command.Name}, 1)
+			}
+			return nil
+		},
+	}
 }
 
-func (cmd *sqlMigrateSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error {
-	const subCmd = progname + " " + sqlMigrateCmdName
+func sqlMigrateAction(appCtx *cli.Context) error {
+	logger := log.Default()
+	conf, err := getConfig(logger, appCtx.String(configFlagName))
+	if err != nil {
+		return err
+	}
 
 	db, clean, err := openDB(conf.DB)
 	if err != nil {
@@ -43,8 +59,9 @@ func (cmd *sqlMigrateSubcommand) Exec(flags *flag.FlagSet, conf config.Config) e
 	}
 	defer clean()
 
+	ignoreUnknown := appCtx.Bool("ignore-unknown")
 	migrationSet := migrate.MigrationSet{
-		IgnoreUnknown: cmd.ignoreUnknown,
+		IgnoreUnknown: ignoreUnknown,
 		TableName:     migrations.MigrationTableName,
 	}
 
@@ -55,40 +72,41 @@ func (cmd *sqlMigrateSubcommand) Exec(flags *flag.FlagSet, conf config.Config) e
 	// Find all migrations that are currently down.
 	planMigrations, _, _ := migrationSet.PlanMigration(db, "postgres", planSource, migrate.Up, 0)
 
+	subCmd := progname + " " + appCtx.Command.Name
 	if len(planMigrations) == 0 {
-		fmt.Fprintf(cmd.w, "%s: all migrations are up\n", subCmd)
+		fmt.Fprintf(appCtx.App.Writer, "%s: all migrations are up\n", subCmd)
 		return nil
 	}
-	fmt.Fprintf(cmd.w, "%s: migrations to apply: %d\n\n", subCmd, len(planMigrations))
+	fmt.Fprintf(appCtx.App.Writer, "%s: migrations to apply: %d\n\n", subCmd, len(planMigrations))
 
 	executed := 0
 	for _, mig := range planMigrations {
-		fmt.Fprintf(cmd.w, "=  %s %v: migrating\n", time.Now().Format(timeFmt), mig.Id)
+		fmt.Fprintf(appCtx.App.Writer, "=  %s %v: migrating\n", time.Now().Format(timeFmt), mig.Id)
 		start := time.Now()
 
-		if cmd.verbose {
-			fmt.Fprintf(cmd.w, "\t%v\n", mig.Up)
+		if appCtx.Bool("verbose") {
+			fmt.Fprintf(appCtx.App.Writer, "\t%v\n", mig.Up)
 		}
 
-		n, err := glsql.MigrateSome(mig.Migration, db, cmd.ignoreUnknown)
+		n, err := glsql.MigrateSome(mig.Migration, db, ignoreUnknown)
 		if err != nil {
 			return fmt.Errorf("%s: fail: %w", time.Now().Format(timeFmt), err)
 		}
 
 		if n > 0 {
-			fmt.Fprintf(cmd.w, "== %s %v: applied (%s)\n", time.Now().Format(timeFmt), mig.Id, time.Since(start))
+			fmt.Fprintf(appCtx.App.Writer, "== %s %v: applied (%s)\n", time.Now().Format(timeFmt), mig.Id, time.Since(start))
 
 			// Additional migrations were run. No harm, but prevents us from tracking their execution duration.
 			if n > 1 {
-				fmt.Fprintf(cmd.w, "warning: %v additional migrations were applied successfully\n", n-1)
+				fmt.Fprintf(appCtx.App.Writer, "warning: %v additional migrations were applied successfully\n", n-1)
 			}
 		} else {
-			fmt.Fprintf(cmd.w, "== %s %v: skipped (%s)\n", time.Now().Format(timeFmt), mig.Id, time.Since(start))
+			fmt.Fprintf(appCtx.App.Writer, "== %s %v: skipped (%s)\n", time.Now().Format(timeFmt), mig.Id, time.Since(start))
 		}
 
 		executed += n
 	}
 
-	fmt.Fprintf(cmd.w, "\n%s: OK (applied %d migrations)\n", subCmd, executed)
+	fmt.Fprintf(appCtx.App.Writer, "\n%s: OK (applied %d migrations)\n", subCmd, executed)
 	return nil
 }
diff --git a/internal/cli/praefect/subcmd_sql_migrate_test.go b/internal/cli/praefect/subcmd_sql_migrate_test.go
index 535c091fa..9fae09ca8 100644
--- a/internal/cli/praefect/subcmd_sql_migrate_test.go
+++ b/internal/cli/praefect/subcmd_sql_migrate_test.go
@@ -2,11 +2,12 @@ package praefect
 
 import (
 	"bytes"
-	"flag"
 	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore/migrations"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testdb"
@@ -15,17 +16,28 @@ import (
 func TestSubCmdSqlMigrate(t *testing.T) {
 	db := testdb.New(t)
 	dbCfg := testdb.GetConfig(t, db.Name)
-	cfg := config.Config{DB: dbCfg}
+	cfg := config.Config{
+		ListenAddr:      "/dev/null",
+		VirtualStorages: []*config.VirtualStorage{{Name: "p", Nodes: []*config.Node{{Storage: "s", Address: "localhost"}}}},
+		DB:              dbCfg,
+	}
+	confPath := writeConfigToFile(t, cfg)
 
 	migrationCt := len(migrations.All())
 
 	for _, tc := range []struct {
 		desc           string
 		up             int
-		verbose        bool
+		args           []string
 		expectedOutput []string
+		expectedErr    error
 	}{
 		{
+			desc:        "unexpected positional arguments",
+			args:        []string{"positonal-arg"},
+			expectedErr: cli.Exit(unexpectedPositionalArgsError{Command: "sql-migrate"}, 1),
+		},
+		{
 			desc:           "All migrations up",
 			up:             migrationCt,
 			expectedOutput: []string{"praefect sql-migrate: all migrations are up"},
@@ -51,9 +63,9 @@ func TestSubCmdSqlMigrate(t *testing.T) {
 			},
 		},
 		{
-			desc:    "Verbose output",
-			up:      0,
-			verbose: true,
+			desc: "Verbose output",
+			up:   0,
+			args: []string{"-verbose"},
 			expectedOutput: []string{
 				fmt.Sprintf("praefect sql-migrate: migrations to apply: %d", migrationCt),
 				"20200109161404_hello_world: migrating",
@@ -67,9 +79,25 @@ func TestSubCmdSqlMigrate(t *testing.T) {
 			testdb.SetMigrations(t, db, cfg, tc.up)
 
 			var stdout bytes.Buffer
-			migrateCmd := sqlMigrateSubcommand{w: &stdout, ignoreUnknown: true, verbose: tc.verbose}
-			assert.NoError(t, migrateCmd.Exec(flag.NewFlagSet("", flag.PanicOnError), cfg))
-
+			var stderr bytes.Buffer
+			app := cli.App{
+				Reader:          bytes.NewReader(nil),
+				Writer:          &stdout,
+				ErrWriter:       &stderr,
+				HideHelpCommand: true,
+				Commands: []*cli.Command{
+					newSQLMigrateCommand(),
+				},
+				Flags: []cli.Flag{
+					&cli.StringFlag{
+						Name:  "config",
+						Value: confPath,
+					},
+				},
+			}
+			err := app.Run(append([]string{progname, sqlMigrateCmdName, "-ignore-unknown"}, tc.args...))
+			require.Equal(t, tc.expectedErr, err)
+			assert.Empty(t, stderr.String())
 			for _, out := range tc.expectedOutput {
 				assert.Contains(t, stdout.String(), out)
 			}