Add client config #8820

cyberbono3 · 2021-03-09T04:38:13Z

Description

This is draft PR that implements Step1 from #8529

Step 1 Creating a new $NODE_DIR/config/client.toml file to hold all client-side configuration (e.g. keyring-backend, node, chain-id etc...).

Questions:

Do i need to include all flags into client config?
Do we want print default value if key is invalid in case of get flag is set here?
Do I need to check if our key matches one of these values?

@AmauryM Could you leave your feedback, please?

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

client/config/config.go

cyberbono3 · 2021-03-10T19:13:30Z

client/config/config.go

@@ -92,220 +48,104 @@ func Cmd(defaultCLIHome string) *cobra.Command {

 	cmd.Flags().String(flags.FlagHome, defaultCLIHome,
 		"set client's home directory for configuration")
-	cmd.Flags().Bool(flagGet, false,
-		"print configuration value or its default if unset")
+	//	cmd.Flags().Bool(flagGet, false,


I think we dont need a flagGet flag here, do we?

that's correct! as per Alessio's proposal #8820 (comment)

cyberbono3 · 2021-03-10T19:16:01Z

client/config/config.go

 	if err != nil {
-		return fmt.Errorf("Unable to get client config %v", err)
+		fmt.Fprintf(os.Stderr, "Unable to get client config: %v\n", err)
+		return err


@alessio Following your suggestion, I print an error here to os.Stderr. I am unable to add os.Exit(1) cause the runConfigCmd is supposed to return an error.

client/config/config.go

client/config/toml.go

server/util.go

cyberbono3

I need more context on adding testing of priority env variable, flag and file.

Additionally, I have added my config subcommand here to rootCmd. I hope it makes sence.

server/util.go

amaury1093 · 2021-03-11T14:03:28Z

client/config/config.go

@@ -92,220 +48,104 @@ func Cmd(defaultCLIHome string) *cobra.Command {

 	cmd.Flags().String(flags.FlagHome, defaultCLIHome,
 		"set client's home directory for configuration")
-	cmd.Flags().Bool(flagGet, false,
-		"print configuration value or its default if unset")
+	//	cmd.Flags().Bool(flagGet, false,


that's correct! as per Alessio's proposal #8820 (comment)

client/config/config.go

client/config/toml.go

client/config/config.go

cyberbono3 · 2021-03-11T21:15:55Z

To improve code readability I would like to move all cmd logic to new cmd.go file . In config.go I leave only logic associated with client config. Does it sound good?

cyberbono3 · 2021-03-11T21:25:40Z

Another suggestion would be to implement separate function or 2 functions to handle get and set cases in runConfigCmd.

cyberbono3 · 2021-03-12T18:24:06Z

I tried to test my code . I removed everything from ~/.simapp and ran this shell script. Unfortunately, client.toml has not been created. Is it proper way to test it or there is another way to do it(e.g. integration testing)?

alessio · 2021-03-12T18:39:30Z

On Fri, Mar 12, 2021 at 6:33 PM Andrei Ivasko ***@***.***> wrote: I tried to test my code . I removed everything from ~/.simapp and ran this shell script. Unfortunately, client.toml has not been created. Is it proper way to test it or there is another way to do it(e.g. integration testing)?

The old `config` command used to create the `toml` file upon execution if the file had not yet been created.

…

-- Alessio Treglia VicePresident of Engineering at Tendermint Cosmonaut at Cosmos @tendermint_team · @cosmos · We're hiring!

…tConfigs

cyberbono3 · 2021-03-15T03:55:24Z

./build/simd init moniker --chain-id sss
I am able to write the client config into client.toml. Thanks @clevinson for help.

client/config/config.go

cyberbono3 · 2021-03-16T03:42:33Z

@alessio I have question regarding interceptConfigs. I use viper field rootViper from serverCtx here . This is my code associated with my client config inserted into interceptConfigs. Does it make sense to decouple server config and client config logic? Should I use different Viper fields for server and client config?

cyberbono3 · 2021-03-16T03:53:58Z

@alessio @AmauryM mentioned here:

Step 2 (for 0.43+). Think about refactoring config as a whole, with the objectives in mind to: remove dependency on singletons make sure we have the exact same configurable variables via flags, ENV variables and config file, and define a priority between them when multiple are set

I would like to start working on Step 2 (second PR) . I need more context regarding it.

which dependency on singleton should be removed?
How we set priorities between ENV variables, flags and config file?

alexanderbez · 2021-03-16T14:17:53Z

How we set priorities between ENV variables, flags and config file?

flags > env vars > config file, this is typically the standard.

amaury1093 · 2021-03-16T15:44:35Z

which dependency on singleton should be removed?

Example:

cosmos-sdk/x/auth/client/cli/tx_multisign.go

Line 356 in 5f71e93

if viper.GetBool(flagNoAutoIncrement) {

. There might be others.

alessio · 2021-03-16T16:26:22Z

How we set priorities between ENV variables, flags and config file?

flags > env vars > config file, this is typically the standard.

Mmm... I think flags should override env vars, and both override config files. Is that correct?

client/config/config.go

cyberbono3 · 2021-03-18T00:11:31Z

Priority rule flags > env vars > config file

I have performed some manual tests. I hope I have performed them right. Otherwise, please advice.

This is my initial client config
./build/simd config
{ "chain-id": "", "keyring-backend": "os", "output": "text", "node": "tcp://localhost:26657", "broadcast-mode": "sync" }
Manual tests:

env variable and config

NODE=tcp://localhost:26658
./build/simd q bank total
Error: post failed: Post "http://localhost:26657": dial tcp 127.0.0.1:26657: connect: connection refused

Conclusion: Test does not pass, it should dial tcp://localhost:26658 cause env variable has higher priority than config

flag and config

./build/simd q bank total --node tcp://localhost:26658
Error: post failed: Post "http://localhost:26658": dial tcp 127.0.0.1:26658: connect: connection refused

Conclusion : test pass cause flag has the highest priority

flag, env var and config

NODE=tcp://localhost:26658
./build/simd q bank total --node tcp://localhost:26659
Error: post failed: Post "http://localhost:26659": dial tcp 127.0.0.1:26659: connect: connection refused

Conclusion: test pass cause flag has the highest priority

change config node to tcp://localhost:26500 and call ./build/simd q bank total

./build/simd config node tcp://localhost:26500
./build/simd config node
tcp://localhost:26500

./build/simd q bank total
Error: post failed: Post "http://localhost:26657": dial tcp 127.0.0.1:26657: connect: connection refused

Conclusion: test does not pass cause it should dial the node from config file tcp://localhost:26500
right?

cyberbono3 · 2021-03-18T00:18:57Z

simapp/simd/cmd/root.go

 	rootCmd := &cobra.Command{
 		Use:   "simd",
 		Short: "simulation app",
 		PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
+


I have also tried to put clicfg.UpdateClientContextFromClientConfig(initClientCtx) inside PersistentPreRunE. In this case I can handle an error occured by reading from client.toml file. I have got the same test results.

Let's put inside PersistentPreRunE then.

The docs say that this function:

PersistentPreRun: children of this command will inherit and execute.

So I guess it's what we want.

cyberbono3 · 2021-03-18T01:19:37Z

It seems, Vyper does not read env variables from linux terminal. It has AutomaticEnv() function which can read env variables from the config file only. Alternatively, os.GetEnv(varname) can be used.

amaury1093

The manual tests you performed looks good to me.

I think with the UpdateClientContextFromClientConfig function, we're getting close! Thanks @cyberbono3

Inside that function, imo we also need to add a logic saying:

make sure the config path & file exist
if the config file doesn't exist, create a new one and populate it with default values

I can have a look at the viper reading from terminal env vars issue.

client/config/config.go

amaury1093 · 2021-03-18T10:25:27Z

home/config/app.toml

@@ -0,0 +1,189 @@
+# This is a TOML config file.


wrong committed file?

amaury1093 · 2021-03-18T10:28:35Z

simapp/simd/cmd/root.go

 	rootCmd := &cobra.Command{
 		Use:   "simd",
 		Short: "simulation app",
 		PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
+


Let's put inside PersistentPreRunE then.

The docs say that this function:

PersistentPreRun: children of this command will inherit and execute.

So I guess it's what we want.

amaury1093 · 2021-03-18T10:30:06Z

client/context.go

@@ -323,7 +334,7 @@ func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddres
 	return info.GetAddress(), info.GetName(), info.GetType(), nil
 }

-func newKeyringFromFlags(ctx Context, backend string) (keyring.Keyring, error) {
+func NewKeyringFromFlags(ctx Context, backend string) (keyring.Keyring, error) {


I believe NewKeyringFromString or NewKeyringFromBackend are both more correct names, and can be used in the toml file logic without sounding confusing.

client/config/config.go

cyberbono3 · 2021-03-18T23:55:08Z

Manual testing 2

0../build/simd init andrei
Outcome: client.toml created inside ~/.simapp/config

./build/simd config node http://localhost:2666
Outcome: http://localhost:2666
'./build/simd q bank total DOES NOT PASS
Expected outcome: can not connect to http://localhost:2666
Actual outcome: Error: post failed: Post "http://localhost:26657": dial tcp 127.0.0.1:26657: connect: connection refused
?
./build/simd q bank total --node http://localhost:2555
Outcome: cannot connect to "http://localhost:2555"
remove whole directory ~/.simapp and then run simd init moniker
Outcome client.toml is created ~/.simapp/config in with default values
DONE
PASS

TODO
Perform these steps for another field from client.toml
let do keyring-backend (will not be in q bank total)

cyberbono3 · 2021-03-19T04:33:15Z

client/config/cmd.go

-	if err := ensureConfigPath(configPath); err != nil {
-		return fmt.Errorf("couldn't make client config: %v", err)
-	}
+	/*


I think we dont need ensureConfigPath anymore, cause ~/.simapp/config directory will exist in any way due to ReadFromClientConfig logic

client/config/toml.go

cyberbono3 · 2021-03-19T04:41:52Z

client/context.go

+cd test/config there is no client.toml configuration file
+*/
+
+func (ctx Context) WithHomeFlag(cmd *cobra.Command) Context {


There is a bug by executing this command ./build/simd init andrei --home ./test. There is no client.toml created inside ./test/config. WithHomeFlag fixes this bug.

cyberbono3 · 2021-03-19T04:42:51Z

simapp/simd/cmd/root.go

@@ -56,11 +55,12 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
 		Short: "simulation app",
 		PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {

+			initClientCtx = initClientCtx.WithHomeFlag(cmd)


Should I move WithHomeFlag(cmd) inside config.ReadFromClientConfig to make our code a little bit cleaner?

tac0turtle · 2021-03-29T16:29:06Z

super seeded by #8953

RiccardoM mentioned this pull request Mar 9, 2021

Missing configuration support desmos-labs/desmos#353

Closed

add client config draft

aecceec

cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from 0071d76 to aecceec Compare March 9, 2021 17:06

alessio reviewed Mar 9, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

my current state

536ec0d

cyberbono3 commented Mar 10, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 10, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 10, 2021

View reviewed changes

client/config/toml.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 10, 2021

View reviewed changes

client/config/toml.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 10, 2021

View reviewed changes

client/config/toml.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 10, 2021

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

refactored code,feedback required

8dbc688

cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from af6f2a1 to 8dbc688 Compare March 10, 2021 19:38

cyberbono3 commented Mar 10, 2021

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

amaury1093 reviewed Mar 11, 2021

View reviewed changes

cyberbono3 added 4 commits March 11, 2021 11:39

use json to print out all fields of ClientConfig struct

b7c9376

introduce default constant values for ClientConfig

d0a3197

removed panic from WriteConfigFile and InitConfigTemplate

db07ea2

implemented setters for ClientConfig

7a1f9a8

cyberbono3 commented Mar 11, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

modified root.go to add clicfg.Cmd to rootCmd and util.go in intercep…

20f5f98

…tConfigs

able to write config to client.toml

f4f6dde

cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from f9f22da to f4f6dde Compare March 15, 2021 03:58

alessio reviewed Mar 15, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

alessio reviewed Mar 15, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

alessio reviewed Mar 15, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

alessio reviewed Mar 15, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

alessio reviewed Mar 15, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

refactored, config subcommand works

560a740

UpdateClientContextFromClientConfig

a807879

cyberbono3 commented Mar 17, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 17, 2021

View reviewed changes

client/config/config.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 18, 2021

View reviewed changes

amaury1093 reviewed Mar 18, 2021

View reviewed changes

ReadFromClientConfig

8125d5e

cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from f49f07a to 8125d5e Compare March 18, 2021 22:29

homeflag bug fixed

ec04cac

cyberbono3 commented Mar 19, 2021

View reviewed changes

client/config/toml.go Outdated Show resolved Hide resolved

cyberbono3 commented Mar 19, 2021

View reviewed changes

current state

d2250f6

cyberbono3 mentioned this pull request Mar 22, 2021

Add client config subcommand to CLI #8953

Merged

9 tasks

tac0turtle closed this Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client config #8820

Add client config #8820

cyberbono3 commented Mar 9, 2021 •

edited

Loading

cyberbono3 Mar 10, 2021

amaury1093 Mar 11, 2021

cyberbono3 Mar 10, 2021

cyberbono3 left a comment

amaury1093 Mar 11, 2021

cyberbono3 commented Mar 11, 2021

cyberbono3 commented Mar 11, 2021

cyberbono3 commented Mar 12, 2021

alessio commented Mar 12, 2021 via email

cyberbono3 commented Mar 15, 2021

cyberbono3 commented Mar 16, 2021 •

edited

Loading

cyberbono3 commented Mar 16, 2021

alexanderbez commented Mar 16, 2021 •

edited

Loading

amaury1093 commented Mar 16, 2021

alessio commented Mar 16, 2021

cyberbono3 commented Mar 18, 2021 •

edited

Loading

cyberbono3 Mar 18, 2021 •

edited

Loading

amaury1093 Mar 18, 2021

cyberbono3 commented Mar 18, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading

amaury1093 Mar 18, 2021

amaury1093 Mar 18, 2021

amaury1093 Mar 18, 2021

cyberbono3 commented Mar 18, 2021 •

edited

Loading

cyberbono3 Mar 19, 2021

cyberbono3 Mar 19, 2021

cyberbono3 Mar 19, 2021

tac0turtle commented Mar 29, 2021

Add client config #8820

Add client config #8820

Conversation

cyberbono3 commented Mar 9, 2021 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 commented Mar 11, 2021

cyberbono3 commented Mar 11, 2021

cyberbono3 commented Mar 12, 2021

alessio commented Mar 12, 2021 via email

cyberbono3 commented Mar 15, 2021

cyberbono3 commented Mar 16, 2021 • edited Loading

cyberbono3 commented Mar 16, 2021

alexanderbez commented Mar 16, 2021 • edited Loading

amaury1093 commented Mar 16, 2021

alessio commented Mar 16, 2021

cyberbono3 commented Mar 18, 2021 • edited Loading

cyberbono3 Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 commented Mar 18, 2021 • edited Loading

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 commented Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Mar 29, 2021

cyberbono3 commented Mar 9, 2021 •

edited

Loading

cyberbono3 commented Mar 16, 2021 •

edited

Loading

alexanderbez commented Mar 16, 2021 •

edited

Loading

cyberbono3 commented Mar 18, 2021 •

edited

Loading

cyberbono3 Mar 18, 2021 •

edited

Loading

cyberbono3 commented Mar 18, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading

cyberbono3 commented Mar 18, 2021 •

edited

Loading