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

feat(tools/confix): upgrade confix to migrate client.toml files #18455

Merged
merged 9 commits into from
Nov 29, 2023
12 changes: 9 additions & 3 deletions tools/confix/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,20 @@ confix set ~/.simapp/config/client.toml chain-id "foo-1" # sets the value chain-

### Migrate

Migrate a configuration file to a new version, e.g.:
Migrate a configuration file to a new version, config type defaults to `app.toml`, if you want to change it to `client.toml`, please indicate it by adding the optional parameter, e.g.:

```shell
simd config migrate v0.47 # migrates defaultHome/config/app.toml to the latest v0.47 config
simd config migrate v0.50 # migrates defaultHome/config/app.toml to the latest v0.47 config
```

```shell
confix migrate v0.47 ~/.simapp/config/app.toml # migrate ~/.simapp/config/app.toml to the latest v0.47 config
confix migrate v0.50 ~/.simapp/config/app.toml # migrate ~/.simapp/config/app.toml to the latest v0.47 config
```

or

```shell
confix migrate v0.50 ~/.simapp/config/client.toml client # migrate ~/.simapp/config/client.toml to the latest v0.47 config
```

### Diff
Expand Down
35 changes: 18 additions & 17 deletions tools/confix/cmd/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,45 @@ package cmd
import (
"errors"
"fmt"
"strings"

"github.com/cosmos/cosmos-sdk/client"
"github.com/spf13/cobra"
"golang.org/x/exp/maps"

"cosmossdk.io/tools/confix"

"github.com/cosmos/cosmos-sdk/client"
)

// DiffCommand creates a new command for comparing configuration files
func DiffCommand() *cobra.Command {
return &cobra.Command{
Use: "diff [target-version] <app-toml-path>",
Short: "Outputs all config values that are different from the app.toml defaults.",
Args: cobra.MinimumNArgs(1),
Use: "diff [target-version] <config-path> [config-type]",
Short: "Outputs all config values that are different from the default.",
Long: "This command compares the specified configuration file (app.toml or client.toml) with the defaults and outputs any differences.",
Args: cobra.MinimumNArgs(2),
amedumer marked this conversation as resolved.
Show resolved Hide resolved
amedumer marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@julienrbrt julienrbrt Nov 29, 2023

Choose a reason for hiding this comment

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

Oh, this wasn't resolved. cobra.MinimumNArgs(2) should have stayed as 1, as the path shouldn't be always needed. I'll make a follow-up.

RunE: func(cmd *cobra.Command, args []string) error {
var filename string
targetVersion := args[0]
configPath := args[1]
configType := confix.AppConfigType // Default to app configuration
clientCtx := client.GetClientContextFromCmd(cmd)
switch {

case len(args) > 1:
filename = args[1]
case clientCtx.HomeDir != "":
filename = fmt.Sprintf("%s/config/app.toml", clientCtx.HomeDir)
default:
return errors.New("must provide a path to the app.toml file")

if len(args) > 2 {
configType = strings.ToLower(args[2])
if configType != confix.AppConfigType && configType != confix.ClientConfigType {
amedumer marked this conversation as resolved.
Show resolved Hide resolved
return errors.New("config type must be 'app' or 'client'")
}
}

targetVersion := args[0]
if _, ok := confix.Migrations[targetVersion]; !ok {
return fmt.Errorf("unknown version %q, supported versions are: %q", targetVersion, maps.Keys(confix.Migrations))
}

targetVersionFile, err := confix.LoadLocalConfig(targetVersion)
targetVersionFile, err := confix.LoadLocalConfig(targetVersion, configType)
if err != nil {
panic(fmt.Errorf("failed to load internal config: %w", err))
amedumer marked this conversation as resolved.
Show resolved Hide resolved
}

rawFile, err := confix.LoadConfig(filename)
rawFile, err := confix.LoadConfig(configPath)
if err != nil {
return fmt.Errorf("failed to load config: %w", err)
}
Expand Down
40 changes: 26 additions & 14 deletions tools/confix/cmd/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ import (
"context"
"errors"
"fmt"
"strings"

"github.com/cosmos/cosmos-sdk/client"
"github.com/spf13/cobra"
"golang.org/x/exp/maps"

"cosmossdk.io/tools/confix"

"github.com/cosmos/cosmos-sdk/client"
)


var (
FlagStdOut bool
FlagVerbose bool
Expand All @@ -21,31 +22,42 @@ var (

func MigrateCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "migrate [target-version] <app-toml-path> (options)",
Short: "Migrate Cosmos SDK app configuration file to the specified version",
Long: `Migrate the contents of the Cosmos SDK app configuration (app.toml) to the specified version.
Use: "migrate [target-version] <config-path> [config-type]",
Short: "Migrate Cosmos SDK configuration file to the specified version",
Long: `Migrate the contents of the Cosmos SDK configuration (app.toml or client.toml) to the specified version. Configuration type is app by default.
The output is written in-place unless --stdout is provided.
In case of any error in updating the file, no output is written.`,
Args: cobra.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
var filename string
var configPath string

clientCtx := client.GetClientContextFromCmd(cmd)
amedumer marked this conversation as resolved.
Show resolved Hide resolved
switch {
targetVersion := args[0]
configType := confix.AppConfigType // Default to app configuration

if len(args) > 2 {
configType = strings.ToLower(args[2])
if configType != confix.AppConfigType && configType != confix.ClientConfigType {
amedumer marked this conversation as resolved.
Show resolved Hide resolved
return errors.New("config type must be 'app' or 'client'")
}
}

switch {
case len(args) > 1:
filename = args[1]
configPath = args[1]
case clientCtx.HomeDir != "":
filename = fmt.Sprintf("%s/config/app.toml", clientCtx.HomeDir)
configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType)
default:
return errors.New("must provide a path to the app.toml file")
return errors.New("must provide a path to the config file")
Comment on lines +46 to +52
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2023

Choose a reason for hiding this comment

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

The logic for determining the configPath is sound, but there is a potential issue with the switch statement. It does not have an expression to switch on, which is not valid syntax in Go. This should be corrected to an if statement or the switch should be given an appropriate expression.

- switch  {
+ if len(args) > 1 {

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change
switch {
case len(args) > 1:
filename = args[1]
configPath = args[1]
case clientCtx.HomeDir != "":
filename = fmt.Sprintf("%s/config/app.toml", clientCtx.HomeDir)
configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType)
default:
return errors.New("must provide a path to the app.toml file")
return errors.New("must provide a path to the config file")
if len(args) > 1 {
configPath = args[1]
case clientCtx.HomeDir != "":
configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType)
default:
return errors.New("must provide a path to the config file")

Copy link
Collaborator

Choose a reason for hiding this comment

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

That advice is not correct, Go allows for more flexible switch statements such as has been included in this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@odeke-em You are absolutely right, and I appreciate the correction. Go does indeed support "tagless" switch statements, which evaluate cases as boolean expressions. My apologies for the oversight. Thank you for pointing that out!

}

targetVersion := args[0]

plan, ok := confix.Migrations[targetVersion]
if !ok {
return fmt.Errorf("unknown version %q, supported versions are: %q", targetVersion, maps.Keys(confix.Migrations))
}

rawFile, err := confix.LoadConfig(filename)
rawFile, err := confix.LoadConfig(configPath)
if err != nil {
return fmt.Errorf("failed to load config: %w", err)
}
Expand All @@ -55,12 +67,12 @@ In case of any error in updating the file, no output is written.`,
ctx = confix.WithLogWriter(ctx, cmd.ErrOrStderr())
}

outputPath := filename
outputPath := configPath
if FlagStdOut {
outputPath = ""
}

if err := confix.Upgrade(ctx, plan(rawFile, targetVersion), filename, outputPath, FlagSkipValidate); err != nil {
if err := confix.Upgrade(ctx, plan(rawFile, targetVersion, configType), configPath, outputPath, FlagSkipValidate); err != nil {
return fmt.Errorf("failed to migrate config: %w", err)
}

Expand Down
18 changes: 10 additions & 8 deletions tools/confix/cmd/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,30 +9,32 @@ import (

"cosmossdk.io/tools/confix/cmd"

"github.com/cosmos/cosmos-sdk/client"
clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli"
)

func TestMigradeCmd(t *testing.T) {
func TestMigrateCmd(t *testing.T) {
clientCtx, cleanup := initClientContext(t)
defer cleanup()

_, err := clitestutil.ExecTestCLICmd(client.Context{}, cmd.MigrateCommand(), []string{"v0.0"})
assert.ErrorContains(t, err, "must provide a path to the app.toml file")

_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.0"})
_, err := clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.0","app"})
assert.ErrorContains(t, err, "unknown version")

// clientCtx does not create app.toml, so this should fail
_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.45"})
_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.45","app"})
assert.ErrorContains(t, err, "no such file or directory")

// try to migrate from client.toml it should fail without --skip-validate
_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.46", fmt.Sprintf("%s/config/client.toml", clientCtx.HomeDir)})
_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.46", fmt.Sprintf("%s/config/client.toml", clientCtx.HomeDir),"app"})
amedumer marked this conversation as resolved.
Show resolved Hide resolved
assert.ErrorContains(t, err, "failed to migrate config")

// try to migrate from client.toml - it should work and give us a big diff
out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.46", fmt.Sprintf("%s/config/client.toml", clientCtx.HomeDir), "--skip-validate", "--verbose"})
assert.NilError(t, err)
assert.Assert(t, strings.Contains(out.String(), "add app-db-backend key"))


// this should work
out, err = clitestutil.ExecTestCLICmd(clientCtx, cmd.MigrateCommand(), []string{"v0.51", fmt.Sprintf("%s/config/client.toml", clientCtx.HomeDir),"client", "--verbose"})
amedumer marked this conversation as resolved.
Show resolved Hide resolved
assert.NilError(t, err)
assert.Assert(t, strings.Contains(out.String(), "add keyring-default-keyname key"))
}
17 changes: 17 additions & 0 deletions tools/confix/data/v0.47-client.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# This is a TOML config file.
# For more information, see https://github.com/toml-lang/toml

###############################################################################
### Client Configuration ###
###############################################################################

# The network chain ID
chain-id = "demo"
# The keyring's backend, where the keys are stored (os|file|kwallet|pass|test|memory)
keyring-backend = "os"
# CLI output format (text|json)
output = "text"
# <host>:<port> to Tendermint RPC interface for this chain
node = "tcp://localhost:26657"
# Transaction broadcasting mode (sync|async|block)
broadcast-mode = "sync"
17 changes: 17 additions & 0 deletions tools/confix/data/v0.50-client.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# This is a TOML config file.
# For more information, see https://github.com/toml-lang/toml

###############################################################################
### Client Configuration ###
###############################################################################

# The network chain ID
chain-id = "demo"
# The keyring's backend, where the keys are stored (os|file|kwallet|pass|test|memory)
keyring-backend = "os"
# CLI output format (text|json)
output = "text"
# <host>:<port> to Tendermint RPC interface for this chain
node = "tcp://localhost:26657"
# Transaction broadcasting mode (sync|async|block)
broadcast-mode = "sync"
19 changes: 19 additions & 0 deletions tools/confix/data/v0.51-client.toml
amedumer marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# This is a TOML config file.
# For more information, see https://github.com/toml-lang/toml

###############################################################################
### Client Configuration ###
###############################################################################

# The network chain ID
chain-id = "demo"
# The keyring's backend, where the keys are stored (os|file|kwallet|pass|test|memory)
keyring-backend = "test"
# Default key name, if set, defines the default key to use for signing transaction when the --from flag is not specified
keyring-default-keyname = ""
# CLI output format (text|json)
output = "text"
# <host>:<port> to CometBFT RPC interface for this chain
node = "tcp://localhost:26657"
# Transaction broadcasting mode (sync|async)
broadcast-mode = "sync"
26 changes: 22 additions & 4 deletions tools/confix/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,24 @@ import (
"embed"
"fmt"
"os"
"strings"

"github.com/creachadair/tomledit"
)

//go:embed data
var data embed.FS

// LoadConfig loads and parses the TOML document from confix data
func LoadLocalConfig(name string) (*tomledit.Document, error) {
f, err := data.Open(fmt.Sprintf("data/%s-app.toml", name))
// LoadLocalConfig loads and parses the TOML document from confix data
func LoadLocalConfig(name string, configType string) (*tomledit.Document, error) {
fileName, err := getFileName(name, configType)
if err != nil {
panic(fmt.Errorf("failed to read file: %w. This file should have been included in confix", err))
return nil, err
}

f, err := data.Open(fmt.Sprintf("data/%s", fileName))
amedumer marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("failed to read file: %w. This file should have been included in confix", err)
}
defer f.Close()

Copy link
Contributor

Choose a reason for hiding this comment

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

The refactoring of LoadLocalConfig to include a configType parameter and use the new getFileName function is a good approach to handle different types of configuration files. The error handling is also improved by returning errors instead of panicking, which is a best practice for error handling in Go. This allows the caller to decide how to handle the error, rather than the application crashing.

Expand All @@ -32,3 +38,15 @@ func LoadConfig(path string) (*tomledit.Document, error) {

return tomledit.Parse(f)
}

// getFileName constructs the filename based on the type of configuration (app or client)
func getFileName(name string, configType string) (string, error) {
switch strings.ToLower(configType) {
case "app":
return fmt.Sprintf("%s-app.toml", name), nil
case "client":
return fmt.Sprintf("%s-client.toml", name), nil
default:
return "", fmt.Errorf("unsupported config type: %s", configType)
amedumer marked this conversation as resolved.
Show resolved Hide resolved
}
}
11 changes: 7 additions & 4 deletions tools/confix/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,30 @@ import (

const (
AppConfig = "app.toml"
AppConfigType = "app"
ClientConfig = "client.toml"
ClientConfigType = "client"
CMTConfig = "config.toml"
)

// MigrationMap defines a mapping from a version to a transformation plan.
type MigrationMap map[string]func(from *tomledit.Document, to string) transform.Plan
type MigrationMap map[string]func(from *tomledit.Document, to string, planType string) transform.Plan

var Migrations = MigrationMap{
"v0.45": NoPlan, // Confix supports only the current supported SDK version. So we do not support v0.44 -> v0.45.
"v0.46": PlanBuilder,
"v0.47": PlanBuilder,
"v0.50": PlanBuilder,
"v0.51": PlanBuilder,
// "v0.xx.x": PlanBuilder, // add specific migration in case of configuration changes in minor versions
}

// PlanBuilder is a function that returns a transformation plan for a given diff between two files.
func PlanBuilder(from *tomledit.Document, to string) transform.Plan {
func PlanBuilder(from *tomledit.Document, to string, planType string) transform.Plan {
plan := transform.Plan{}
deletedSections := map[string]bool{}

target, err := LoadLocalConfig(to)
target, err := LoadLocalConfig(to,planType)
amedumer marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous comment about error handling in PlanBuilder is still valid. The function should not panic but rather return an error. This is crucial for robust error handling and to avoid crashing the application.

- func PlanBuilder(from *tomledit.Document, to string, planType string) transform.Plan {
+ func PlanBuilder(from *tomledit.Document, to string, planType string) (transform.Plan, error) {
-   panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err))
+   return transform.Plan{}, fmt.Errorf("failed to parse file: %w. This file should have been valid", err)
}

And then update the call sites of PlanBuilder to handle the returned error.


Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change
target, err := LoadLocalConfig(to,planType)
if err != nil {
panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err))
func PlanBuilder(from *tomledit.Document, to string, planType string) (transform.Plan, error) {
target, err := LoadLocalConfig(to, planType)
if err != nil {
return transform.Plan{}, fmt.Errorf("failed to parse file: %w. This file should have been valid", err)
}

}
Expand Down Expand Up @@ -115,7 +118,7 @@ func PlanBuilder(from *tomledit.Document, to string) transform.Plan {
}

// NoPlan returns a no-op plan.
func NoPlan(_ *tomledit.Document, to string) transform.Plan {
func NoPlan(_ *tomledit.Document, to string, planType string) transform.Plan {
fmt.Printf("no migration needed to %s\n", to)
return transform.Plan{}
amedumer marked this conversation as resolved.
Show resolved Hide resolved
}
Loading