fix: Race condition when testing CLI (#2713)

## Relevant issue(s)

Resolves #2712 

## Description

This PR fixes a race condition found in #2700 

## Tasks

- [x] I made sure the code is well commented, particularly
hard-to-understand areas.
- [x] I made sure the repository-held documentation is changed
accordingly.
- [x] I made sure the pull request title adheres to the conventional
commit style (the subset used in the project can be found in
[tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)).
- [x] I made sure to discuss its limitations such as threats to
validity, vulnerability to mistake and misuse, robustness to
invalidation of assumptions, resource requirements, ...

## How has this been tested?

`make test`

Specify the platform(s) on which this was tested:
- MacOS

cli/config.go

@@ -65,6 +65,32 @@ var configFlags = map[string]string{
 	"no-keyring":        "keyring.disabled",
 }
 
+// configDefaults contains default values for config entries.
+var configDefaults = map[string]any{
+	"api.address":                       "127.0.0.1:9181",
+	"api.allowed-origins":               []string{},
+	"datastore.badger.path":             "data",
+	"datastore.maxtxnretries":           5,
+	"datastore.store":                   "badger",
+	"datastore.badger.valuelogfilesize": 1 << 30,
+	"net.p2pdisabled":                   false,
+	"net.p2paddresses":                  []string{"/ip4/127.0.0.1/tcp/9171"},
+	"net.peers":                         []string{},
+	"net.pubSubEnabled":                 true,
+	"net.relay":                         false,
+	"keyring.backend":                   "file",
+	"keyring.disabled":                  false,
+	"keyring.namespace":                 "defradb",
+	"keyring.path":                      "keys",
+	"log.caller":                        false,
+	"log.colordisabled":                 false,
+	"log.format":                        "text",
+	"log.level":                         "info",
+	"log.output":                        "stderr",
+	"log.source":                        false,
+	"log.stacktrace":                    false,
+}
+
 // defaultConfig returns a new config with default values.
 func defaultConfig() *viper.Viper {
 	cfg := viper.New()
@@ -76,20 +102,18 @@ func defaultConfig() *viper.Viper {
 	cfg.SetConfigName("config")
 	cfg.SetConfigType("yaml")
 
-	cfg.SetDefault("datastore.badger.path", "data")
-	cfg.SetDefault("net.pubSubEnabled", true)
-	cfg.SetDefault("net.relay", false)
-	cfg.SetDefault("log.caller", false)
-
+	for key, val := range configDefaults {
+		cfg.SetDefault(key, val)
+	}
 	return cfg
 }
 
 // createConfig writes the default config file if one does not exist.
-func createConfig(rootdir string) error {
+func createConfig(rootdir string, flags *pflag.FlagSet) error {
 	cfg := defaultConfig()
 	cfg.AddConfigPath(rootdir)
 
-	if err := bindConfigFlags(cfg); err != nil {
+	if err := bindConfigFlags(cfg, flags); err != nil {
 		return err
 	}
 	// make sure rootdir exists
@@ -107,7 +131,7 @@ func createConfig(rootdir string) error {
 }
 
 // loadConfig returns a new config with values from the config in the given rootdir.
-func loadConfig(rootdir string) (*viper.Viper, error) {
+func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) {
 	cfg := defaultConfig()
 	cfg.AddConfigPath(rootdir)
 
@@ -120,7 +144,7 @@ func loadConfig(rootdir string) (*viper.Viper, error) {
 		return nil, err
 	}
 	// bind cli flags to config keys
-	if err := bindConfigFlags(cfg); err != nil {
+	if err := bindConfigFlags(cfg, flags); err != nil {
 		return nil, err
 	}
 
@@ -149,12 +173,9 @@ func loadConfig(rootdir string) (*viper.Viper, error) {
 }
 
 // bindConfigFlags binds the set of cli flags to config values.
-func bindConfigFlags(cfg *viper.Viper) error {
+func bindConfigFlags(cfg *viper.Viper, flags *pflag.FlagSet) error {
 	var errs []error
-	rootFlags.VisitAll(func(f *pflag.Flag) {
-		errs = append(errs, cfg.BindPFlag(configFlags[f.Name], f))
-	})
-	startFlags.VisitAll(func(f *pflag.Flag) {
+	flags.VisitAll(func(f *pflag.Flag) {
 		errs = append(errs, cfg.BindPFlag(configFlags[f.Name], f))
 	})
 	return errors.Join(errs...)

cli/config_test.go

@@ -14,25 +14,30 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestCreateConfig(t *testing.T) {
 	rootdir := t.TempDir()
-	err := createConfig(rootdir)
+	flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+
+	err := createConfig(rootdir, flags)
 	require.NoError(t, err)
 
 	// ensure no errors when config already exists
-	err = createConfig(rootdir)
+	err = createConfig(rootdir, flags)
 	require.NoError(t, err)
 
 	assert.FileExists(t, filepath.Join(rootdir, "config.yaml"))
 }
 
 func TestLoadConfigNotExist(t *testing.T) {
 	rootdir := t.TempDir()
-	cfg, err := loadConfig(rootdir)
+	flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+
+	cfg, err := loadConfig(rootdir, flags)
 	require.NoError(t, err)
 
 	assert.Equal(t, 5, cfg.GetInt("datastore.maxtxnretries"))

cli/root.go

@@ -12,98 +12,90 @@ package cli
 
 import (
 	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
 )
 
-// rootFlags is a set of persistent flags that are bound to config values.
-var rootFlags = pflag.NewFlagSet("root", pflag.ContinueOnError)
+func MakeRootCommand() *cobra.Command {
+	var cmd = &cobra.Command{
+		SilenceUsage: true,
+		Use:          "defradb",
+		Short:        "DefraDB Edge Database",
+		Long: `DefraDB is the edge database to power the user-centric future.
 
-func init() {
-	rootFlags.String(
+Start a DefraDB node, interact with a local or remote node, and much more.
+`,
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
+			if err := setContextRootDir(cmd); err != nil {
+				return err
+			}
+			return setContextConfig(cmd)
+		},
+	}
+	// set default flag values from config
+	cfg := defaultConfig()
+	cmd.PersistentFlags().String(
 		"rootdir",
 		"",
 		"Directory for persistent data (default: $HOME/.defradb)",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"log-level",
-		"info",
+		cfg.GetString(configFlags["log-level"]),
 		"Log level to use. Options are debug, info, error, fatal",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"log-output",
-		"stderr",
+		cfg.GetString(configFlags["log-output"]),
 		"Log output path. Options are stderr or stdout.",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"log-format",
-		"text",
+		cfg.GetString(configFlags["log-format"]),
 		"Log format to use. Options are text or json",
 	)
-	rootFlags.Bool(
+	cmd.PersistentFlags().Bool(
 		"log-stacktrace",
-		false,
+		cfg.GetBool(configFlags["log-stacktrace"]),
 		"Include stacktrace in error and fatal logs",
 	)
-	rootFlags.Bool(
+	cmd.PersistentFlags().Bool(
 		"log-source",
-		false,
+		cfg.GetBool(configFlags["log-source"]),
 		"Include source location in logs",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"log-overrides",
-		"",
+		cfg.GetString(configFlags["log-overrides"]),
 		"Logger config overrides. Format <name>,<key>=<val>,...;<name>,...",
 	)
-	rootFlags.Bool(
+	cmd.PersistentFlags().Bool(
 		"no-log-color",
-		false,
+		cfg.GetBool(configFlags["no-log-color"]),
 		"Disable colored log output",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"url",
-		"127.0.0.1:9181",
+		cfg.GetString(configFlags["url"]),
 		"URL of HTTP endpoint to listen on or connect to",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"keyring-namespace",
-		"defradb",
+		cfg.GetString(configFlags["keyring-namespace"]),
 		"Service name to use when using the system backend",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"keyring-backend",
-		"file",
+		cfg.GetString(configFlags["keyring-backend"]),
 		"Keyring backend to use. Options are file or system",
 	)
-	rootFlags.String(
+	cmd.PersistentFlags().String(
 		"keyring-path",
-		"keys",
+		cfg.GetString(configFlags["keyring-path"]),
 		"Path to store encrypted keys when using the file backend",
 	)
-	rootFlags.Bool(
+	cmd.PersistentFlags().Bool(
 		"no-keyring",
-		false,
+		cfg.GetBool(configFlags["no-keyring"]),
 		"Disable the keyring and generate ephemeral keys",
 	)
-}
-
-func MakeRootCommand() *cobra.Command {
-	var cmd = &cobra.Command{
-		SilenceUsage: true,
-		Use:          "defradb",
-		Short:        "DefraDB Edge Database",
-		Long: `DefraDB is the edge database to power the user-centric future.
-
-Start a DefraDB node, interact with a local or remote node, and much more.
-`,
-		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
-			if err := setContextRootDir(cmd); err != nil {
-				return err
-			}
-			return setContextConfig(cmd)
-		},
-	}
-
-	cmd.PersistentFlags().AddFlagSet(rootFlags)
-
 	return cmd
 }

cli/start.go

@@ -18,7 +18,6 @@ import (
 
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
 
 	"github.com/sourcenetwork/defradb/errors"
 	"github.com/sourcenetwork/defradb/http"
@@ -29,57 +28,6 @@ import (
 	"github.com/sourcenetwork/defradb/node"
 )
 
-// startFlags is a set of persistent flags that are bound to config values.
-var startFlags = pflag.NewFlagSet("start", pflag.ContinueOnError)
-
-func init() {
-	startFlags.StringArray(
-		"peers",
-		[]string{},
-		"List of peers to connect to",
-	)
-	startFlags.Int(
-		"max-txn-retries",
-		5,
-		"Specify the maximum number of retries per transaction",
-	)
-	startFlags.String(
-		"store",
-		"badger",
-		"Specify the datastore to use (supported: badger, memory)",
-	)
-	startFlags.Int(
-		"valuelogfilesize",
-		1<<30,
-		"Specify the datastore value log file size (in bytes). In memory size will be 2*valuelogfilesize",
-	)
-	startFlags.StringSlice(
-		"p2paddr",
-		[]string{"/ip4/127.0.0.1/tcp/9171"},
-		"Listen addresses for the p2p network (formatted as a libp2p MultiAddr)",
-	)
-	startFlags.Bool(
-		"no-p2p",
-		false,
-		"Disable the peer-to-peer network synchronization system",
-	)
-	startFlags.StringArray(
-		"allowed-origins",
-		[]string{},
-		"List of origins to allow for CORS requests",
-	)
-	startFlags.String(
-		"pubkeypath",
-		"",
-		"Path to the public key for tls",
-	)
-	startFlags.String(
-		"privkeypath",
-		"",
-		"Path to the private key for tls",
-	)
-}
-
 func MakeStartCommand() *cobra.Command {
 	var cmd = &cobra.Command{
 		Use:   "start",
@@ -91,7 +39,7 @@ func MakeStartCommand() *cobra.Command {
 				return err
 			}
 			rootdir := mustGetContextRootDir(cmd)
-			if err := createConfig(rootdir); err != nil {
+			if err := createConfig(rootdir, cmd.Flags()); err != nil {
 				return err
 			}
 			return setContextConfig(cmd)
@@ -186,8 +134,52 @@ func MakeStartCommand() *cobra.Command {
 			return nil
 		},
 	}
-
-	cmd.PersistentFlags().AddFlagSet(startFlags)
-
+	// set default flag values from config
+	cfg := defaultConfig()
+	cmd.PersistentFlags().StringArray(
+		"peers",
+		cfg.GetStringSlice(configFlags["peers"]),
+		"List of peers to connect to",
+	)
+	cmd.PersistentFlags().Int(
+		"max-txn-retries",
+		cfg.GetInt(configFlags["max-txn-retries"]),
+		"Specify the maximum number of retries per transaction",
+	)
+	cmd.PersistentFlags().String(
+		"store",
+		cfg.GetString(configFlags["store"]),
+		"Specify the datastore to use (supported: badger, memory)",
+	)
+	cmd.PersistentFlags().Int(
+		"valuelogfilesize",
+		cfg.GetInt(configFlags["valuelogfilesize"]),
+		"Specify the datastore value log file size (in bytes). In memory size will be 2*valuelogfilesize",
+	)
+	cmd.PersistentFlags().StringSlice(
+		"p2paddr",
+		cfg.GetStringSlice(configFlags["p2paddr"]),
+		"Listen addresses for the p2p network (formatted as a libp2p MultiAddr)",
+	)
+	cmd.PersistentFlags().Bool(
+		"no-p2p",
+		cfg.GetBool(configFlags["no-p2p"]),
+		"Disable the peer-to-peer network synchronization system",
+	)
+	cmd.PersistentFlags().StringArray(
+		"allowed-origins",
+		cfg.GetStringSlice(configFlags["allowed-origins"]),
+		"List of origins to allow for CORS requests",
+	)
+	cmd.PersistentFlags().String(
+		"pubkeypath",
+		cfg.GetString(configFlags["pubkeypath"]),
+		"Path to the public key for tls",
+	)
+	cmd.PersistentFlags().String(
+		"privkeypath",
+		cfg.GetString(configFlags["privkeypath"]),
+		"Path to the private key for tls",
+	)
 	return cmd
 }