improve several kwild cmd issues with config parsing and error handling #1284
Conversation
jchappelow
changed the title
|
|
||
| // clean up any previous temp admin snapshots | ||
| err = cleanupTmpKwilAdminDir(dir) |
There was a problem hiding this comment.
@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.
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.
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).
| }, | ||
| } | ||
|
|
||
| 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.
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.
Should we have root dir bound globally?
Entire subcommand (e.g. validators) have no use for it.
There was a problem hiding this comment.
Almost all commands actually do. In the case of all the commands that use admin RPC, like validators, the on-disk config is used as a default when --rpcserver is not set on the CLI (if the config file doesn't exist, the hard-coded default are used, of course). This means root directory can affect it.
We have the ability to hide it for other commands as needed though. For example key gen had it's own output flag, and the root flag their was confusing, so I hid it.
use node config file in snapshot and reset commands fix more misuse of display.PrintErr
jchappelow
force-pushed
the
ver-acct-misc
branch
from
cfcc680 to
5ed5aba
Compare
Going through several small things that @KwilLuke mentioned yesterday
~/.kwild/config.tomlunnecessarily, leading to confusing errors if the file is invalid since it's really irrelevant to the commanddisplay.PrintErrand document proper useconf.ActiveConfig, which is the merged configuration in several commands instead of manually loading the toml filestartcommand's docs to reflect that it is no longer the default commandsetup initwill error if the destination directory already exists. Previous it ran with unpredictable results.