[+] refactor config command and sub-commands (#621)

* [+] refactor `config` command and sub-commands
* [*] remove extra checking now done in `ValidateConfig()`
* [+] add `config init` for yaml sources file

Author: pashagolub

internal/cmdopts/cmdconfig.go

@@ -3,7 +3,6 @@ package cmdopts
 import (
 	"context"
 	"errors"
-	"fmt"
 
 	"github.com/cybertec-postgresql/pgwatch/v3/internal/metrics"
 	"github.com/cybertec-postgresql/pgwatch/v3/internal/sources"
@@ -27,91 +26,74 @@ type ConfigInitCommand struct {
 	owner *Options
 }
 
-func (cmd *ConfigInitCommand) Execute([]string) error {
-	if len(cmd.owner.Sources.Sources)+len(cmd.owner.Metrics.Metrics) == 0 {
-		return errors.New("both --sources and --metrics are empty")
+// Execute initializes the configuration.
+func (cmd *ConfigInitCommand) Execute([]string) (err error) {
+	if err = cmd.owner.ValidateConfig(); err != nil {
+		return
 	}
 	if cmd.owner.Metrics.Metrics > "" {
-		cmd.InitMetrics()
+		err = cmd.InitMetrics()
 	}
 	if cmd.owner.Sources.Sources > "" && cmd.owner.Metrics.Metrics != cmd.owner.Sources.Sources {
-		cmd.InitSources()
+		err = errors.Join(err, cmd.InitSources())
 	}
-	cmd.owner.CommandCompleted = true
-	return nil
+	cmd.owner.CompleteCommand(map[bool]int32{
+		true:  ExitCodeOK,
+		false: ExitCodeConfigError,
+	}[err == nil])
+	return
 }
 
-func (cmd *ConfigInitCommand) InitSources() {
+// InitSources initializes the sources configuration.
+func (cmd *ConfigInitCommand) InitSources() (err error) {
 	ctx := context.Background()
-	kind, err := cmd.owner.GetConfigKind(cmd.owner.Sources.Sources)
-	if err != nil {
-		cmd.owner.CompleteCommand(ExitCodeConfigError)
-		return
-	}
-	switch kind {
-	case ConfigPgURL:
-		if err = cmd.owner.InitSourceReader(ctx); err != nil {
-			cmd.owner.CompleteCommand(ExitCodeConfigError)
-			return
-		}
-	case ConfigFile:
-		writer, _ := sources.NewYAMLSourcesReaderWriter(ctx, cmd.owner.Sources.Sources)
-		if err = writer.WriteSources(sources.Sources{sources.Source{}}); err != nil {
-			fmt.Printf("cannot init sources: %s", err)
-			cmd.owner.CompleteCommand(ExitCodeConfigError)
-			return
-		}
+	opts := cmd.owner
+	if opts.IsPgConnStr(opts.Sources.Sources) {
+		return opts.InitSourceReader(ctx)
 	}
-	fmt.Println("Sources initialized")
+	rw, _ := sources.NewYAMLSourcesReaderWriter(ctx, opts.Sources.Sources)
+	return rw.WriteSources(sources.Sources{sources.Source{}})
 }
 
-func (cmd *ConfigInitCommand) InitMetrics() {
+// InitMetrics initializes the metrics configuration.
+func (cmd *ConfigInitCommand) InitMetrics() (err error) {
 	ctx := context.Background()
-	if cmd.owner.IsPgConnStr(cmd.owner.Metrics.Metrics) {
-		if err := cmd.owner.InitMetricReader(ctx); err != nil {
-			cmd.owner.CompleteCommand(ExitCodeConfigError)
-			return
-		}
-	} else {
-		reader, _ := metrics.NewYAMLMetricReaderWriter(ctx, "")
-		writer, _ := metrics.NewYAMLMetricReaderWriter(ctx, cmd.owner.Metrics.Metrics)
-		defMetrics, _ := reader.GetMetrics()
-		if err := writer.WriteMetrics(defMetrics); err != nil {
-			fmt.Printf("cannot init metrics: %s", err)
-			cmd.owner.CompleteCommand(ExitCodeConfigError)
-			return
-		}
+	opts := cmd.owner
+	err = opts.InitMetricReader(ctx)
+	if err != nil || opts.IsPgConnStr(opts.Metrics.Metrics) {
+		return // nothing to do, database initialized automatically
 	}
-	fmt.Println("Metrics initialized")
+	reader, _ := metrics.NewYAMLMetricReaderWriter(ctx, "")
+	defMetrics, _ := reader.GetMetrics()
+	return opts.MetricsReaderWriter.WriteMetrics(defMetrics)
 }
 
 type ConfigUpgradeCommand struct {
 	owner *Options
 }
 
+// Execute upgrades the configuration schema.
 func (cmd *ConfigUpgradeCommand) Execute([]string) (err error) {
-	err = cmd.owner.InitConfigReaders(context.Background())
-	if err != nil {
-		cmd.owner.CompleteCommand(ExitCodeConfigError)
-		fmt.Println(err)
+	opts := cmd.owner
+	if err = opts.ValidateConfig(); err != nil {
 		return
 	}
-	if m, ok := cmd.owner.MetricsReaderWriter.(metrics.Migrator); ok {
-		if err = m.Migrate(); err != nil {
-			cmd.owner.CompleteCommand(ExitCodeUpgradeError)
-			fmt.Printf("cannot upgrade metrics: %s", err)
+	// for now only postgres configuration is upgradable
+	if opts.IsPgConnStr(opts.Metrics.Metrics) && opts.IsPgConnStr(opts.Sources.Sources) {
+		err = opts.InitMetricReader(context.Background())
+		if err != nil {
+			opts.CompleteCommand(ExitCodeConfigError)
+			return
+		}
+		if m, ok := opts.MetricsReaderWriter.(metrics.Migrator); ok {
+			err = m.Migrate()
+			opts.CompleteCommand(map[bool]int32{
+				true:  ExitCodeOK,
+				false: ExitCodeConfigError,
+			}[err == nil])
 			return
 		}
-		cmd.owner.CompleteCommand(ExitCodeOK)
-		fmt.Println("Configuration schema upgraded")
-		return
 	}
-	// if s, ok := cmd.owner.SourcesReaderWriter.(sources.Migrator); ok {
-	// 	if err = cmd.UpgradeConfiguration(s); err != nil {
-	// 		cmd.owner.CompleteCommand(ExitCodeUpgradeError)
-	// 		fmt.Printf("cannot upgrade sources: %s", err)
-	// 		return
-	// 	}
-	// }
-	return errors.New("Configuration storage does not support upgrade")
+	opts.CompleteCommand(ExitCodeConfigError)
+	return errors.New("configuration storage does not support upgrade")
 }

internal/cmdopts/cmdconfig_test.go

@@ -40,18 +40,49 @@ func TestConfigInitCommand_Execute(t *testing.T) {
 		a.True(fi.Size() > 0)
 	})
 
+	t.Run("sources is a proper file name", func(*testing.T) {
+		fname := t.TempDir() + "/sources.yaml"
+		os.Args = []string{0: "config_test", "--sources=" + fname, "config", "init"}
+		_, err := New(io.Discard)
+		a.NoError(err)
+		a.FileExists(fname)
+		fi, err := os.Stat(fname)
+		require.NoError(t, err)
+		a.True(fi.Size() > 0)
+	})
+
 	t.Run("metrics is an invalid file name", func(*testing.T) {
 		os.Args = []string{0: "config_test", "--metrics=/", "config", "init"}
 		opts, err := New(io.Discard)
-		a.NoError(err) // parsing should not fail
+		a.Error(err)
 		a.Equal(ExitCodeConfigError, opts.ExitCode)
 	})
 
 	t.Run("metrics is proper postgres connectin string", func(*testing.T) {
 		os.Args = []string{0: "config_test", "--metrics=postgresql://foo@bar/baz", "config", "init"}
 		opts, err := New(io.Discard)
-		a.NoError(err) // parsing should not fail
+		a.Error(err)
 		a.Equal(ExitCodeConfigError, opts.ExitCode)
 	})
 
 }
+
+func TestConfigUpgradeCommand_Execute(t *testing.T) {
+	a := assert.New(t)
+
+	t.Run("sources and metrics are empty", func(*testing.T) {
+		os.Args = []string{0: "config_test", "config", "upgrade"}
+		_, err := New(io.Discard)
+		a.Error(err)
+	})
+
+	t.Run("metrics is a proper file name but files are not upgradable", func(*testing.T) {
+		fname := t.TempDir() + "/metrics.yaml"
+		os.Args = []string{0: "config_test", "--metrics=" + fname, "config", "upgrade"}
+		c, err := New(io.Discard)
+		a.Error(err)
+		a.True(c.CommandCompleted)
+		a.Equal(ExitCodeConfigError, c.ExitCode)
+	})
+
+}

internal/cmdopts/cmdoptions.go

@@ -61,9 +61,10 @@ func addCommands(parser *flags.Parser, opts *Options) {
 }
 
 // New returns a new instance of Options and immediately executes the subcommand if specified.
-// Errors are returned for parsing only, if the command line arguments are invalid.
-// Subcommands execution errors (if any) do not affect error returned.
-// Subcommands are responsible for setting exit code only.
+// Subcommands are responsible for setting exit code.
+// Function prints help message only if options are incorrect. If subcommand is executed
+// but fails, function outputs the error message only, indicating that some argument
+// values might be incorrect, e.g. wrong file name, lack of privileges, etc.
 func New(writer io.Writer) (cmdOpts *Options, err error) {
 	cmdOpts = new(Options)
 	parser := flags.NewParser(cmdOpts, flags.HelpFlag)
@@ -74,7 +75,7 @@ func New(writer io.Writer) (cmdOpts *Options, err error) {
 		if flagsErr, ok := err.(*flags.Error); ok && flagsErr.Type == flags.ErrHelp {
 			cmdOpts.Help = true
 		}
-		if !flags.WroteHelp(err) {
+		if !flags.WroteHelp(err) && !cmdOpts.CommandCompleted {
 			parser.WriteHelp(writer)
 		}
 		return cmdOpts, err
@@ -85,7 +86,7 @@ func New(writer io.Writer) (cmdOpts *Options, err error) {
 	if len(nonParsedArgs) > 0 { // we don't expect any non-parsed arguments
 		return cmdOpts, fmt.Errorf("unknown argument(s): %v", nonParsedArgs)
 	}
-	err = validateConfig(cmdOpts)
+	err = cmdOpts.ValidateConfig()
 	return
 }
 
@@ -123,30 +124,21 @@ func (c *Options) IsPgConnStr(arg string) bool {
 
 // InitMetricReader creates a new source reader based on the configuration kind from the options.
 func (c *Options) InitMetricReader(ctx context.Context) (err error) {
-	if c.Metrics.Metrics == "" { //if config database is configured, use it for metrics as well
-		if c.IsPgConnStr(c.Sources.Sources) {
-			c.Metrics.Metrics = c.Sources.Sources
-		} else { // otherwise use built-in metrics
-			c.MetricsReaderWriter, err = metrics.NewYAMLMetricReaderWriter(ctx, "")
-			return err
-		}
+	if c.Metrics.Metrics == "" { // use built-in metrics
+		c.MetricsReaderWriter, err = metrics.NewYAMLMetricReaderWriter(ctx, "")
+		return
 	}
 	if c.IsPgConnStr(c.Metrics.Metrics) {
 		c.MetricsReaderWriter, err = metrics.NewPostgresMetricReaderWriter(ctx, c.Metrics.Metrics)
 	} else {
 		c.MetricsReaderWriter, err = metrics.NewYAMLMetricReaderWriter(ctx, c.Metrics.Metrics)
 	}
-	return err
+	return
 }
 
 // InitSourceReader creates a new source reader based on the configuration kind from the options.
 func (c *Options) InitSourceReader(ctx context.Context) (err error) {
 	var configKind Kind
-	if c.Sources.Sources == "" { //if config database is configured, use it for sources as well
-		if c.IsPgConnStr(c.Metrics.Metrics) {
-			c.Sources.Sources = c.Metrics.Metrics
-		}
-	}
 	if configKind, err = c.GetConfigKind(c.Sources.Sources); err != nil {
 		return
 	}
@@ -156,7 +148,7 @@ func (c *Options) InitSourceReader(ctx context.Context) (err error) {
 	default:
 		c.SourcesReaderWriter, err = sources.NewYAMLSourcesReaderWriter(ctx, c.Sources.Sources)
 	}
-	return err
+	return
 }
 
 // InitConfigReaders creates the configuration readers based on the configuration kind from the options.
@@ -178,10 +170,19 @@ func (c *Options) NeedsSchemaUpgrade() (upgrade bool, err error) {
 	return
 }
 
-func validateConfig(c *Options) error {
+// ValidateConfig checks if the configuration is valid.
+// Configuration database can be specified for one of the --sources or --metrics.
+// If one is specified, the other one is set to the same value.
+func (c *Options) ValidateConfig() error {
 	if len(c.Sources.Sources)+len(c.Metrics.Metrics) == 0 {
 		return errors.New("both --sources and --metrics are empty")
 	}
+	switch { // if specified configuration database, use it for both sources and metrics
+	case c.Sources.Sources == "" && c.IsPgConnStr(c.Metrics.Metrics):
+		c.Sources.Sources = c.Metrics.Metrics
+	case c.Metrics.Metrics == "" && c.IsPgConnStr(c.Sources.Sources):
+		c.Metrics.Metrics = c.Sources.Sources
+	}
 	if c.Sources.Refresh <= 1 {
 		return errors.New("--servers-refresh-loop-seconds must be greater than 1")
 	}