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

fix client config don't take effect #9211

Merged
merged 4 commits into from
May 19, 2021
Merged

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Apr 27, 2021

Description

  • Set keyring-backend in client.toml don't work, because keyring-dir is not set properly.
    Solution: set keyring-dir to home by default when reading client.toml.
  • Some commands override what's set in client.toml even if not pass command line flag.
    Solution: keep client.toml value if flag not passed.

closes: #XXXX


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

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #9211 (3a5a5f3) into master (9d05048) will decrease coverage by 0.65%.
The diff coverage is 72.72%.

❗ Current head 3a5a5f3 differs from pull request most recent head 9116210. Consider uploading reports for the commit 9116210 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9211      +/-   ##
==========================================
- Coverage   60.38%   59.72%   -0.66%     
==========================================
  Files         587      595       +8     
  Lines       36920    37348     +428     
==========================================
+ Hits        22294    22306      +12     
- Misses      12651    13060     +409     
- Partials     1975     1982       +7     
Impacted Files Coverage Δ
client/keys/show.go 83.67% <33.33%> (-1.90%) ⬇️
client/keys/add.go 62.50% <83.33%> (ø)
client/config/config.go 54.83% <100.00%> (+1.50%) ⬆️
x/feegrant/keeper/grpc_query.go 0.00% <0.00%> (-81.40%) ⬇️
x/feegrant/keeper/msg_server.go 6.89% <0.00%> (-79.32%) ⬇️
x/bank/client/testutil/cli_helpers.go 8.10% <0.00%> (-41.90%) ⬇️
x/params/module.go 11.11% <0.00%> (-40.75%) ⬇️
types/codec.go 50.00% <0.00%> (-16.67%) ⬇️
store/internal/proofs/create.go 67.56% <0.00%> (-13.83%) ⬇️
x/authz/keeper/grpc_query.go 67.21% <0.00%> (-9.26%) ⬇️
... and 117 more

@yihuang yihuang changed the title fix client keyring config fix client config don't have effect Apr 28, 2021
@yihuang yihuang changed the title fix client config don't have effect fix client config don't take effect Apr 28, 2021
@cyberbono3
Copy link
Contributor

cyberbono3 commented Apr 29, 2021

@yihuang To scope the problem, can you provide some examples please for:

  1. Set keyring-backend in client.toml don't work?
  2. Some commands override what's set in client.toml even if not pass command line flag

You can use --home flag to set HomeDir. ReadHomeFlag checks if home flag is set. If this is a case, it updates HomeDir field of client. Context. Otherwise, it uses default HomeDir.

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.

I reproduced the error, and this fix works. Approving.

Set keyring-backend in client.toml don't work?

Yeah, client.toml is correctly updated, but the CLI doesn't read from it.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

utACK

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label May 18, 2021
@github-actions github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels May 19, 2021
@mergify mergify bot merged commit b4d1a5e into cosmos:master May 19, 2021
mergify bot pushed a commit that referenced this pull request May 19, 2021
* fix client keyring config

* fix output flag of keys commads

Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Amaury <[email protected]>
(cherry picked from commit b4d1a5e)

# Conflicts:
#	client/keys/add.go
aaronc pushed a commit that referenced this pull request May 19, 2021
* fix client config don't take effect (#9211)

* fix client keyring config

* fix output flag of keys commads

Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Amaury <[email protected]>
(cherry picked from commit b4d1a5e)

# Conflicts:
#	client/keys/add.go

* Fix conflicts

Co-authored-by: yihuang <[email protected]>
Co-authored-by: Amaury M <[email protected]>
roysc pushed a commit to vulcanize/cosmos-sdk that referenced this pull request Jun 23, 2021
* fix client keyring config

* fix output flag of keys commads

Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Amaury <[email protected]>
faddat pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request Dec 19, 2022
* fix client config don't take effect (cosmos#9211)

* fix client keyring config

* fix output flag of keys commads

Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Amaury <[email protected]>
(cherry picked from commit b4d1a5e)

* Fix conflicts

Co-authored-by: yihuang <[email protected]>
Co-authored-by: Amaury M <[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. C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants