-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Configurable pruning #3195
Conversation
c1c6270
to
d2453bc
Compare
71fe7f3
to
32e43e4
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3195 +/- ##
===========================================
+ Coverage 54.83% 54.88% +0.04%
===========================================
Files 133 134 +1
Lines 9559 9551 -8
===========================================
Hits 5242 5242
+ Misses 3996 3988 -8
Partials 321 321 |
Codecov Report
@@ Coverage Diff @@
## develop #3195 +/- ##
===========================================
+ Coverage 55.33% 55.33% +<.01%
===========================================
Files 134 135 +1
Lines 9602 9594 -8
===========================================
- Hits 5313 5309 -4
+ Misses 3957 3953 -4
Partials 332 332 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hleb-albau. Left a bit of minor feedback. In addition, this needs a pending log update under the breaking section.
|
||
func NewPruningOptions(strategy string) (opt PruningOptions) { | ||
switch strategy { | ||
case "nothing": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be constants defined in this file and used everywhere NewPruningOptions
is called imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this method to parse viper flag value. To be able to change default pruning options, baseapp should take not just strategy, but options itself. So we can't just pass strategy to ** baseapp**. Maybe I miss something?
Btw, I open PruneEverything, PruneNothing, PruneSyncable and change tests to use them, instead of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK for now.
32e43e4
to
c943482
Compare
fff1c64
to
d791ee4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, thanks for the contribution!
|
||
func NewPruningOptions(strategy string) (opt PruningOptions) { | ||
switch strategy { | ||
case "nothing": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK for now.
closes: #3194
Allows custom configuration for syncable strategy