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

Remove bank_snapshots_dir param #31249

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

xiangzhu70
Copy link
Contributor

@xiangzhu70 xiangzhu70 commented Apr 18, 2023

Problem

From suggestion in #30978 (comment), use a split PR to remove the bank_snapshots_dir in a few functions. The reason is that it can be derived from bank_snapshot_info.

Summary of Changes

Remove the param in new_from_snapshot() and the related calling functions.
Added SnapshotError::InvalidSnapshotDirPath to handle parent() error.

Fixes #

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #31249 (20d8ca9) into master (f5fe260) will increase coverage by 0.0%.
The diff coverage is 66.6%.

@@           Coverage Diff           @@
##           master   #31249   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         729      729           
  Lines      206621   206626    +5     
=======================================
+ Hits       168484   168519   +35     
+ Misses      38137    38107   -30     

@xiangzhu70 xiangzhu70 requested a review from apfitzge April 18, 2023 18:17
@xiangzhu70 xiangzhu70 marked this pull request as ready for review April 18, 2023 18:17
@xiangzhu70 xiangzhu70 requested a review from brooksprumo April 18, 2023 18:18
apfitzge
apfitzge previously approved these changes Apr 18, 2023
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@xiangzhu70 xiangzhu70 merged commit a5275f8 into solana-labs:master Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants