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 export segfault #2128

Merged
merged 1 commit into from
Aug 24, 2018
Merged

Fix export segfault #2128

merged 1 commit into from
Aug 24, 2018

Conversation

mslipper
Copy link
Contributor

Closes #1834

  • 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)

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK -- looks great @mslipper, just left a remark or two 👍

server/export.go Outdated
}

if emptyState {
fmt.Println("WARNING: State is not initialized. Returning genesis.json.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning genesis file. I think might sound a bit better?

@@ -43,3 +60,12 @@ func ExportCmd(ctx *Context, cdc *wire.Codec, appExporter AppExporter) *cobra.Co
},
}
}

func isEmptyState(home string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ValarDragon is this sanity check enough or is there a more direct method we can perform via some attempted state lookup?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me since we ensure all file writes persist to disk. I think we can merge this, and then write an issue to query state directly later. (#postlaunch concern)

os.Stdout = old
out := <-outC
require.Contains(t, out, "WARNING: State is not initialized")
require.Contains(t, out, "consensus_params")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check for other top level keys?

@alexanderbez
Copy link
Contributor

Also let's merge develop in so the test_sim_gaia_fast will pass 👍

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #2128 into develop will increase coverage by 0.05%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #2128      +/-   ##
===========================================
+ Coverage    63.85%   63.91%   +0.05%     
===========================================
  Files          134      134              
  Lines         8175     8194      +19     
===========================================
+ Hits          5220     5237      +17     
+ Misses        2605     2604       -1     
- Partials       350      353       +3

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.

utACK, thanks.

In the future, please don't continuously squash commits - we can do that when we merge (the checklist is confusing, sorry...)

@cwgoes cwgoes merged commit 5128a7c into cosmos:develop Aug 24, 2018
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