From 673792a1ab1583d47b01d01e8251a35aca736d79 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Thu, 17 Nov 2022 17:33:56 -0700 Subject: [PATCH 01/14] [1216]: Load the defaults before loading each file. --- cmd/provenanced/config/manager.go | 33 +++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/cmd/provenanced/config/manager.go b/cmd/provenanced/config/manager.go index f8a7066cbc..3891b3464a 100644 --- a/cmd/provenanced/config/manager.go +++ b/cmd/provenanced/config/manager.go @@ -400,13 +400,14 @@ func loadUnpackedConfig(cmd *cobra.Command) error { // Both the server context and client context should be using the same Viper, so this is good for both. vpr := server.GetServerContextFromCmd(cmd).Viper - // Load the tendermint config if it exists, or else defaults. + // Load the tendermint config defaults, then file if it exists. + tdErr := addFieldMapToViper(vpr, MakeFieldValueMap(tmconfig.DefaultConfig(), false)) + if tdErr != nil { + return fmt.Errorf("tendermint config defaults load error: %w", tdErr) + } switch _, err := os.Stat(tmConfFile); { case os.IsNotExist(err): - lerr := addFieldMapToViper(vpr, MakeFieldValueMap(tmconfig.DefaultConfig(), false)) - if lerr != nil { - return fmt.Errorf("tendermint config file load error: %w", lerr) - } + // Do nothing. case err != nil: return fmt.Errorf("tendermint config file stat error: %w", err) default: @@ -417,13 +418,14 @@ func loadUnpackedConfig(cmd *cobra.Command) error { } } - // Load the app/cosmos config if it exists, or else defaults. + // Load the app/cosmos config defaults, then file if it exists. + adErr := addFieldMapToViper(vpr, MakeFieldValueMap(serverconfig.DefaultConfig(), false)) + if adErr != nil { + return fmt.Errorf("app config defaults load error: %w", adErr) + } switch _, err := os.Stat(appConfFile); { case os.IsNotExist(err): - lerr := addFieldMapToViper(vpr, MakeFieldValueMap(serverconfig.DefaultConfig(), false)) - if lerr != nil { - return fmt.Errorf("app config load error: %w", lerr) - } + // Do nothing. case err != nil: return fmt.Errorf("app config file stat error: %w", err) default: @@ -434,13 +436,14 @@ func loadUnpackedConfig(cmd *cobra.Command) error { } } - // Load the client config if it exists, or else defaults. + // Load the client config defaults, then file if it exists. + cdErr := addFieldMapToViper(vpr, MakeFieldValueMap(DefaultClientConfig(), false)) + if cdErr != nil { + return fmt.Errorf("client config defaults load error: %w", cdErr) + } switch _, err := os.Stat(clientConfFile); { case os.IsNotExist(err): - lerr := addFieldMapToViper(vpr, MakeFieldValueMap(DefaultClientConfig(), false)) - if lerr != nil { - return fmt.Errorf("client config file load error: %w", lerr) - } + // Do nothing. case err != nil: return fmt.Errorf("client config file stat error: %w", err) default: From 0bc4924535a6eee095b521b0ca5f0cd1136a9757 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Thu, 17 Nov 2022 18:24:54 -0700 Subject: [PATCH 02/14] [1216]: Add a unit test for the update command. --- cmd/provenanced/cmd/config_test.go | 99 ++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/cmd/provenanced/cmd/config_test.go b/cmd/provenanced/cmd/config_test.go index 9e5f0a42b5..7cd59f4555 100644 --- a/cmd/provenanced/cmd/config_test.go +++ b/cmd/provenanced/cmd/config_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io/ioutil" + "os" "regexp" "strings" "testing" @@ -810,3 +811,101 @@ func (s *ConfigTestSuite) TestPackUnpack() { assert.True(t, provconfig.FileExists(clientFile), "file exists: client") }) } + +func (s *ConfigTestSuite) TestUpdate() { + // Write a dumb version of each file that is missing almost everything, but has an easily identifiable comment. + customComment := "# There's no way this is comment is in the real config." + uFileField := "bananas" + uFileValue := "no it's not" + uFileContents := fmt.Sprintf("%s\n%s = %q\n", customComment, uFileField, uFileValue) + dbBackend := "bananas" + tFileContents := fmt.Sprintf("%s\n%s = %q\n", customComment, "db_backend", dbBackend) + minGasPrices := "not a lot" + aFileContents := fmt.Sprintf("%s\n%s = %q\n", customComment, "minimum-gas-prices", minGasPrices) + chainId := "this-will-never-work" + cFileContents := fmt.Sprintf("%s\n%s = %q\n", customComment, "chain-id", chainId) + configCmd := s.getConfigCmd() + configDir := provconfig.GetFullPathToConfigDir(configCmd) + uFile := provconfig.GetFullPathToUnmanagedConf(configCmd) + tFile := provconfig.GetFullPathToTmConf(configCmd) + aFile := provconfig.GetFullPathToAppConf(configCmd) + cFile := provconfig.GetFullPathToClientConf(configCmd) + s.Require().NoError(os.MkdirAll(configDir, 0o755), "making config dir") + s.Require().NoError(os.WriteFile(uFile, []byte(uFileContents), 0o644), "writing unmanaged config") + s.Require().NoError(os.WriteFile(tFile, []byte(tFileContents), 0o644), "writing tm config") + s.Require().NoError(os.WriteFile(aFile, []byte(aFileContents), 0o644), "writing app config") + s.Require().NoError(os.WriteFile(cFile, []byte(cFileContents), 0o644), "writing client config") + + // Load the config from files. + // Usually this is done in the pre-run handler, but that's defined in the root command, + // so this one doesn't know about it. + err := provconfig.LoadConfigFromFiles(configCmd) + s.Require().NoError(err, "loading config from files") + + // Run the command! + args := []string{"update"} + configCmd.SetArgs(args) + err = configCmd.Execute() + s.Require().NoError(err, "%s %s - unexpected error in execution", configCmd.Name(), args) + + s.Run("unmanaged file is unchanged", func() { + actualUFileContents, err := os.ReadFile(uFile) + s.Require().NoError(err, "reading unmanaged file: %s", uFile) + s.Assert().Equal(uFileContents, string(actualUFileContents), "unmanaged file contents") + }) + + s.Run("tm file has been updated", func() { + actualTFileContents, err := os.ReadFile(tFile) + s.Require().NoError(err, "reading tm file: %s", tFile) + s.Assert().NotEqual(tFileContents, string(actualTFileContents), "tm file contents") + lines := strings.Split(string(actualTFileContents), "\n") + s.Assert().Greater(len(lines), 2, "number of lines in tm file.") + }) + + s.Run("app file has been updated", func() { + actualAFileContents, err := os.ReadFile(aFile) + s.Require().NoError(err, "reading app file: %s", aFile) + s.Assert().NotEqual(aFileContents, string(actualAFileContents), "app file contents") + lines := strings.Split(string(actualAFileContents), "\n") + s.Assert().Greater(len(lines), 2, "number of lines in app file.") + }) + + s.Run("client file has been updated", func() { + actualCFileContents, err := os.ReadFile(cFile) + s.Require().NoError(err, "reading client file: %s", cFile) + s.Assert().NotEqual(aFileContents, string(actualCFileContents), "client file contents") + lines := strings.Split(string(actualCFileContents), "\n") + s.Assert().Greater(len(lines), 2, "number of lines in client file.") + }) + + err = provconfig.LoadConfigFromFiles(configCmd) + s.Require().NoError(err, "loading config from files") + + s.Run("tm db_backend value unchanged", func() { + tmConfig, err := provconfig.ExtractTmConfig(configCmd) + s.Require().NoError(err, "ExtractTmConfig") + actual := tmConfig.DBBackend + s.Assert().Equal(dbBackend, actual, "DBBackend") + }) + + s.Run("app minimum-gas-prices value unchanged", func() { + appConfig, err := provconfig.ExtractAppConfig(configCmd) + s.Require().NoError(err, "ExtractAppConfig") + actual := appConfig.MinGasPrices + s.Assert().Equal(minGasPrices, actual, "MinGasPrices") + }) + + s.Run("client chain-id value unchanged", func() { + clientConfig, err := provconfig.ExtractClientConfig(configCmd) + s.Require().NoError(err, "ExtractClientConfig") + actual := clientConfig.ChainID + s.Assert().Equal(chainId, actual, "ChainID") + }) + + s.Run("unmanaged config entry still applies", func() { + ctx := client.GetClientContextFromCmd(configCmd) + vpr := ctx.Viper + actual := vpr.GetString(uFileField) + s.Assert().Equal(uFileValue, actual, "unmanaged config entry") + }) +} From ec0744ca8371884e643d3af33ba51a0059dfcd48 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Thu, 17 Nov 2022 18:26:18 -0700 Subject: [PATCH 03/14] [1216]: It turns out that the unpack command is exactly what's needed for update. So Just add that as an alias, and a little extra note in the Long entry for it. --- cmd/provenanced/cmd/config.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/provenanced/cmd/config.go b/cmd/provenanced/cmd/config.go index dc22fcf085..b61f6ed196 100644 --- a/cmd/provenanced/cmd/config.go +++ b/cmd/provenanced/cmd/config.go @@ -217,14 +217,17 @@ Settings that are their default value will not be included. // ConfigUnpackCmd returns a CLI command for creating the several config toml files. func ConfigUnpackCmd() *cobra.Command { cmd := &cobra.Command{ - Use: "unpack", - Short: "Unpack configuration into separate config files", + Use: "unpack", + Aliases: []string{"update"}, + Short: "Unpack configuration into separate config files", Long: fmt.Sprintf(`Unpack configuration into separate config files. Splits the %[1]s file into %[2]s, %[3]s, and %[4]s. Settings defined through environment variables will be included in the unpacked files. Default values are filled in appropriately. +This can also be used to update the config files using the current template so they include all current fields. + `, provconfig.PackedConfFilename, provconfig.AppConfFilename, provconfig.TmConfFilename, provconfig.ClientConfFilename), Example: fmt.Sprintf(`$ %[1]s unpack`, configCmdStart), Args: cobra.ExactArgs(0), From 06c450a05d3942a1006a6dc45a2385c928b6ccb7 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Thu, 17 Nov 2022 18:37:06 -0700 Subject: [PATCH 04/14] [1216]: Add changelog entries. --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c0cd12aff..8810acb7f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## Unreleased -* nothing +### Improvements + +* Alias the `config unpack` command to `config update`. It can be used to update config files to include new fields [PR 1233](https://github.com/provenance-io/provenance/pull/1233). +* When loading the unpacked configs, always load the defaults before reading the files (instead of only loading the defaults if the file doesn't exist) [PR 1233](https://github.com/provenance-io/provenance/pull/1233). --- From c2fc4174544fa45eef66e469eb6bf357f1ac7fc4 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 15:21:45 -0700 Subject: [PATCH 05/14] [1216]: Move the arg checks to the top of runConfigSetCmd. --- cmd/provenanced/cmd/config.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/cmd/provenanced/cmd/config.go b/cmd/provenanced/cmd/config.go index b61f6ed196..e8835af28d 100644 --- a/cmd/provenanced/cmd/config.go +++ b/cmd/provenanced/cmd/config.go @@ -315,10 +315,17 @@ func runConfigGetCmd(cmd *cobra.Command, args []string) error { } // runConfigSetCmd sets values as provided. -// The first return value is whether or not to include help with the output of an error. +// The first return value is whether to include help with the output of an error. // This will only ever be true if an error is also returned. // The second return value is any error encountered. func runConfigSetCmd(cmd *cobra.Command, args []string) (bool, error) { + if len(args) == 0 { + return true, errors.New("no key/value pairs provided") + } + if len(args)%2 != 0 { + return true, errors.New("an even number of arguments are required when setting values") + } + // Warning: This wipes out all the viper setup stuff up to this point. // It needs to be done so that just the file values or defaults are loaded // without considering environment variables. @@ -347,12 +354,6 @@ func runConfigSetCmd(cmd *cobra.Command, args []string) (bool, error) { return false, fmt.Errorf("couldn't get client config: %w", ccerr) } - if len(args) == 0 { - return true, errors.New("no key/value pairs provided") - } - if len(args)%2 != 0 { - return true, errors.New("an even number of arguments are required when setting values") - } keyCount := len(args) / 2 keys := make([]string, keyCount) vals := make([]string, keyCount) From 8361b006dd74362fa89e8d9e844bf13d8cade471 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 15:27:36 -0700 Subject: [PATCH 06/14] [1216]: When loading a FieldValueMap into viper, Instead of loading sectioned keys into viper as the full thing, use a section entry with each sub-key in that. That's how it gets loaded when being read from a file. And since we now want to load the defaults (via FieldValueMap) then, if the file exists, read/load it, we ended up with things in two places. It was either do this or change all the file loading to manually read the file, create FieldValueMaps from it and give that to viper. --- cmd/provenanced/config/reflector.go | 61 +++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/cmd/provenanced/config/reflector.go b/cmd/provenanced/config/reflector.go index 4077e6e57e..ff57f13f54 100644 --- a/cmd/provenanced/config/reflector.go +++ b/cmd/provenanced/config/reflector.go @@ -223,6 +223,67 @@ func (m FieldValueMap) SetFromString(key, valueStr string) error { return fmt.Errorf("no field found for key: %s", key) } +// AsConfigMap converts this into a map[string]interface{} adding sub-sections as single entries +// (as opposed to full keys containing a . (period)). +func (m FieldValueMap) AsConfigMap() (map[string]interface{}, error) { + rv := make(map[string]interface{}) + + for key, value := range m { + if !strings.Contains(key, ".") { + if v, has := rv[key]; has { + return nil, fmt.Errorf("error at key %q: should not already exist but has type %T", key, v) + } + rv[key] = value.Interface() + continue + } + parts := strings.Split(key, ".") + justKey := parts[len(parts)-1] + sections := parts[:len(parts)-1] + secMap := rv + for _, section := range sections { + if secM, has := secMap[section]; has { + secT, ok := secM.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("error at key %q at section %q: sub-section should have type map[string]interface{}, got %T", key, section, secM) + } + secMap = secT + continue + } + sec := make(map[string]interface{}) + secMap[section] = sec + secMap = sec + } + if v, has := secMap[key]; has { + return nil, fmt.Errorf("error at key %q: key %q should not already exist in sub-section but has type %T", key, justKey, v) + } + valueI := value.Interface() + secMap[justKey] = valueI + + // The telemetry.global-labels field in the app config struct is a `[][]string`. + // But serverconfig.GetConfig expects viper to return it as a `[]interface{}`. + // Then each element of that is expected to also be a `[]interface{}`. + // So we need to convert that field so that it's as expected. + // In the SDK's main branch, they fixed that function, so eventually we can remove this. + if key == "telemetry.global-labels" { + newv := make([]interface{}, 0) + if valueI != nil { + if gl, ok := valueI.([][]string); ok { + for _, p := range gl { + var newp []interface{} + for _, k := range p { + newp = append(newp, k) + } + newv = append(newv, newp) + } + } + } + secMap[justKey] = newv + } + } + + return rv, nil +} + // setValueFromString sets a value from the provided string. // The string is converted appropriately for the underlying value type. // Assuming the value came from MakeFieldValueMap, this will actually be updating the From bce463fa7dbb3b98b2df27c242626c080457d5db Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 15:30:38 -0700 Subject: [PATCH 07/14] [1216]: Create a DefaultAppConfig that applies our default min-gas-prices to the object and use that instead of serverconfig.DefaultConfig where needed. The result is that all the config stuff no longer thinks the default min-gas-prices is an empty string. Plus, if the app.toml doesn't exist, viper ends up with our default for min-gas-prices too. Also, use the new AsConfigMap for creating the config map in addFieldMapToViper. --- cmd/provenanced/config/manager.go | 42 ++++++++++++------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/cmd/provenanced/config/manager.go b/cmd/provenanced/config/manager.go index 3891b3464a..f56e91bcec 100644 --- a/cmd/provenanced/config/manager.go +++ b/cmd/provenanced/config/manager.go @@ -17,6 +17,8 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/server" serverconfig "github.com/cosmos/cosmos-sdk/server/config" + + "github.com/provenance-io/provenance/internal/pioconfig" ) // PackConfig generates and saves the packed config file then removes the individual config files. @@ -63,7 +65,7 @@ func FileExists(fullFilePath string) bool { // ExtractAppConfig creates an app/cosmos config from the command context. func ExtractAppConfig(cmd *cobra.Command) (*serverconfig.Config, error) { v := server.GetServerContextFromCmd(cmd).Viper - conf := serverconfig.DefaultConfig() + conf := DefaultAppConfig() if err := v.Unmarshal(conf); err != nil { return nil, fmt.Errorf("error extracting app config: %w", err) } @@ -141,11 +143,18 @@ func ExtractClientConfigAndMap(cmd *cobra.Command) (*ClientConfig, FieldValueMap return conf, fields, nil } +// DefaultAppConfig gets our default app config. +func DefaultAppConfig() *serverconfig.Config { + rv := serverconfig.DefaultConfig() + rv.MinGasPrices = pioconfig.GetProvenanceConfig().ProvenanceMinGasPrices + return rv +} + // GetAllConfigDefaults gets a field map from the defaults of all the configs. func GetAllConfigDefaults() FieldValueMap { rv := FieldValueMap{} rv.AddEntriesFrom( - MakeFieldValueMap(serverconfig.DefaultConfig(), false), + MakeFieldValueMap(DefaultAppConfig(), false), removeUndesirableTmConfigEntries(MakeFieldValueMap(tmconfig.DefaultConfig(), false)), MakeFieldValueMap(DefaultClientConfig(), false), ) @@ -419,7 +428,7 @@ func loadUnpackedConfig(cmd *cobra.Command) error { } // Load the app/cosmos config defaults, then file if it exists. - adErr := addFieldMapToViper(vpr, MakeFieldValueMap(serverconfig.DefaultConfig(), false)) + adErr := addFieldMapToViper(vpr, MakeFieldValueMap(DefaultAppConfig(), false)) if adErr != nil { return fmt.Errorf("app config defaults load error: %w", adErr) } @@ -477,7 +486,7 @@ func loadPackedConfig(cmd *cobra.Command) error { } // Start with the defaults - appConfigMap := MakeFieldValueMap(serverconfig.DefaultConfig(), false) + appConfigMap := MakeFieldValueMap(DefaultAppConfig(), false) tmConfigMap := MakeFieldValueMap(tmconfig.DefaultConfig(), false) clientConfigMap := MakeFieldValueMap(DefaultClientConfig(), false) @@ -533,28 +542,9 @@ func loadPackedConfig(cmd *cobra.Command) error { } func addFieldMapToViper(vpr *viper.Viper, fvmap FieldValueMap) error { - configMap := make(map[string]interface{}) - for k, v := range fvmap { - configMap[k] = v.Interface() - } - // The telemetry.global-labels field in the app config struct is a `[][]string`. - // But in serverconfig.GetConfig, it expects viper to return it as a `[]interface{}`. - // Then each element of that is expected to also be a `[]interface{}`. - // So we need to convert that field before adding it to viper. - if gli, hasGL := configMap["telemetry.global-labels"]; hasGL { - newv := make([]interface{}, 0) - if gli != nil { - if gl, ok := gli.([][]string); ok { - for _, p := range gl { - var newp []interface{} - for _, k := range p { - newp = append(newp, k) - } - newv = append(newv, newp) - } - } - } - configMap["telemetry.global-labels"] = newv + configMap, err := fvmap.AsConfigMap() + if err != nil { + return err } return vpr.MergeConfigMap(configMap) } From 931e47264e0d6cb46bbdfd71c66416e17b38708c Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 15:33:10 -0700 Subject: [PATCH 08/14] [1216]: Update the config tests (slightly) to account for the default min-gas-prices being what we've defined and add a test to the packed config that makes sure that default is being applied. --- cmd/provenanced/cmd/config_test.go | 34 ++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/cmd/provenanced/cmd/config_test.go b/cmd/provenanced/cmd/config_test.go index 7cd59f4555..fac1f41988 100644 --- a/cmd/provenanced/cmd/config_test.go +++ b/cmd/provenanced/cmd/config_test.go @@ -50,6 +50,8 @@ func (s *ConfigTestSuite) SetupTest() { s.Home = s.T().TempDir() s.T().Logf("%s Home: %s", s.T().Name(), s.Home) + pioconfig.SetProvenanceConfig("confcoin", 5) + encodingConfig := sdksim.MakeTestEncodingConfig() clientCtx := client.Context{}. WithCodec(encodingConfig.Codec). @@ -451,7 +453,7 @@ func (s *ConfigTestSuite) TestConfigChanged() { } expectedAppOutLines := []string{ s.makeAppDiffHeaderLines(), - fmt.Sprintf(`minimum-gas-prices="%s" (default="")`, pioconfig.GetProvenanceConfig().ProvenanceMinGasPrices), + allEqual("app"), "", } expectedTMOutLines := []string{ @@ -716,14 +718,14 @@ func (s *ConfigTestSuite) TestConfigSetMulti() { name: "two of each", args: []string{"set", "consensus.timeout_commit", "951ms", - "api.swagger", "true", + "api.enable", "false", "telemetry.service-name", "blocky2", "node", "tcp://localhost:26657", "output", "text", "log_format", "plain"}, out: s.makeMultiLine( s.makeAppConfigUpdateLines(), - s.makeKeyUpdatedLine("api.swagger", "false", "true"), + s.makeKeyUpdatedLine("api.enable", "true", "false"), s.makeKeyUpdatedLine("telemetry.service-name", `"blocky"`, `"blocky2"`), "", s.makeTMConfigUpdateLines(), @@ -754,9 +756,7 @@ func (s *ConfigTestSuite) TestConfigSetMulti() { func (s *ConfigTestSuite) TestPackUnpack() { s.T().Run("pack", func(t *testing.T) { - expectedPacked := map[string]string{ - "minimum-gas-prices": "1905nhash", - } + expectedPacked := map[string]string{} expectedPackedJSON, jerr := json.MarshalIndent(expectedPacked, "", " ") require.NoError(t, jerr, "making expected json") expectedPackedJSONStr := string(expectedPackedJSON) @@ -812,6 +812,28 @@ func (s *ConfigTestSuite) TestPackUnpack() { }) } +func (s *ConfigTestSuite) TestEmptyPackedConfigHasDefaultMinGas() { + expected := provconfig.DefaultAppConfig().MinGasPrices + s.Require().NotEqual("", expected, "default MinGasPrices") + + // Pack the config, then rewrite the file to be an empty object. + pcmd := s.getConfigCmd() + pcmd.SetArgs([]string{"pack"}) + err := pcmd.Execute() + s.Require().NoError(err, "pack the config") + s.Require().NoError(os.WriteFile(provconfig.GetFullPathToPackedConf(pcmd), []byte("{}"), 0o644), "writing empty packed config") + + // Now read the config and check that the min gas prices are the default that we want. + ncmd := s.getConfigCmd() + err = provconfig.LoadConfigFromFiles(ncmd) + s.Require().NoError(err, "LoadConfigFromFiles") + + appConfig, err := provconfig.ExtractAppConfig(ncmd) + s.Require().NoError(err, "ExtractAppConfig") + actual := appConfig.MinGasPrices + s.Assert().Equal(expected, actual, "MinGasPrices") +} + func (s *ConfigTestSuite) TestUpdate() { // Write a dumb version of each file that is missing almost everything, but has an easily identifiable comment. customComment := "# There's no way this is comment is in the real config." From ec6ddc643a46f5bb759296c28282935a7703815e Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 15:36:08 -0700 Subject: [PATCH 09/14] [1216]: In the manager tests, switch to DefaultAppConfig as needed and add a canary for the global-labels special handling. --- cmd/provenanced/config/manager_test.go | 72 +++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 7 deletions(-) diff --git a/cmd/provenanced/config/manager_test.go b/cmd/provenanced/config/manager_test.go index b040fd5754..8ca73433ab 100644 --- a/cmd/provenanced/config/manager_test.go +++ b/cmd/provenanced/config/manager_test.go @@ -104,7 +104,7 @@ func (s *ConfigManagerTestSuite) TestManagerWriteAppConfigWithIndexEventsThenRea func (s *ConfigManagerTestSuite) TestPackedConfigCosmosLoadDefaults() { dCmd := s.makeDummyCmd() - appConfig := serverconfig.DefaultConfig() + appConfig := DefaultAppConfig() tmConfig := tmconfig.DefaultConfig() clientConfig := DefaultClientConfig() generateAndWritePackedConfig(dCmd, appConfig, tmConfig, clientConfig, false) @@ -132,11 +132,13 @@ func (s *ConfigManagerTestSuite) TestPackedConfigCosmosLoadGlobalLabels() { ctx := client.GetClientContextFromCmd(dCmd) vpr := ctx.Viper + var appConfig2 serverconfig.Config + var err error s.Require().NotPanics(func() { - appConfig2, err := serverconfig.GetConfig(vpr) - s.Require().NoError(err, "GetConfig") - s.Assert().Equal(appConfig.Telemetry.GlobalLabels, appConfig2.Telemetry.GlobalLabels) - }) + appConfig2, err = serverconfig.GetConfig(vpr) + }, "GetConfig") + s.Require().NoError(err, "GetConfig") + s.Assert().Equal(appConfig.Telemetry.GlobalLabels, appConfig2.Telemetry.GlobalLabels) } func (s *ConfigManagerTestSuite) TestUnmanagedConfig() { @@ -170,7 +172,7 @@ func (s *ConfigManagerTestSuite) TestUnmanagedConfig() { s.T().Run("unmanaged config is read with unpacked files", func(t *testing.T) { dCmd := s.makeDummyCmd() uFile := GetFullPathToUnmanagedConf(dCmd) - SaveConfigs(dCmd, serverconfig.DefaultConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) + SaveConfigs(dCmd, DefaultAppConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) require.NoError(t, os.WriteFile(uFile, []byte("my-custom-entry = \"stuff\"\n"), 0o644), "writing unmanaged config") require.NoError(t, LoadConfigFromFiles(dCmd)) ctx := client.GetClientContextFromCmd(dCmd) @@ -182,7 +184,7 @@ func (s *ConfigManagerTestSuite) TestUnmanagedConfig() { s.T().Run("unmanaged config is read with packed config", func(t *testing.T) { dCmd := s.makeDummyCmd() uFile := GetFullPathToUnmanagedConf(dCmd) - SaveConfigs(dCmd, serverconfig.DefaultConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) + SaveConfigs(dCmd, DefaultAppConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) require.NoError(t, PackConfig(dCmd), "packing config") require.NoError(t, os.WriteFile(uFile, []byte("other-custom-entry = 8\n"), 0o644), "writing unmanaged config") require.NoError(t, LoadConfigFromFiles(dCmd)) @@ -192,3 +194,59 @@ func (s *ConfigManagerTestSuite) TestUnmanagedConfig() { assert.Equal(t, 8, actual, "unmanaged field value") }) } + +func (s *ConfigManagerTestSuite) TestServerGetConfigGlobalLabelsCanary() { + // This test checks to see if the special handling of the telemetry.global-labels + // field in the config map is still needed. + // + // As of writing this, the serverconfig.GetConfig function requires viper to return it as a []interface{} + // with each element itself being a []interface{}. The main branch of the sdk has been updated to not + // care though, so eventually we can clean up our stuff. + // + // If this test fails, remove the s.T().Skip() line from TestServerGetConfigGlobalLabels and run that. + globalLabels := [][]string{ + {"keya", "valuea"}, + {"keyb", "valueb"}, + {"keyc", "valuec"}, + } + telemetry := map[string]interface{}{ + "global-labels": globalLabels, + } + cfgMap := map[string]interface{}{ + "telemetry": telemetry, + } + + vpr := viper.New() + s.Require().NoError(vpr.MergeConfigMap(cfgMap), "MergeConfigMap") + expErr := "failed to parse global-labels config" + _, err := serverconfig.GetConfig(vpr) + s.Require().ErrorContains(err, expErr, "GetConfig") +} + +func (s *ConfigManagerTestSuite) TestServerGetConfigGlobalLabels() { + // If TestServerGetConfigGlobalLabels fails, remove the s.T().Skip line and run this test. + // If this then passes: + // 1. Remove the now-unneeded special handling of telemetry.global-labels. + // 2. Delete the TestServerGetConfigGlobalLabelsCanary test. + // 3. Remove this whole set of comments. + // + // As of writing this, that special handling is in reflector.go: FieldValueMap.AsConfigMap. + // Note that the special handling in setValueFromString is probably still needed though, so leave that. + s.T().Skip("This cannot pass until TestServerGetConfigCanary fails.") + globalLabels := [][]string{ + {"keya", "valuea"}, + {"keyb", "valueb"}, + {"keyc", "valuec"}, + } + telemetry := map[string]interface{}{ + "global-labels": globalLabels, + } + cfgMap := map[string]interface{}{ + "telemetry": telemetry, + } + + vpr := viper.New() + s.Require().NoError(vpr.MergeConfigMap(cfgMap), "MergeConfigMap") + _, err := serverconfig.GetConfig(vpr) + s.Require().NoError(err, "GetConfig") +} From e4b457cd4a50f946ef384ca504b402110d4b1458 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 16:11:05 -0700 Subject: [PATCH 10/14] [1216]: Add some tests about the min-gas-prices being. --- cmd/provenanced/config/manager_test.go | 118 +++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/cmd/provenanced/config/manager_test.go b/cmd/provenanced/config/manager_test.go index 8ca73433ab..380ff0d7b6 100644 --- a/cmd/provenanced/config/manager_test.go +++ b/cmd/provenanced/config/manager_test.go @@ -20,6 +20,8 @@ import ( tmconfig "github.com/tendermint/tendermint/config" "github.com/tendermint/tendermint/libs/log" + + "github.com/provenance-io/provenance/internal/pioconfig" ) type ConfigManagerTestSuite struct { @@ -250,3 +252,119 @@ func (s *ConfigManagerTestSuite) TestServerGetConfigGlobalLabels() { _, err := serverconfig.GetConfig(vpr) s.Require().NoError(err, "GetConfig") } + +func (s *ConfigManagerTestSuite) TestConfigMinGasPrices() { + configDir := GetFullPathToConfigDir(s.makeDummyCmd()) + s.Require().NoError(os.MkdirAll(configDir, 0o755), "making config dir") + + pioconfig.SetProvenanceConfig("manager", 42) + defaultMinGasPrices := pioconfig.GetProvenanceConfig().ProvenanceMinGasPrices + s.Require().NotEqual("", defaultMinGasPrices, "ProvenanceMinGasPrices") + + s.Run("DefaultAppConfig has MinGasPrices", func() { + cfg := DefaultAppConfig() + actual := cfg.MinGasPrices + s.Assert().Equal(defaultMinGasPrices, actual, "MinGasPrices") + }) + + s.Run("no files", func() { + cmd := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal(defaultMinGasPrices, actual, "MinGasPrices") + }) + + s.Run("tm and client files but no app file", func() { + cmd1 := s.makeDummyCmd() + SaveConfigs(cmd1, nil, tmconfig.DefaultConfig(), DefaultClientConfig(), false) + appCfgFile := GetFullPathToAppConf(cmd1) + _, err := os.Stat(appCfgFile) + fileExists := !os.IsNotExist(err) + s.Require().False(fileExists, "file exists: %s", appCfgFile) + + cmd2 := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd2), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd2) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal(defaultMinGasPrices, actual, "MinGasPrices") + }) + + s.Run("all files exist min gas price empty", func() { + cmd1 := s.makeDummyCmd() + appCfg := DefaultAppConfig() + appCfg.MinGasPrices = "" + SaveConfigs(cmd1, appCfg, tmconfig.DefaultConfig(), DefaultClientConfig(), false) + appCfgFile := GetFullPathToAppConf(cmd1) + _, err := os.Stat(appCfgFile) + fileExists := !os.IsNotExist(err) + s.Require().True(fileExists, "file exists: %s", appCfgFile) + + cmd2 := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd2), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd2) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal("", actual, "MinGasPrices") + }) + + s.Run("all files exist min gas price something else", func() { + cmd1 := s.makeDummyCmd() + appCfg := DefaultAppConfig() + appCfg.MinGasPrices = "something else" + SaveConfigs(cmd1, appCfg, tmconfig.DefaultConfig(), DefaultClientConfig(), false) + appCfgFile := GetFullPathToAppConf(cmd1) + _, err := os.Stat(appCfgFile) + fileExists := !os.IsNotExist(err) + s.Require().True(fileExists, "file exists: %s", appCfgFile) + + cmd2 := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd2), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd2) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal("something else", actual, "MinGasPrices") + }) + + s.Run("packed config without min-gas-prices", func() { + cmd1 := s.makeDummyCmd() + SaveConfigs(cmd1, DefaultAppConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) + s.Require().NoError(PackConfig(cmd1), "PackConfig") + packedCfgFile := GetFullPathToPackedConf(cmd1) + _, err := os.Stat(packedCfgFile) + fileExists := !os.IsNotExist(err) + s.Require().True(fileExists, "file exists: %s", packedCfgFile) + + // Just to be sure, rewrite the file as just "{}". + s.Require().NoError(os.WriteFile(packedCfgFile, []byte("{}"), 0o644), "writing packed config") + + cmd2 := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd2), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd2) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal(defaultMinGasPrices, actual) + }) + + s.Run("packed config with min-gas-prices", func() { + cmd1 := s.makeDummyCmd() + SaveConfigs(cmd1, DefaultAppConfig(), tmconfig.DefaultConfig(), DefaultClientConfig(), false) + s.Require().NoError(PackConfig(cmd1), "PackConfig") + packedCfgFile := GetFullPathToPackedConf(cmd1) + _, err := os.Stat(packedCfgFile) + fileExists := !os.IsNotExist(err) + s.Require().True(fileExists, "file exists: %s", packedCfgFile) + + // rewrite the packed file to include min-gas-prices + s.Require().NoError(os.WriteFile(packedCfgFile, []byte(`{"minimum-gas-prices":"65blue"}`), 0o644), "writing packed config") + + cmd2 := s.makeDummyCmd() + s.Require().NoError(LoadConfigFromFiles(cmd2), "LoadConfigFromFiles") + cfg, err := ExtractAppConfig(cmd2) + s.Require().NoError(err, "ExtractAppConfig") + actual := cfg.MinGasPrices + s.Assert().Equal("65blue", actual) + }) +} From d65be244d7253f0131c25b6ebd8f1413b86b93bc Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 16:14:09 -0700 Subject: [PATCH 11/14] [1216]: Undo a small change to one of the tests that isn't actually needed (was just for troubleshooting). --- cmd/provenanced/cmd/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/provenanced/cmd/config_test.go b/cmd/provenanced/cmd/config_test.go index fac1f41988..944bb82e38 100644 --- a/cmd/provenanced/cmd/config_test.go +++ b/cmd/provenanced/cmd/config_test.go @@ -718,14 +718,14 @@ func (s *ConfigTestSuite) TestConfigSetMulti() { name: "two of each", args: []string{"set", "consensus.timeout_commit", "951ms", - "api.enable", "false", + "api.swagger", "true", "telemetry.service-name", "blocky2", "node", "tcp://localhost:26657", "output", "text", "log_format", "plain"}, out: s.makeMultiLine( s.makeAppConfigUpdateLines(), - s.makeKeyUpdatedLine("api.enable", "true", "false"), + s.makeKeyUpdatedLine("api.swagger", "false", "true"), s.makeKeyUpdatedLine("telemetry.service-name", `"blocky"`, `"blocky2"`), "", s.makeTMConfigUpdateLines(), From 641437f3491a52ecef47b1abf1ae7aeb2196fa25 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 16:18:00 -0700 Subject: [PATCH 12/14] [1216]: Undo another change in the tests that was just from troubleshooting. --- cmd/provenanced/config/manager_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/provenanced/config/manager_test.go b/cmd/provenanced/config/manager_test.go index 380ff0d7b6..b68598e58a 100644 --- a/cmd/provenanced/config/manager_test.go +++ b/cmd/provenanced/config/manager_test.go @@ -134,13 +134,11 @@ func (s *ConfigManagerTestSuite) TestPackedConfigCosmosLoadGlobalLabels() { ctx := client.GetClientContextFromCmd(dCmd) vpr := ctx.Viper - var appConfig2 serverconfig.Config - var err error s.Require().NotPanics(func() { - appConfig2, err = serverconfig.GetConfig(vpr) + appConfig2, err := serverconfig.GetConfig(vpr) + s.Require().NoError(err, "GetConfig") + s.Assert().Equal(appConfig.Telemetry.GlobalLabels, appConfig2.Telemetry.GlobalLabels) }, "GetConfig") - s.Require().NoError(err, "GetConfig") - s.Assert().Equal(appConfig.Telemetry.GlobalLabels, appConfig2.Telemetry.GlobalLabels) } func (s *ConfigManagerTestSuite) TestUnmanagedConfig() { From c7931399b9e5a9b1625c95d5db1594b812393597 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 17:00:01 -0700 Subject: [PATCH 13/14] [1216]: Add some unit tests on the AsConfigMap function. --- cmd/provenanced/config/reflector_test.go | 107 +++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/cmd/provenanced/config/reflector_test.go b/cmd/provenanced/config/reflector_test.go index 36b4041b3f..cac294795d 100644 --- a/cmd/provenanced/config/reflector_test.go +++ b/cmd/provenanced/config/reflector_test.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "reflect" "testing" @@ -326,3 +327,109 @@ func (s *ReflectorTestSuit) TestGetFieldValueMapSubFields() { } })) } + +func (s *ReflectorTestSuit) TestAsConfigMap() { + // Note: Because the AsConfigMap function loops over a map, + // for the ones expecting errors, there's no telling which error it will hit. + // I.e. we don't know if it ends up adding the section first or the key first. + // So we can only make sure that the error is one of a possible set. + tests := []struct { + name string + fvm FieldValueMap + exp map[string]interface{} + errOneOf []string + }{ + { + name: "empty", + fvm: FieldValueMap{}, + exp: make(map[string]interface{}), + }, + { + name: "two keys", + fvm: FieldValueMap{ + "thing1": reflect.ValueOf("value of thing 1"), + "thing2": reflect.ValueOf(99), + }, + exp: map[string]interface{}{ + "thing1": "value of thing 1", + "thing2": 99, + }, + }, + { + name: "some deep things", + fvm: FieldValueMap{ + "rootthing": reflect.ValueOf("root thing"), + "sub1.sub2.thing1": reflect.ValueOf("I am thing 1"), + "sub1.nothing": reflect.ValueOf("do I exist?"), + "sub1.sub2.thing2": reflect.ValueOf(2), + "sub1.sub3.thing3": reflect.ValueOf([]string{"a slice!"}), + }, + exp: map[string]interface{}{ + "rootthing": "root thing", + "sub1": map[string]interface{}{ + "nothing": "do I exist?", + "sub2": map[string]interface{}{ + "thing1": "I am thing 1", + "thing2": 2, + }, + "sub3": map[string]interface{}{ + "thing3": []string{"a slice!"}, + }, + }, + }, + }, + { + name: "reused section name", + fvm: FieldValueMap{ + "mysect": reflect.ValueOf("oops"), + "mysect.value": reflect.ValueOf("a deeper value"), + }, + errOneOf: []string{ + `error at key "mysect": should not already exist but has type map[string]interface {}`, + `error at key "mysect.value" at section "mysect": sub-section should have type map[string]interface{}, got string`, + }, + }, + { + name: "deep reused section name", + fvm: FieldValueMap{ + "rootthing": reflect.ValueOf("a root thing"), + "sub1.sub2.value1": reflect.ValueOf("value 1"), + "sub1.sub2": reflect.ValueOf("sub 2 oops"), + }, + errOneOf: []string{ + `error at key "sub1.sub2.value1" at section "sub2": sub-section should have type map[string]interface{}, got string`, + `error at key "sub1.sub2": key "sub2" should not already exist in sub-section but has type map[string]interface {}`, + }, + }, + } + + for _, tc := range tests { + s.Run(tc.name, func() { + actual, err := tc.fvm.AsConfigMap() + if len(tc.errOneOf) > 0 { + if s.Assert().Error(err, "AsConfigMap error: actual = %+v", actual) { + s.Assert().Contains(tc.errOneOf, err.Error(), "AsConfigMap error") + } + } else { + if s.Assert().NoError(err, "AsConfigMap error") { + s.Assert().Equal(tc.exp, actual, "AsConfigMap result") + } + } + }) + } +} + +func (s *ReflectorTestSuit) TestAsConfigMapLotsMoreTimes() { + // Because of the non-deterministic nature of map loops, there's some + // uncertainty in what happens in AsConfigMap (i.e. which error gets triggered). + // To be more certain that it's all good, run it 1,000 more times. + // Hopefully in that time, the map looping order changes and things happen + // differently, but the tests still pass. + + for i := 1; i <= 1000; i++ { + s.Run(fmt.Sprintf("%04d", i), s.TestAsConfigMap) + if s.T().Failed() { + break + } + } +} From 222d89c85ac66445b7d5f6faca3a9f8ccbf43ad2 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Fri, 18 Nov 2022 17:09:15 -0700 Subject: [PATCH 14/14] [1216]: Fix AsConfigMap subsection dup check. --- cmd/provenanced/config/reflector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/provenanced/config/reflector.go b/cmd/provenanced/config/reflector.go index ff57f13f54..541c1b1114 100644 --- a/cmd/provenanced/config/reflector.go +++ b/cmd/provenanced/config/reflector.go @@ -253,7 +253,7 @@ func (m FieldValueMap) AsConfigMap() (map[string]interface{}, error) { secMap[section] = sec secMap = sec } - if v, has := secMap[key]; has { + if v, has := secMap[justKey]; has { return nil, fmt.Errorf("error at key %q: key %q should not already exist in sub-section but has type %T", key, justKey, v) } valueI := value.Interface()