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

Add client config subcommand to CLI #8953

Merged
merged 9 commits into from
Mar 31, 2021

Conversation

cyberbono3
Copy link
Contributor

@cyberbono3 cyberbono3 commented Mar 22, 2021

Description

This PR implements the first step from Amaury's proposal.

  • creates a new $NODE_DIR/config/client.toml file to hold all client-side configuration (chainID, keyringBackend, output, node, broadcastMode)
  • provides set functionality (e.g. ./build/simd config node tcp://localhost:127)
  • provides get functionality ((e.g. ./build/simd config node returns tcp://localhost:127)
  • prints out current client configuration in JSON format (./build/simd config)
    Output: { "chain-id": "chain-7e7zGq","keyring-backend": "test", "output": "json","node": "tcp://localhost:26657","broadcast-mode": "async"}

Me and @clevinson made manual testing. I can make some automatic tests if needed in a separate PR.

closes: #8529


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

@jackzampolin
Copy link
Member

This is important work @cyberbono3

client/config/cmd.go Outdated Show resolved Hide resolved
client/config/cmd.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
Comment on lines 49 to 51
tmos.MustWriteFile(configFilePath, buffer.Bytes(), 0644)
return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

MustWriteFile is removed from future tendermint releases. I suggest adopting the following (and equivalent) form:

Suggested change
tmos.MustWriteFile(configFilePath, buffer.Bytes(), 0644)
return nil
return ioutil.WriteFile(configFilePath, buffer.Bytes(), 0644)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

client/config/toml.go Outdated Show resolved Hide resolved
client/config/toml.go Outdated Show resolved Hide resolved
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Functionally, it works well!

I tested:

1. `node` set in client.toml, no flag: uses node from client.toml
2. `node` set in client.toml, with `--node` flag: uses node from flag

Left some comments about the code.

client/config/config.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
client/config/toml.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
@cyberbono3 cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch 3 times, most recently from 1af4dee to ff6eeeb Compare March 23, 2021 20:56
client/config/cmd.go Outdated Show resolved Hide resolved
client/config/cmd.go Outdated Show resolved Hide resolved
client/config/toml.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
client/context.go Outdated Show resolved Hide resolved
client/utils.go Outdated Show resolved Hide resolved
client/context.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #8953 (4a5a2b2) into master (feed37d) will decrease coverage by 0.02%.
The diff coverage is 34.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8953      +/-   ##
==========================================
- Coverage   58.96%   58.94%   -0.03%     
==========================================
  Files         575      575              
  Lines       32159    32171      +12     
==========================================
- Hits        18963    18962       -1     
- Misses      10982    10995      +13     
  Partials     2214     2214              
Impacted Files Coverage Δ
client/cmd.go 56.77% <0.00%> (-0.49%) ⬇️
client/utils.go 41.93% <0.00%> (-1.40%) ⬇️
server/rosetta/converter.go 54.44% <ø> (ø)
x/auth/client/tx.go 36.06% <0.00%> (-3.23%) ⬇️
client/context.go 55.85% <40.00%> (-1.16%) ⬇️
simapp/simd/cmd/root.go 61.71% <71.42%> (+0.24%) ⬆️
client/keys/list.go 73.68% <100.00%> (-6.32%) ⬇️
testutil/network/network.go 90.00% <100.00%> (+0.04%) ⬆️
x/auth/client/cli/tx_sign.go 73.88% <100.00%> (-0.71%) ⬇️

@orijbot
Copy link

orijbot commented Mar 24, 2021

@cyberbono3 cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from a889d7e to f885b5d Compare March 24, 2021 21:05
@orijbot
Copy link

orijbot commented Mar 24, 2021

@cyberbono3
Copy link
Contributor Author

I run golangci-lint run
Output: server/rosetta/converter.go:316:2: Consider preallocating rawTxOps(prealloc) var rawTxOps []*rosettatypes.Operation ^ server/rosetta/converter.go:342:2: Consider preallocatingops(prealloc) var ops []*rosettatypes.Operation
How can I handle this? I have not modified rosetta package

@alessio
Copy link
Contributor

alessio commented Mar 24, 2021

I run golangci-lint run
Output: server/rosetta/converter.go:316:2: Consider preallocating rawTxOps(prealloc) var rawTxOps []*rosettatypes.Operation ^ server/rosetta/converter.go:342:2: Consider preallocatingops(prealloc) var ops []*rosettatypes.Operation
How can I handle this? I have not modified rosetta package

At times new linter releases break with past releases.
Feel free to add //nolint: prealloc at server/rosetta/converter.go:342.

As a word of advice: instead of running golangci-lint run I'd advise to just type make lint. Bonus point for running make format too :)

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

tACK, thanks @cyberbono3 for this PR!

I tested:

simd config
simd config node
simd config node "some value" # set some real node

simd q staking validators # uses node from config file
simd q staking validators --node # uses node from flag

simd config node "some value" --home ~/.test # changes value in ~/.test
simd q staking validators --home ~/.test # uses node from ~/.test config file

I left some cosmetic changes suggestions.

client/context.go Show resolved Hide resolved
client/utils.go Outdated Show resolved Hide resolved
client/utils.go Show resolved Hide resolved
./build/simd init andrei --home ./test
cd test/config there is no client.toml configuration file
*/
func ReadHomeFlag(clientCtx Context, cmd *cobra.Command) Context {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can L96-L99 call this function too?

client/config/toml.go Outdated Show resolved Hide resolved
@cyberbono3 cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from eeca868 to fc5b76a Compare March 29, 2021 21:07
@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 30, 2021

@amaury1093
Copy link
Contributor

@cyberbono3 Could you fix the tests? I believe the PR's almost ready to be merged!

@cyberbono3
Copy link
Contributor Author

cyberbono3 commented Mar 30, 2021

I believe all tests pass except rosetta and codecov. I would appreciate your help to fix them.

@cyberbono3 cyberbono3 force-pushed the cyberbono3/8529-add-client-config branch from 9e97433 to d6f55a4 Compare March 30, 2021 22:43
@amaury1093
Copy link
Contributor

The rosetta test needs to be fixed #8772
I never look at the codecov check, only the comment (e.g. #8953 (comment)) and it looks good.

Putting automerge on then!

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. backport/0.42.x (Stargate) labels Mar 31, 2021
@amaury1093 amaury1093 merged commit 410d8ed into master Mar 31, 2021
@amaury1093 amaury1093 deleted the cyberbono3/8529-add-client-config branch March 31, 2021 10:04
@amaury1093 amaury1093 mentioned this pull request Mar 31, 2021
4 tasks
@alessio
Copy link
Contributor

alessio commented Mar 31, 2021

The rosetta test needs to be fixed #8772

CC'ing @fdymylja @jgimeno

@amaury1093
Copy link
Contributor

@Mergifyio backport release/v0.42.x

@mergify
Copy link
Contributor

mergify bot commented May 3, 2021

Command backport release/v0.42.x: failure

No backport have been created
GitHub App like Mergify are not allowed to create pull request where .github/workflows is changed.

amaury1093 added a commit that referenced this pull request May 3, 2021
* add client config

* addressed reviewers comments

* refactored,ready for review

* fixed linter issues and addressed reviewers comments

* Bump golangci-lint

* fix linter warnings

* fix some tests

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Amaury <[email protected]>
aaronc pushed a commit that referenced this pull request May 17, 2021
* Add client config subcommand to CLI (#8953)

* add client config

* addressed reviewers comments

* refactored,ready for review

* fixed linter issues and addressed reviewers comments

* Bump golangci-lint

* fix linter warnings

* fix some tests

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Amaury <[email protected]>

* Fix build

Co-authored-by: Andrei Ivasko <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* add client config

* addressed reviewers comments

* refactored,ready for review

* fixed linter issues and addressed reviewers comments

* Bump golangci-lint

* fix linter warnings

* fix some tests

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Amaury <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add back cli config option for flags
6 participants