diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-04-06 12:38:01 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-04-08 15:45:01 +0300 |
commit | 3016db2f28ed9d33dd09acf8a2f20922ed7d4016 (patch) | |
tree | a9c5acc696323d728480e2204fc61794fc965d0b | |
parent | 0887bf430d11fdf8e9b54070e85d5dd2e7cce6ea (diff) |
praefect: Move subcommands into a map
Subcommands of the "praefect" executable are currently implemented kind
of loosely without much format. This has several downsides:
- Some commands generate usage information, some don't.
- Some commands will execute even if passed "-h", which is totally
unexpected for the user.
- Some commands simply ignore any parameters being passed to them
instead of generating an error.
- We can't print a list of all available commands without
hand-coding it.
Let's rearrange them into a map of `subcmd` interfaces, which allow us
to unify handling of those subcommands.
-rw-r--r-- | cmd/praefect/subcmd.go | 89 | ||||
-rw-r--r-- | cmd/praefect/subcmd_pingnodes.go | 15 | ||||
-rw-r--r-- | cmd/praefect/subcmd_reconcile.go | 37 | ||||
-rw-r--r-- | cmd/praefect/subcmd_sqldown.go | 58 |
4 files changed, 112 insertions, 87 deletions
diff --git a/cmd/praefect/subcmd.go b/cmd/praefect/subcmd.go index a29882a90..4ebba9ad7 100644 --- a/cmd/praefect/subcmd.go +++ b/cmd/praefect/subcmd.go @@ -3,6 +3,7 @@ package main import ( "context" "database/sql" + "flag" "fmt" "os" "os/signal" @@ -16,6 +17,21 @@ import ( "google.golang.org/grpc" ) +type subcmd interface { + FlagSet() *flag.FlagSet + Exec(flags *flag.FlagSet, config config.Config) error +} + +var ( + subcommands = map[string]subcmd{ + "sql-ping": &sqlPingSubcommand{}, + "sql-migrate": &sqlMigrateSubcommand{}, + "dial-nodes": &dialNodesSubcommand{}, + "reconcile": &reconcileSubcommand{}, + "sql-migrate-down": &sqlMigrateDownSubcommand{}, + } +) + const invocationPrefix = progname + " -config CONFIG_TOML" // subCommand returns an exit code, to be fed into os.Exit. @@ -28,65 +44,78 @@ func subCommand(conf config.Config, arg0 string, argRest []string) int { os.Exit(130) // indicates program was interrupted }() - switch arg0 { - case "sql-ping": - return sqlPing(conf) - case "sql-migrate": - return sqlMigrate(conf) - case subCmdSQLMigrateDown: - return sqlMigrateDown(conf, argRest) - case "dial-nodes": - return dialNodes(conf) - case "reconcile": - return reconcile(conf, argRest) - default: + subcmd, ok := subcommands[arg0] + if !ok { printfErr("%s: unknown subcommand: %q\n", progname, arg0) return 1 } + + flags := subcmd.FlagSet() + + if err := flags.Parse(argRest); err != nil { + printfErr("%s\n", err) + return 1 + } + + if err := subcmd.Exec(flags, conf); err != nil { + printfErr("%s\n", err) + return 1 + } + + return 0 +} + +type sqlPingSubcommand struct{} + +func (s *sqlPingSubcommand) FlagSet() *flag.FlagSet { + return flag.NewFlagSet("sql-ping", flag.ExitOnError) } -func sqlPing(conf config.Config) int { +func (s *sqlPingSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error { const subCmd = progname + " sql-ping" - db, clean, code := openDB(conf.DB) - if code != 0 { - return code + db, clean, err := openDB(conf.DB) + if err != nil { + return err } defer clean() if err := datastore.CheckPostgresVersion(db); err != nil { - printfErr("%s: fail: %v\n", subCmd, err) - return 1 + return fmt.Errorf("%s: fail: %v\n", subCmd, err) } fmt.Printf("%s: OK\n", subCmd) - return 0 + return nil } -func sqlMigrate(conf config.Config) int { +type sqlMigrateSubcommand struct{} + +func (s *sqlMigrateSubcommand) FlagSet() *flag.FlagSet { + return flag.NewFlagSet("sql-migrate", flag.ExitOnError) +} + +func (s *sqlMigrateSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error { const subCmd = progname + " sql-migrate" - db, clean, code := openDB(conf.DB) - if code != 0 { - return code + db, clean, err := openDB(conf.DB) + if err != nil { + return err } defer clean() n, err := glsql.Migrate(db) if err != nil { - printfErr("%s: fail: %v\n", subCmd, err) - return 1 + return fmt.Errorf("%s: fail: %v\n", subCmd, err) } fmt.Printf("%s: OK (applied %d migrations)\n", subCmd, n) - return 0 + return nil } -func openDB(conf config.DB) (*sql.DB, func(), int) { +func openDB(conf config.DB) (*sql.DB, func(), error) { db, err := glsql.OpenDB(conf) if err != nil { - printfErr("sql open: %v\n", err) - return nil, nil, 1 + return nil, nil, fmt.Errorf("sql open: %v\n", err) } clean := func() { @@ -95,7 +124,7 @@ func openDB(conf config.DB) (*sql.DB, func(), int) { } } - return db, clean, 0 + return db, clean, nil } func printfErr(format string, a ...interface{}) (int, error) { diff --git a/cmd/praefect/subcmd_pingnodes.go b/cmd/praefect/subcmd_pingnodes.go index b3bf24fec..742f8eb12 100644 --- a/cmd/praefect/subcmd_pingnodes.go +++ b/cmd/praefect/subcmd_pingnodes.go @@ -3,6 +3,7 @@ package main import ( "context" "errors" + "flag" "fmt" "log" "sync" @@ -45,7 +46,13 @@ func flattenNodes(conf config.Config) map[string]*nodePing { return nodeByAddress } -func dialNodes(conf config.Config) int { +type dialNodesSubcommand struct{} + +func (s *dialNodesSubcommand) FlagSet() *flag.FlagSet { + return flag.NewFlagSet("dial-nodes", flag.ExitOnError) +} + +func (s *dialNodesSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error { nodes := flattenNodes(conf) var wg sync.WaitGroup @@ -58,14 +65,14 @@ func dialNodes(conf config.Config) int { } wg.Wait() - exitCode := 0 + var err error for _, n := range nodes { if n.err != nil { - exitCode = 1 + err = n.err } } - return exitCode + return err } func (npr *nodePing) dial() (*grpc.ClientConn, error) { diff --git a/cmd/praefect/subcmd_reconcile.go b/cmd/praefect/subcmd_reconcile.go index 21f3ed7fc..bb2b12b6c 100644 --- a/cmd/praefect/subcmd_reconcile.go +++ b/cmd/praefect/subcmd_reconcile.go @@ -19,32 +19,33 @@ type nodeReconciler struct { referenceStorage string } -func reconcile(conf config.Config, subCmdArgs []string) int { - var ( - fs = flag.NewFlagSet("reconcile", flag.ExitOnError) - vs = fs.String("virtual", "", "virtual storage for target storage") - t = fs.String("target", "", "target storage to reconcile") - r = fs.String("reference", "", "reference storage to reconcile (optional)") - ) - - if err := fs.Parse(subCmdArgs); err != nil { - log.Printf("unable to parse args %v: %s", subCmdArgs, err) - return 1 - } +type reconcileSubcommand struct { + virtual string + target string + reference string +} +func (s *reconcileSubcommand) FlagSet() *flag.FlagSet { + fs := flag.NewFlagSet("reconcile", flag.ExitOnError) + fs.StringVar(&s.virtual, "virtual", "", "virtual storage for target storage") + fs.StringVar(&s.target, "target", "", "target storage to reconcile") + fs.StringVar(&s.reference, "reference", "", "reference storage to reconcile (optional)") + return fs +} + +func (s *reconcileSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error { nr := nodeReconciler{ conf: conf, - virtualStorage: *vs, - targetStorage: *t, - referenceStorage: *r, + virtualStorage: s.virtual, + targetStorage: s.target, + referenceStorage: s.reference, } if err := nr.reconcile(); err != nil { - log.Print("unable to reconcile: ", err) - return 1 + return fmt.Errorf("unable to reconcile: %s", err) } - return 0 + return nil } func (nr nodeReconciler) reconcile() error { diff --git a/cmd/praefect/subcmd_sqldown.go b/cmd/praefect/subcmd_sqldown.go index f34395d6c..6e0d41275 100644 --- a/cmd/praefect/subcmd_sqldown.go +++ b/cmd/praefect/subcmd_sqldown.go @@ -1,6 +1,7 @@ package main import ( + "errors" "flag" "fmt" "strconv" @@ -9,43 +10,30 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/praefect/datastore" ) -const subCmdSQLMigrateDown = "sql-migrate-down" - -func sqlMigrateDown(conf config.Config, args []string) int { - cmd := &sqlMigrateDownCmd{Config: conf} - return cmd.Run(args) +type sqlMigrateDownSubcommand struct { + force bool } -type sqlMigrateDownCmd struct{ config.Config } - -func (*sqlMigrateDownCmd) prefix() string { return progname + " " + subCmdSQLMigrateDown } - -func (*sqlMigrateDownCmd) invocation() string { return invocationPrefix + " " + subCmdSQLMigrateDown } - -func (smd *sqlMigrateDownCmd) Run(args []string) int { - flagset := flag.NewFlagSet(smd.prefix(), flag.ExitOnError) - flagset.Usage = func() { - printfErr("usage: %s [-f] MAX_MIGRATIONS\n", smd.invocation()) - } - force := flagset.Bool("f", false, "apply down-migrations (default is dry run)") +func (*sqlMigrateDownSubcommand) prefix() string { return progname + " sql-migrate-down" } - _ = flagset.Parse(args) // No error check because flagset is set to ExitOnError +func (*sqlMigrateDownSubcommand) invocation() string { return invocationPrefix + " sql-migrate-down" } - if flagset.NArg() != 1 { - flagset.Usage() - return 1 +func (s *sqlMigrateDownSubcommand) FlagSet() *flag.FlagSet { + flags := flag.NewFlagSet(s.prefix(), flag.ExitOnError) + flags.Usage = func() { + printfErr("usage: %s [-f] MAX_MIGRATIONS\n", s.invocation()) } + flags.BoolVar(&s.force, "f", false, "apply down-migrations (default is dry run)") + return flags +} - if err := smd.run(*force, flagset.Arg(0)); err != nil { - printfErr("%s: fail: %v\n", smd.prefix(), err) - return 1 +func (s *sqlMigrateDownSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error { + if flags.NArg() != 1 { + flags.Usage() + return errors.New("invalid usage") } - return 0 -} - -func (smd *sqlMigrateDownCmd) run(force bool, maxString string) error { - maxMigrations, err := strconv.Atoi(maxString) + maxMigrations, err := strconv.Atoi(flags.Arg(0)) if err != nil { return err } @@ -54,26 +42,26 @@ func (smd *sqlMigrateDownCmd) run(force bool, maxString string) error { return fmt.Errorf("number of migrations to roll back must be 1 or more") } - if force { - n, err := datastore.MigrateDown(smd.Config, maxMigrations) + if s.force { + n, err := datastore.MigrateDown(conf, maxMigrations) if err != nil { return err } - fmt.Printf("%s: OK (applied %d \"down\" migrations)\n", smd.prefix(), n) + fmt.Printf("%s: OK (applied %d \"down\" migrations)\n", s.prefix(), n) return nil } - planned, err := datastore.MigrateDownPlan(smd.Config, maxMigrations) + planned, err := datastore.MigrateDownPlan(conf, maxMigrations) if err != nil { return err } - fmt.Printf("%s: DRY RUN -- would roll back:\n\n", smd.prefix()) + fmt.Printf("%s: DRY RUN -- would roll back:\n\n", s.prefix()) for _, id := range planned { fmt.Printf("- %s\n", id) } - fmt.Printf("\nTo apply these migrations run: %s -f %d\n", smd.invocation(), maxMigrations) + fmt.Printf("\nTo apply these migrations run: %s -f %d\n", s.invocation(), maxMigrations) return nil } |