-
Notifications
You must be signed in to change notification settings - Fork 12
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
improve several kwild cmd issues with config parsing and error handling #1284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,12 @@ import ( | |
"os" | ||
"path/filepath" | ||
|
||
"github.com/kwilteam/kwil-db/app/node/conf" | ||
"github.com/kwilteam/kwil-db/app/shared/bind" | ||
"github.com/kwilteam/kwil-db/app/shared/display" | ||
"github.com/kwilteam/kwil-db/app/snapshot" | ||
"github.com/kwilteam/kwil-db/config" | ||
"github.com/kwilteam/kwil-db/node" | ||
"github.com/kwilteam/kwil-db/node/pg" | ||
"github.com/spf13/cobra" | ||
) | ||
|
||
|
@@ -41,16 +41,19 @@ kwild setup genesis-hash --snapshot "/path/to/snapshot.sql.gz" --genesis "~/.kwi | |
) | ||
|
||
func GenesisHashCmd() *cobra.Command { | ||
var genesisFile, rootDir, snapshotFile string | ||
var genesisFile, snapshotFile string | ||
|
||
cmd := &cobra.Command{ | ||
Use: "genesis-hash", | ||
Short: "Compute genesis hash from existing PostgreSQL data, and optionally update genesis.json.", | ||
Long: genesisHashLong, | ||
Example: genesisHashExample, | ||
Args: cobra.NoArgs, | ||
// Override the root's PersistentPreRunE to bind only the config file, | ||
// not the full node flag set. | ||
PersistentPreRunE: bind.ChainPreRuns(conf.PreRunBindConfigFileStrict[config.Config]), // but not the flags | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
if cmd.Flags().Changed("snapshot") && cmd.Flags().Changed("root-dir") { | ||
if cmd.Flags().Changed("snapshot") && cmd.Flags().Changed(bind.RootFlagName) { | ||
return display.PrintErr(cmd, errors.New("cannot use both --snapshot and --root-dir")) | ||
} | ||
|
||
|
@@ -61,43 +64,30 @@ func GenesisHashCmd() *cobra.Command { | |
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
} else { | ||
var pgConf *pg.ConnConfig | ||
var err error | ||
if rootDir != "" { | ||
rootDir, err = node.ExpandPath(rootDir) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
|
||
pgConf, err = loadPGConfigFromTOML(cmd, rootDir) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
} else { | ||
pgConf, err = bind.GetPostgresFlags(cmd) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
} else { // create a snapshot first | ||
dbCfg := conf.ActiveConfig().DB | ||
pgConf, err := bind.GetPostgresFlags(cmd, &dbCfg) | ||
if err != nil { | ||
return display.PrintErr(cmd, fmt.Errorf("failed to get postgres flags: %v", err)) | ||
} | ||
|
||
dir, err := tmpKwilAdminSnapshotDir() | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
|
||
// ensure the temp admin snapshots directory exists | ||
err = ensureTmpKwilAdminDir(dir) | ||
// clean up any previous temp admin snapshots | ||
err = cleanupTmpKwilAdminDir(dir) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
defer cleanupTmpKwilAdminDir(dir) // clean up temp admin snapshots directory on exit after app hash computation | ||
|
||
// clean up any previous temp admin snapshots | ||
err = cleanupTmpKwilAdminDir(dir) | ||
// ensure the temp admin snapshots directory exists | ||
err = ensureTmpKwilAdminDir(dir) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
defer cleanupTmpKwilAdminDir(dir) // clean up temp admin snapshots directory on exit after app hash computation | ||
|
||
_, _, genCfg, err := snapshot.PGDump(cmd.Context(), pgConf.DBName, pgConf.User, pgConf.Pass, pgConf.Host, pgConf.Port, dir) | ||
if err != nil { | ||
|
@@ -107,15 +97,22 @@ func GenesisHashCmd() *cobra.Command { | |
appHash = genCfg.StateHash | ||
} | ||
|
||
return writeAndReturnGenesisHash(cmd, genesisFile, appHash) | ||
if genesisFile != "" { | ||
err := writeAndReturnGenesisHash(genesisFile, appHash) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
} | ||
return display.PrintCmd(cmd, &genesisHashRes{ | ||
Hash: base64.StdEncoding.EncodeToString(appHash), | ||
}) | ||
}, | ||
} | ||
|
||
cmd.Flags().StringVarP(&genesisFile, "genesis", "g", "", "optional path to the genesis file to patch with the computed app hash") | ||
cmd.Flags().StringVarP(&rootDir, "root-dir", "r", "", "optional path to the root directory of the kwild node from which the genesis hash will be computed") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root is bound globally. A handful of commands hide the flag, but the root command binds it for all subcommands. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have root dir bound globally? Entire subcommand (e.g. validators) have no use for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost all commands actually do. In the case of all the commands that use admin RPC, like |
||
cmd.Flags().StringVarP(&snapshotFile, "snapshot", "s", "", "optional path to the snapshot file to use for the genesis hash computation") | ||
|
||
bind.BindPostgresFlags(cmd) | ||
bind.BindPostgresFlags(cmd, &conf.ActiveConfig().DB) | ||
return cmd | ||
} | ||
|
||
|
@@ -152,56 +149,20 @@ func appHashFromSnapshotFile(filePath string) ([]byte, error) { | |
return hash.Sum(nil), nil | ||
} | ||
|
||
// loadPGConfigFromTOML loads the postgres connection configuration from the toml file | ||
// and merges it with the flags set in the command. | ||
func loadPGConfigFromTOML(cmd *cobra.Command, rootDir string) (*pg.ConnConfig, error) { | ||
tomlFile := config.ConfigFilePath(rootDir) | ||
|
||
// Check if the config file exists | ||
if _, err := os.Stat(tomlFile); os.IsNotExist(err) { | ||
return bind.GetPostgresFlags(cmd) // return the flags if the config file does not exist | ||
} | ||
|
||
cfg, err := config.LoadConfig(tomlFile) | ||
func writeAndReturnGenesisHash(genesisFile string, appHash []byte) error { | ||
genesisFile, err := node.ExpandPath(genesisFile) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load config from toml file: %w", err) | ||
return err | ||
} | ||
|
||
conf := &pg.ConnConfig{ | ||
Host: cfg.DB.Host, | ||
Port: cfg.DB.Port, | ||
User: cfg.DB.User, | ||
Pass: cfg.DB.Pass, | ||
DBName: cfg.DB.DBName, | ||
cfg, err := config.LoadGenesisConfig(genesisFile) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// merge the flags with the config file | ||
return bind.MergePostgresFlags(conf, cmd) | ||
} | ||
|
||
func writeAndReturnGenesisHash(cmd *cobra.Command, genesisFile string, appHash []byte) error { | ||
if genesisFile != "" { | ||
genesisFile, err := node.ExpandPath(genesisFile) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
|
||
cfg, err := config.LoadGenesisConfig(genesisFile) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
|
||
cfg.StateHash = appHash | ||
cfg.StateHash = appHash | ||
|
||
err = cfg.SaveAs(genesisFile) | ||
if err != nil { | ||
return display.PrintErr(cmd, err) | ||
} | ||
} | ||
|
||
return display.PrintCmd(cmd, &genesisHashRes{ | ||
Hash: base64.StdEncoding.EncodeToString(appHash), | ||
}) | ||
return cfg.SaveAs(genesisFile) | ||
} | ||
|
||
type genesisHashRes struct { | ||
|
@@ -234,14 +195,11 @@ func tmpKwilAdminSnapshotDir() (string, error) { | |
|
||
// ensureTmpKwilAdminDir ensures that the temporary directory for kwil-admin snapshots exists. | ||
func ensureTmpKwilAdminDir(dir string) error { | ||
if _, err := os.Stat(dir); os.IsNotExist(err) { | ||
_, err := os.Stat(dir) | ||
if os.IsNotExist(err) { | ||
err = os.Mkdir(dir, 0755) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
return err | ||
} | ||
|
||
// cleanupTmpKwilAdminDir removes the temporary directory for kwil-admin snapshots. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charithabandi AFAICT, this was deleting the directory that was just created above, and the
PGDump
function was recreating it internally. Please double check this for me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, its temporary dir for storing snapshots till we get the genesis hash, we don't actually need it after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that's why we defer the cleanup, but I'm saying the original order in this function made no sense. It created it and immediately removed it, then created it again (this last one inside
snapshot.PGDump
).