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.

Sign up for GitHub

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

Jump to bottom

R4R: Fix gaiacli config and make it non-interactive only #2972

Merged
merged 6 commits into from
Dec 4, 2018
Merged

R4R: Fix gaiacli config and make it non-interactive only #2972

merged 6 commits into from
Dec 4, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Dec 1, 2018 •
edited
Loading

Closes: #2734

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio alessio added the wip label Dec 1, 2018
@codecov
Copy link

codecov bot commented Dec 1, 2018 •
edited
Loading

Codecov Report

Merging #2972 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2972      +/-   ##
===========================================
+ Coverage     55.5%   55.53%   +0.02%     
===========================================
  Files          120      120              
  Lines         8494     8494              
===========================================
+ Hits          4715     4717       +2     
+ Misses        3457     3455       -2     
  Partials       322      322

@alessio alessio force-pushed the alessio/new-gaiacli-config branch 3 times, most recently from e4db8b3 to 970d56a Compare December 1, 2018 14:28
@alessio alessio changed the title WIP: Make gaiacli non-interactive R4R: Fix gaiacli and make it non-interactive only Dec 1, 2018
@alessio alessio changed the title R4R: Fix gaiacli and make it non-interactive only R4R: Fix gaiacli config and make it non-interactive only Dec 1, 2018
@alessio alessio force-pushed the alessio/new-gaiacli-config branch from 970d56a to 2945018 Compare December 1, 2018 14:30
@jackzampolin
Copy link
Member

This looks amazing, this has been annoying me lately. Maybe we rename this to gaiacli init instead to match gaiad? Will do more thorough review later.

jackzampolin
jackzampolin approved these changes Dec 2, 2018
View reviewed changes
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

This is working locally for me. Huge improvement over whats there currently! Thanks @alessio!

cwgoes
cwgoes suggested changes Dec 3, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Right direction but I think this CLI is confusing.

How do I just print out the config? Tried:

$ gaiacli config
ERROR: wrong number of arguments
$ gaiacli config --list
ERROR: wrong number of arguments

IMO gaiacli config should just print the config, and also we should set the default values (if applicable, e.g. for node) when we initialize the config so the user can see what the defaults are.

alexanderbez
alexanderbez reviewed Dec 3, 2018
View reviewed changes
client/config.go
configDefaults = map[string]string{
"chain_id": "",
"output": "text",
"node": "tcp://localhost:26657",
Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI flag and this should just use a constant variable, no? Just to be safe.

@alexanderbez
Copy link
Contributor

What is gaiacli config only meant to do? I ran it and no output appeared. This means I never ran gaiacli config before and I have no config. Should we output a warning stating no config exists and no key was given?

@alessio
Copy link
Contributor Author

alessio commented Dec 4, 2018 •
edited
Loading

EDIT: true, nothing happens as the file doesn't exist. Printing a warning seems sensible.

@alessio alessio force-pushed the alessio/new-gaiacli-config branch from d1881e4 to 82eab7b Compare December 4, 2018 10:23
@cwgoes
Copy link
Contributor

cwgoes commented Dec 4, 2018

Tested ACK; in the future I think we could make the possible config keys more obvious.

@cwgoes cwgoes merged commit d32e4a9 into develop Dec 4, 2018
@cwgoes cwgoes deleted the alessio/new-gaiacli-config branch December 4, 2018 13:17
mircea-c pushed a commit that referenced this pull request Dec 5, 2018
@alessio
Merge PR #2972: Fix gaiacli config and make it non-interactive only
0e0b9e8 
* Fix gaiacli config and make it non-interactive only

Closes: #2734

* Update cli tests

* Remove --list, by default and with no args print config

* Small improvements

* Warn user when file doesn't exist

* Fix integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@cwgoes cwgoes cwgoes approved these changes

@jackzampolin jackzampolin jackzampolin approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alessio @jackzampolin @alexanderbez @cwgoes