fix(app): exit with error code and use regen env prefix #285
Conversation
| rootCmd, _ := cmd.NewRootCmd() | ||
| rootCmd.SetArgs([]string{ | ||
| "init", // Test the init cmd | ||
| "regenapp-test", // Moniker | ||
| fmt.Sprintf("--%s=%s", cli.FlagOverwrite, "true"), // Overwrite genesis.json, in case it already exists |
There was a problem hiding this comment.
I dropped this as it seemed unnecessary. If folks think there is a good reason to keep this flag, I can add it back in.
There was a problem hiding this comment.
Hmm I don't think it hurts. There could be edge cases where is needed.
There was a problem hiding this comment.
What edge cases are you thinking of? Ideally this should be run as a self contained test with full teardown of any artifacts right? The home directory is created from scratch on each test run, so I don't see how we could expect there to already be a genesis.json here.
Previously this line was there because it always looked at your default home directory for running (or created one if it didnt exist). This PR changes that so it creates a temp home dir, which should render the --overwrite=true flag unnecessary. Lmk if i'm missing something
app/regen/cmd/root.go
Outdated
| @@ -69,26 +66,6 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) { | |||
| return rootCmd, encodingConfig | |||
| } | |||
|
|
|||
| // Execute executes the root command. | |||
| func Execute(rootCmd *cobra.Command) error { | |||
There was a problem hiding this comment.
In Cosmos SDK simapp we're now using sdk's server.Execute(), i figured this was what we should update to as well. There's no need to have this function defined in regen-ledger if its exposed on the SDK right? (see cosmos/cosmos-sdk#8144)
There was a problem hiding this comment.
@clevinson Only issue I see is, we may not be able to use prefix based ENV variables. iirc, PrepareBaseCmd takes an argument (in our case it was REGEN) through which it maps ENV variables to flags. I didn't test if it works with empty prefix, if not, that might create some UX issues
There was a problem hiding this comment.
@anilcse Hmm... Yeah you're 100% right here. Seems strange then that server.Execute() is exposed at all if it doesn't provide a mechanism for chains to add their own prefix... Do you think this should be addressed upstream?
Maybe best to revert this part of my PR for now.
There was a problem hiding this comment.
Yes, we can fix this upstream to take an extra argument for this ENV prefix.
There was a problem hiding this comment.
Opened a pull request in the sdk: cosmos/cosmos-sdk#10950
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
==========================================
- Coverage 77.45% 68.31% -9.15%
==========================================
Files 221 107 -114
Lines 16120 16029 -91
==========================================
- Hits 12486 10950 -1536
- Misses 2620 4291 +1671
+ Partials 1014 788 -226
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
@anilcse could you possibly take a look at this? I don't have very good context. |
|
Is this PR still relevant? Anyone working on it or should have a look? |
ryanchristo
changed the title
|
This PR will need to wait for 0.46 release (see #285 (comment)) |
ryanchristo
changed the title
When working on #277 I kept having local tests fail despite CI working fine. Turns out that
TestInitCmd()was looking into the default node home directory (~/.regen) for testing out the root command. This is fixed now (we should probably also fix this upstream on the SDK in the simapp tests?).While investigating, there were a few other updates in usages of
server.Execute()that I found in the SDK which I wanted to port over here to regen ledger, as it seems this is how we should be interfacing w/ the root command now.Those PRs were cosmos/cosmos-sdk#8144, and cosmos/cosmos-sdk#7480