Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Reorganize global CLI flags #2615

Merged
merged 7 commits into from
May 14, 2024
Merged

refactor: Reorganize global CLI flags #2615

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fcafbc1
reorganize cli flags
nasdf May 13, 2024
854812e
make bound flags package variables
nasdf May 13, 2024
6020ee0
fix start flags
nasdf May 13, 2024
48b1f31
ContinueOnError when parsing flags
nasdf May 14, 2024
fe9ea42
change log-no-color flag to no-log-color
nasdf May 14, 2024
29224d6
fix logging config comment
nasdf May 14, 2024
cba497f
Merge branch 'develop' into nasdf/refactor/cli-flags
nasdf May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 37 additions & 34 deletions cli/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package cli

import (
"errors"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -39,29 +40,29 @@ var configPaths = []string{
"keyring.path",
}

// configFlags is a mapping of config keys to cli flags to bind to.
// configFlags is a mapping of cli flag names to config keys to bind.
var configFlags = map[string]string{
"log.level": "log-level",
"log.output": "log-output",
"log.format": "log-format",
"log.stacktrace": "log-stacktrace",
"log.source": "log-source",
"log.overrides": "log-overrides",
"log.nocolor": "log-no-color",
"api.address": "url",
"datastore.maxtxnretries": "max-txn-retries",
"datastore.store": "store",
"datastore.badger.valuelogfilesize": "valuelogfilesize",
"net.peers": "peers",
"net.p2paddresses": "p2paddr",
"net.p2pdisabled": "no-p2p",
"api.allowed-origins": "allowed-origins",
"api.pubkeypath": "pubkeypath",
"api.privkeypath": "privkeypath",
"keyring.namespace": "keyring-namespace",
"keyring.backend": "keyring-backend",
"keyring.path": "keyring-path",
"keyring.disabled": "no-keyring",
"log-level": "log.level",
"log-output": "log.output",
"log-format": "log.format",
"log-stacktrace": "log.stacktrace",
"log-source": "log.source",
"log-overrides": "log.overrides",
"log-no-color": "log.nocolor",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Would be nice to have a consistent usage of no and disable terminology and the no prefix to disable a feature.

For example: To disable key ring and p2p users use: no-keyring and no-p2p respectively, i.e. leading no prefix

But for disabling log, it is log-no-color rather than no-log-color.

Now for config keys we use: keyring.disabled net.p2pdisabled (the disabled term)

But for log we have log.nocolor instead of log.colordisabled

IMO would be nice to have at least user facing flags to be predictable / consistent in terminology.

"url": "api.address",
"max-txn-retries": "datastore.maxtxnretries",
"store": "datastore.store",
"valuelogfilesize": "datastore.badger.valuelogfilesize",
"peers": "net.peers",
"p2paddr": "net.p2paddresses",
"no-p2p": "net.p2pdisabled",
"allowed-origins": "api.allowed-origins",
"pubkeypath": "api.pubkeypath",
"privkeypath": "api.privkeypath",
"keyring-namespace": "keyring.namespace",
"keyring-backend": "keyring.backend",
"keyring-path": "keyring.path",
"no-keyring": "keyring.disabled",
}

// defaultConfig returns a new config with default values.
Expand All @@ -84,11 +85,11 @@ func defaultConfig() *viper.Viper {
}

// createConfig writes the default config file if one does not exist.
func createConfig(rootdir string, flags *pflag.FlagSet) error {
func createConfig(rootdir string) error {
cfg := defaultConfig()
cfg.AddConfigPath(rootdir)

if err := bindConfigFlags(cfg, flags); err != nil {
if err := bindConfigFlags(cfg); err != nil {
return err
}
// make sure rootdir exists
Expand All @@ -106,7 +107,7 @@ func createConfig(rootdir string, flags *pflag.FlagSet) error {
}

// loadConfig returns a new config with values from the config in the given rootdir.
func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) {
func loadConfig(rootdir string) (*viper.Viper, error) {
cfg := defaultConfig()
cfg.AddConfigPath(rootdir)

Expand All @@ -119,7 +120,7 @@ func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) {
return nil, err
}
// bind cli flags to config keys
if err := bindConfigFlags(cfg, flags); err != nil {
if err := bindConfigFlags(cfg); err != nil {
return nil, err
}

Expand All @@ -138,6 +139,7 @@ func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) {
Output: cfg.GetString("log.output"),
EnableStackTrace: cfg.GetBool("log.stacktrace"),
EnableSource: cfg.GetBool("log.source"),
DisableColor: cfg.GetBool("log.nocolor"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😁

Copy link
Contributor

@AndrewSisley AndrewSisley May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The // set default logging config appears incorrect though - this is not setting the default, but instead takes from the CLI params? Suggest either changing the comment or removing it.

})

// set logging config overrides
Expand All @@ -147,12 +149,13 @@ func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) {
}

// bindConfigFlags binds the set of cli flags to config values.
func bindConfigFlags(cfg *viper.Viper, flags *pflag.FlagSet) error {
for key, flag := range configFlags {
err := cfg.BindPFlag(key, flags.Lookup(flag))
if err != nil {
return err
}
}
return nil
func bindConfigFlags(cfg *viper.Viper) 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) {
errs = append(errs, cfg.BindPFlag(configFlags[f.Name], f))
})
return errors.Join(errs...)
}
6 changes: 3 additions & 3 deletions cli/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@ import (

func TestCreateConfig(t *testing.T) {
rootdir := t.TempDir()
err := createConfig(rootdir, NewDefraCommand().PersistentFlags())
err := createConfig(rootdir)
require.NoError(t, err)

// ensure no errors when config already exists
err = createConfig(rootdir, NewDefraCommand().PersistentFlags())
err = createConfig(rootdir)
require.NoError(t, err)

assert.FileExists(t, filepath.Join(rootdir, "config.yaml"))
}

func TestLoadConfigNotExist(t *testing.T) {
rootdir := t.TempDir()
cfg, err := loadConfig(rootdir, NewDefraCommand().PersistentFlags())
cfg, err := loadConfig(rootdir)
require.NoError(t, err)

assert.Equal(t, 5, cfg.GetInt("datastore.maxtxnretries"))
Expand Down
132 changes: 37 additions & 95 deletions cli/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,156 +12,98 @@ package cli

import (
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

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.
// rootFlags is a set of persistent flags that are bound to config values.
var rootFlags = pflag.NewFlagSet("root", pflag.ContinueOnError)

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().String(
func init() {
rootFlags.String(
"rootdir",
"",
"Directory for persistent data (default: $HOME/.defradb)",
)

cmd.PersistentFlags().String(
rootFlags.String(
"log-level",
"info",
"Log level to use. Options are debug, info, error, fatal",
)

cmd.PersistentFlags().String(
rootFlags.String(
"log-output",
"stderr",
"Log output path. Options are stderr or stdout.",
)

cmd.PersistentFlags().String(
rootFlags.String(
"log-format",
"text",
"Log format to use. Options are text or json",
)

cmd.PersistentFlags().Bool(
rootFlags.Bool(
"log-stacktrace",
false,
"Include stacktrace in error and fatal logs",
)

cmd.PersistentFlags().Bool(
rootFlags.Bool(
"log-source",
false,
"Include source location in logs",
)

cmd.PersistentFlags().String(
rootFlags.String(
"log-overrides",
"",
"Logger config overrides. Format <name>,<key>=<val>,...;<name>,...",
)

cmd.PersistentFlags().Bool(
rootFlags.Bool(
"log-no-color",
false,
"Disable colored log output",
)

cmd.PersistentFlags().String(
rootFlags.String(
"url",
"127.0.0.1:9181",
"URL of HTTP endpoint to listen on or connect to",
)

cmd.PersistentFlags().StringArray(
"peers",
[]string{},
"List of peers to connect to",
)

cmd.PersistentFlags().Int(
"max-txn-retries",
5,
"Specify the maximum number of retries per transaction",
)

cmd.PersistentFlags().String(
"store",
"badger",
"Specify the datastore to use (supported: badger, memory)",
)

cmd.PersistentFlags().Int(
"valuelogfilesize",
1<<30,
"Specify the datastore value log file size (in bytes). In memory size will be 2*valuelogfilesize",
)

cmd.PersistentFlags().StringSlice(
"p2paddr",
[]string{"/ip4/127.0.0.1/tcp/9171"},
"Listen addresses for the p2p network (formatted as a libp2p MultiAddr)",
)

cmd.PersistentFlags().Bool(
"no-p2p",
false,
"Disable the peer-to-peer network synchronization system",
)

cmd.PersistentFlags().StringArray(
"allowed-origins",
[]string{},
"List of origins to allow for CORS requests",
)

cmd.PersistentFlags().String(
"pubkeypath",
"",
"Path to the public key for tls",
)

cmd.PersistentFlags().String(
"privkeypath",
"",
"Path to the private key for tls",
)

cmd.PersistentFlags().String(
rootFlags.String(
"keyring-namespace",
"defradb",
"Service name to use when using the system backend",
)

cmd.PersistentFlags().String(
rootFlags.String(
"keyring-backend",
"file",
"Keyring backend to use. Options are file or system",
)

cmd.PersistentFlags().String(
rootFlags.String(
"keyring-path",
"keys",
"Path to store encrypted keys when using the file backend",
)

cmd.PersistentFlags().Bool(
rootFlags.Bool(
"no-keyring",
false,
"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
}
Loading
Loading