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.

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

fix: Resolve handful of CLI issues #1318

Merged
merged 5 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
10 changes: 7 additions & 3 deletions cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,18 @@ import (

const badgerDatastoreName = "badger"

// List of cobra errors indicating an error occurred in the way the command was invoked.
// They are subject to change with new versions of cobra.
// Errors with how the command is invoked by user
var usageErrors = []string{
// cobra errors - subject to change with new versions of cobra
"flag needs an argument",
"invalid syntax",
"unknown flag",
"unknown shorthand flag",
"missing argument", // custom to defradb
"unknown command",
// custom defradb errors
errMissingArg,
errMissingArgs,
errTooManyArgs,
}

var log = logging.MustNewLogger("defra.cli")
Expand Down
4 changes: 2 additions & 2 deletions cli/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ package cli
import "github.com/sourcenetwork/defradb/errors"

const (
errMissingArg string = "missing arguement"
errMissingArgs string = "missing arguements"
errMissingArg string = "missing argument"
errMissingArgs string = "missing arguments"
errTooManyArgs string = "too many arguments"
errEmptyStdin string = "empty stdin"
errEmptyFile string = "empty file"
Expand Down
16 changes: 6 additions & 10 deletions cli/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ It covers three possible situations:
- root dir exists and contains a config file
*/
var initCmd = &cobra.Command{
Use: "init [rootdir]",
Use: "init",
Short: "Initialize DefraDB's root directory and configuration file",
Long: "Initialize a directory for configuration and data at the given path.",
// Load a default configuration, considering env. variables and CLI flags.
Expand All @@ -41,15 +41,6 @@ var initCmd = &cobra.Command{
return nil
},
RunE: func(cmd *cobra.Command, args []string) error {
cfg.Rootdir = config.DefaultRootDir()
if len(args) == 1 {
cfg.Rootdir = args[0]
} else if len(args) > 1 {
if err := cmd.Usage(); err != nil {
return err
}
return errors.New("init command requires one rootdir argument, or no argument")
}
if config.FolderExists(cfg.Rootdir) {
if cfg.ConfigFileExists() {
if reinitialize {
Expand Down Expand Up @@ -89,4 +80,9 @@ func init() {
&reinitialize, "reinitialize", false,
"Reinitialize the configuration file",
)

initCmd.Flags().StringVar(
&cfg.Rootdir, "rootdir", config.DefaultRootDir(),
"Directory for data and configuration to use",
)
}
10 changes: 3 additions & 7 deletions cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@ import (

"github.com/spf13/cobra"

"github.com/sourcenetwork/defradb/config"
"github.com/sourcenetwork/defradb/errors"
)

var rootDirParam string

var rootCmd = &cobra.Command{
Use: "defradb",
Short: "DefraDB Edge Database",
Expand All @@ -35,9 +34,6 @@ See https://docs.source.network/BSL.txt for more information.
// Loads the rootDir containing the configuration file, otherwise warn about it and load a default configuration.
// This allows some subcommands (`init`, `start`) to override the PreRun to create a rootDir by default.
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
if rootDirParam != "" {
cfg.Rootdir = rootDirParam
}
if cfg.ConfigFileExists() {
if err := cfg.LoadWithRootdir(true); err != nil {
return errors.Wrap("failed to load config", err)
Expand All @@ -55,8 +51,8 @@ See https://docs.source.network/BSL.txt for more information.

func init() {
rootCmd.PersistentFlags().StringVar(
&rootDirParam, "rootdir", "",
"Directory for data and configuration to use (default \"$HOME/.defradb\")",
&cfg.Rootdir, "rootdir", config.DefaultRootDir(),
"Directory for data and configuration to use",
)

rootCmd.PersistentFlags().String(
Expand Down
12 changes: 2 additions & 10 deletions cli/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,10 @@ var startCmd = &cobra.Command{
Long: "Start a new instance of DefraDB node.",
// Load the root config if it exists, otherwise create it.
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
if rootDirParam != "" {
cfg.Rootdir = rootDirParam
}
if cfg.ConfigFileExists() {
if err := cfg.LoadWithRootdir(true); err != nil {
return errors.Wrap("failed to load config", err)
}
log.FeedbackInfo(cmd.Context(), fmt.Sprintf("Configuration loaded from DefraDB directory %v", cfg.Rootdir))
} else {
if err := cfg.LoadWithRootdir(false); err != nil {
return errors.Wrap("failed to load config", err)
Expand All @@ -65,10 +61,10 @@ var startCmd = &cobra.Command{
if err := cfg.CreateRootDirAndConfigFile(); err != nil {
return err
}

}
}
log.FeedbackInfo(cmd.Context(), fmt.Sprintf("Configuration loaded from DefraDB directory %v", cfg.Rootdir))

return nil
},
RunE: func(cmd *cobra.Command, args []string) error {
Expand Down Expand Up @@ -219,11 +215,7 @@ func start(ctx context.Context) (*defraInstance, error) {

var err error
if cfg.Datastore.Store == badgerDatastoreName {
log.FeedbackInfo(
ctx,
"Opening badger store",
logging.NewKV("Path", cfg.Datastore.Badger.Path),
)
log.FeedbackInfo(ctx, "Opening badger store", logging.NewKV("Path", cfg.Datastore.Badger.Path))
rootstore, err = badgerds.NewDatastore(
cfg.Datastore.Badger.Path,
cfg.Datastore.Badger.Options,
Expand Down
21 changes: 17 additions & 4 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ func DefaultConfig() *Config {
// Use on a Config struct already loaded with default values from DefaultConfig().
// To be executed once at the beginning of the program.
func (cfg *Config) LoadWithRootdir(withRootdir bool) error {
var err error

// Use default logging configuration here, so that
// we can log errors in a consistent way even in the case of early failure.
defaultLogCfg := defaultLogConfig()
Expand All @@ -112,6 +114,12 @@ func (cfg *Config) LoadWithRootdir(withRootdir bool) error {
return err
}

// using absolute rootdir for robustness.
cfg.Rootdir, err = filepath.Abs(cfg.Rootdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Have you tested that this all works on database restart? I.e. to make sure #1293 isnt re-introduced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirm that it works on database restart

if err != nil {
return err
}

if withRootdir {
cfg.v.AddConfigPath(cfg.Rootdir)
if err := cfg.v.ReadInConfig(); err != nil {
Expand Down Expand Up @@ -180,10 +188,16 @@ func (cfg *Config) validate() error {
}

func (cfg *Config) paramsPreprocessing() error {
// We prefer using absolute paths.
// We prefer using absolute paths, relative to the rootdir.
if !filepath.IsAbs(cfg.v.GetString("datastore.badger.path")) {
cfg.v.Set("datastore.badger.path", filepath.Join(cfg.Rootdir, cfg.v.GetString("datastore.badger.path")))
}
if !filepath.IsAbs(cfg.v.GetString("api.privkeypath")) {
cfg.v.Set("api.privkeypath", filepath.Join(cfg.Rootdir, cfg.v.GetString("api.privkeypath")))
}
if !filepath.IsAbs(cfg.v.GetString("api.pubkeypath")) {
cfg.v.Set("api.pubkeypath", filepath.Join(cfg.Rootdir, cfg.v.GetString("api.pubkeypath")))
}

// log.logger configuration as a string
logloggerAsStringSlice := cfg.v.GetStringSlice("log.logger")
Expand Down Expand Up @@ -269,12 +283,11 @@ type APIConfig struct {
}

func defaultAPIConfig() *APIConfig {
rootDir := DefaultRootDir()
return &APIConfig{
Address: "localhost:9181",
TLS: false,
PubKeyPath: filepath.Join(rootDir, "certs/server.key"),
PrivKeyPath: filepath.Join(rootDir, "certs/server.crt"),
PubKeyPath: "certs/server.key",
PrivKeyPath: "certs/server.crt",
Email: DefaultAPIEmail,
}
}
Expand Down
8 changes: 3 additions & 5 deletions config/configfile_yaml.gotmpl
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
# DefraDB configuration (YAML)

# NOTE: Paths below are relative to the DefraDB directory unless otherwise stated.
# By default, the DefraDB directory is "$HOME/.defradb", but
# can be changed via the $DEFRA_ROOTDIR env variable or --rootdir CLI flag.
# The default DefraDB directory is "$HOME/.defradb". It can be changed via the --rootdir CLI flag.
# Relative paths are interpreted as being rooted in the DefraDB directory.

datastore:
# Store can be badger | memory
# badger: fast pure Go key-value store optimized for SSDs (https://github.com/dgraph-io/badger)
# memory: in-memory version of badger
store: {{ .Datastore.Store }}
badger:
# The path to the database data file(s), relative paths here will be converted to absolute file paths
# on database start.
# The path to the database data file(s).
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I do think it is worth documenting that we will mutate the config file and replace relative paths with absolute paths. I see that as potentially surprising behaviour, and a behaviour that may potentially mess with CI systems (e.g. config files are sometimes directly pulled from source control, and impromptu edits can mess with the next pull)

Copy link
Contributor Author

@orpheuslummis orpheuslummis Apr 11, 2023

Choose a reason for hiding this comment

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

Currently we don't overwrite the config file. We write a config file when it doesn't exist. For now, the paths written are absolute paths, although what would be nicer is to have relative paths there, but which will probabIy be enabled by a refactor of config.

Copy link
Contributor

Choose a reason for hiding this comment

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

for future reference - we chatted over discord, and I was wrong here - it doesnt mutate existing files :)

path: {{ .Datastore.Badger.Path }}
# Maximum file size of the value log files. The in-memory file size will be 2*valuelogfilesize.
# Human friendly units can be used (ex: 500MB).
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/cli/log_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func captureLogLines(t *testing.T, setup func(), predicate func()) []string {
// Set the default logger output path to a file in the temp dir
// so that production logs don't polute and confuse the tests
// os.Args = append(os.Args, "--logoutput", directory+"/log.txt")
os.Args = append(os.Args, "init", directory)
os.Args = append(os.Args, "init", "--rootdir", directory)

setup()
cli.Execute()
Expand Down