Skip to content

Fix export segfault#2128

Merged
cwgoes merged 1 commit intocosmos:developfrom
mslipper:issue-1834
Aug 24, 2018
Merged

Fix export segfault#2128
cwgoes merged 1 commit intocosmos:developfrom
mslipper:issue-1834

Conversation

@mslipper
Copy link
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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?

server/export.go Outdated
Copy link
Copy Markdown
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
Copy Markdown
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)

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
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