Skip to content

cli: Full snapshot interval doesn't need to be a multiple of the incremental#6595

Merged
brooksprumo merged 2 commits intoanza-xyz:masterfrom
brooksprumo:snap/cli
Jun 16, 2025
Merged

cli: Full snapshot interval doesn't need to be a multiple of the incremental#6595
brooksprumo merged 2 commits intoanza-xyz:masterfrom
brooksprumo:snap/cli

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Jun 15, 2025

Since PR #5519, the cli help for --full-snapshot-interval-slots is wrong: the full snapshot interval does not need to be a multiple of the incremental snapshot interval.

@brooksprumo brooksprumo self-assigned this Jun 15, 2025
@brooksprumo brooksprumo marked this pull request as ready for review June 15, 2025 17:45
@brooksprumo brooksprumo requested a review from jstarry June 15, 2025 17:47
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.8%. Comparing base (6b566ba) to head (a21617f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6595   +/-   ##
=======================================
  Coverage    82.8%    82.8%           
=======================================
  Files         849      849           
  Lines      379047   379047           
=======================================
+ Hits       313966   313973    +7     
+ Misses      65081    65074    -7     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brooksprumo brooksprumo requested a review from steviez June 16, 2025 17:43
Comment thread validator/src/commands/run/args.rs Outdated
Comment on lines +447 to +448
"Number of slots between generating full snapshots. \
Only used when incremental snapshots are enabled.",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Must it still be greater or equal to the incremental snapshot interval?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes. Must be strictly greater than. Want me to add that?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah that would be great, thx

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Fixed in e00c9ab.

@brooksprumo brooksprumo requested a review from jstarry June 16, 2025 20:26
@brooksprumo brooksprumo merged commit 40c7522 into anza-xyz:master Jun 16, 2025
28 checks passed
@brooksprumo brooksprumo deleted the snap/cli branch June 16, 2025 21:55
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Glad you didn't wait for me to merge, but in clearing my queue, this LGTM

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