Skip to content

split snapshotter #2245

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

Merged
merged 4 commits into from
Jan 27, 2025
Merged

split snapshotter #2245

merged 4 commits into from
Jan 27, 2025

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jan 24, 2025

Content

This PR makes the snapshotter code more manageable by:

  • moving it to services so it's no longer uncategorized at the source route
  • split the module into several files to limit amount of code to navigate and make it more cohesive.

This PR also simplify local uploaders constructors by removing their StdResult (no longer needed since #2241)

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@Alenar Alenar added the refactoring 🛠️ Code refactoring and enhancements label Jan 24, 2025
@Alenar Alenar requested review from sfauvel and jpraynaud January 24, 2025 17:52
Copy link

github-actions bot commented Jan 24, 2025

Test Results

    4 files  ±0     52 suites  ±0   10m 23s ⏱️ -11s
1 545 tests ±0  1 545 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 801 runs  ±0  1 801 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cf2654d. ± Comparison against base commit c519913.

This pull request removes 15 and adds 15 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ snapshotter::tests::can_return_full_archive_path
mithril-aggregator ‑ snapshotter::tests::can_set_temp_dir_with_str_or_string
mithril-aggregator ‑ snapshotter::tests::is_snapshot_exist_return_true_when_file_exists
mithril-aggregator ‑ snapshotter::tests::should_clean_pending_snapshot_directory_if_already_exists
mithril-aggregator ‑ snapshotter::tests::should_create_a_valid_archive_with_gzip_snapshotter
mithril-aggregator ‑ snapshotter::tests::should_create_a_valid_archive_with_zstandard_snapshotter
mithril-aggregator ‑ snapshotter::tests::should_create_directory_if_does_not_exist
mithril-aggregator ‑ snapshotter::tests::should_delete_tmp_file_in_pending_snapshot_directory_if_snapshotting_fail
mithril-aggregator ‑ snapshotter::tests::snapshot_overwrite_archive_already_existing
mithril-aggregator ‑ snapshotter::tests::snapshot_subset_return_error_when_empty_entries
…
mithril-aggregator ‑ services::snapshotter::appender::tests::snapshot_subset_return_error_when_empty_entries
mithril-aggregator ‑ services::snapshotter::appender::tests::snapshot_subset_return_error_when_file_or_directory_not_exist
mithril-aggregator ‑ services::snapshotter::appender::tests::snapshot_subset_should_create_archive_only_for_specified_directories_and_files
mithril-aggregator ‑ services::snapshotter::appender::tests::snapshot_subset_with_duplicate_files_and_directories
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::can_return_full_archive_path
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::can_set_temp_dir_with_str_or_string
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::is_snapshot_exist_return_true_when_file_exists
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::should_clean_pending_snapshot_directory_if_already_exists
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::should_create_a_valid_archive_with_gzip_snapshotter
mithril-aggregator ‑ services::snapshotter::compressed_archive_snapshotter::tests::should_create_a_valid_archive_with_zstandard_snapshotter
…

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview January 24, 2025 18:01 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet January 24, 2025 18:01 — with GitHub Actions Inactive
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM
Just one remark on the file that contain tests for TarAppender

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Co-authored-by: Damien Lachaume <dlachaume@users.noreply.github.com>
@Alenar Alenar force-pushed the ensemble/move_split_snapshotter branch from ec310e0 to 0831822 Compare January 27, 2025 14:05
Alenar and others added 3 commits January 27, 2025 15:06
The snapshotter has become quite big with time with more than 900 lines
and multiples structs, this split makes the code more manageable.

Co-authored-by: Damien Lachaume <dlachaume@users.noreply.github.com>
by removing the StdResult as they raise no errors.

Co-authored-by: Damien Lachaume <dlachaume@users.noreply.github.com>
* mithril-aggregator from `0.6.19` to `0.6.20`
@Alenar Alenar force-pushed the ensemble/move_split_snapshotter branch from 0831822 to cf2654d Compare January 27, 2025 14:06
@Alenar Alenar temporarily deployed to testing-preview January 27, 2025 14:19 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet January 27, 2025 14:19 — with GitHub Actions Inactive
@dlachaume dlachaume merged commit dddc0ee into main Jan 27, 2025
46 checks passed
@dlachaume dlachaume deleted the ensemble/move_split_snapshotter branch January 27, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring 🛠️ Code refactoring and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants