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

Jump to bottom

fix(app): exit with error code and use regen env prefix #285

Merged
merged 6 commits into from
Aug 5, 2022
Merged

fix(app): exit with error code and use regen env prefix #285

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc2afa8
server.Execute cleanup, and make TestInitCmd self contained
clevinson Mar 5, 2021
d78c5d8
fix setting of home flag
clevinson Mar 5, 2021
6dbf0d4
Merge branch 'master' into clevinson/isolated-cmd-test
anilcse Mar 8, 2021
b69c246
Merge branch 'master' into clevinson/isolated-cmd-test
ryanchristo Jan 13, 2022
d74199c
Merge branch 'master' into clevinson/isolated-cmd-test
ryanchristo Jan 13, 2022
d29448f
Merge branch 'master' into clevinson/isolated-cmd-test
ryanchristo Aug 5, 2022
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: 6 additions & 4 deletions app/regen/cmd/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
package regen_test

import (
"fmt"
"io/ioutil"
"testing"

svrcmd "github.com/cosmos/cosmos-sdk/server/cmd"
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/x/genutil/client/cli"
cmd "github.com/regen-network/regen-ledger/app/regen/cmd"
)

func TestInitCmd(t *testing.T) {
nodeHome, err := ioutil.TempDir(t.TempDir(), ".regen")
require.NoError(t, err)

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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I don't think it hurts. There could be edge cases where is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

})

err := cmd.Execute(rootCmd)
err = svrcmd.Execute(rootCmd, nodeHome)
require.NoError(t, err)
}
23 changes: 0 additions & 23 deletions app/regen/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package regen

import (
"context"
"errors"
"io"
"os"
Expand All @@ -12,10 +11,8 @@ import (
"github.com/cosmos/cosmos-sdk/snapshots"
"github.com/regen-network/regen-ledger/app"

"github.com/rs/zerolog"
"github.com/spf13/cast"
"github.com/spf13/cobra"
tmcfg "github.com/tendermint/tendermint/config"
tmcli "github.com/tendermint/tendermint/libs/cli"
"github.com/tendermint/tendermint/libs/log"
dbm "github.com/tendermint/tm-db"
Expand Down Expand Up @@ -69,26 +66,6 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
return rootCmd, encodingConfig
}

// Execute executes the root command.
func Execute(rootCmd *cobra.Command) error {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this deleted?

Copy link
Member Author

@clevinson clevinson Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@anilcse anilcse Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can fix this upstream to take an extra argument for this ENV prefix.

Copy link
Member

Choose a reason for hiding this comment

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

Opened a pull request in the sdk: cosmos/cosmos-sdk#10950

// Create and set a client.Context on the command's Context. During the pre-run
// of the root command, a default initialized client.Context is provided to
// seed child command execution with values such as AccountRetriver, Keyring,
// and a Tendermint RPC. This requires the use of a pointer reference when
// getting and setting the client.Context. Ideally, we utilize
// https://github.com/spf13/cobra/pull/1118.

ctx := context.Background()
ctx = context.WithValue(ctx, client.ClientContextKey, &client.Context{})
ctx = context.WithValue(ctx, server.ServerContextKey, server.NewDefaultContext())

rootCmd.PersistentFlags().String(flags.FlagLogLevel, zerolog.InfoLevel.String(), "The logging level (trace|debug|info|warn|error|fatal|panic)")
rootCmd.PersistentFlags().String(flags.FlagLogFormat, tmcfg.LogFormatPlain, "The logging format (json|plain)")

executor := tmcli.PrepareBaseCmd(rootCmd, "REGEN", app.DefaultNodeHome)
return executor.ExecuteContext(ctx)
}

func initRootCmd(rootCmd *cobra.Command, encodingConfig params.EncodingConfig) {
authclient.Codec = encodingConfig.Marshaler

Expand Down
13 changes: 11 additions & 2 deletions app/regen/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,22 @@ package main
import (
"os"

"github.com/cosmos/cosmos-sdk/server"
svrcmd "github.com/cosmos/cosmos-sdk/server/cmd"

"github.com/regen-network/regen-ledger/app"
cmd "github.com/regen-network/regen-ledger/app/regen/cmd"
)

// In main we call the rootCmd
func main() {
rootCmd, _ := cmd.NewRootCmd()
if err := cmd.Execute(rootCmd); err != nil {
os.Exit(1)
if err := svrcmd.Execute(rootCmd, app.DefaultNodeHome); err != nil {
switch e := err.(type) {
case server.ErrorCode:
os.Exit(e.Code)
default:
os.Exit(1)
}
}
}