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

R4R: Add validate-genesis command #3399

Merged
merged 7 commits into from
Jan 28, 2019
Merged

Conversation

jackzampolin
Copy link
Member

@jackzampolin jackzampolin commented Jan 25, 2019

Fixes: #3398

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • 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)

RunE: func(cmd *cobra.Command, args []string) (err error) {
var genesis string
var genDoc types.GenesisDoc
var genstate app.GenesisState
Copy link
Contributor

Choose a reason for hiding this comment

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

These vars should be defined right before their first usage (aka genesis on line 31 etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

one minor comment to be addressed, otherwise LGTM

@codecov
Copy link

codecov bot commented Jan 25, 2019

Codecov Report

Merging #3399 into develop will decrease coverage by 0.19%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop    #3399     +/-   ##
==========================================
- Coverage    59.38%   59.19%   -0.2%     
==========================================
  Files          131      132      +1     
  Lines         9862     9884     +22     
==========================================
- Hits          5857     5851      -6     
- Misses        3632     3660     +28     
  Partials       373      373

} else {
genesis = args[0]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a diagnostic info here just to inform the user which location genesis.json is being read from:

fmt.Fprintf(os.Stderr, "validating %s", genesis)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great add! Thank you!

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Minor comment, not a blocker though. Good to go for me.

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

I'm sorry, forgot one thing: please add a cli test

@jackzampolin
Copy link
Member Author

Thank you for keeping me honest @alessio will do!

cmd/gaia/init/validate_genesis.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

@jackzampolin any updates on this? Also, integration_tests is failing.

@jackzampolin
Copy link
Member Author

@alexanderbez I did update it Satuday, need to adjust test to not fail.

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

ACK - pending tests pass

@jackzampolin jackzampolin merged commit df86585 into develop Jan 28, 2019
@cwgoes cwgoes deleted the jack/genesis-validate-command branch January 28, 2019 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants