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.

Sign up for GitHub

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

Jump to bottom

Refactor: rework snapshot_uploaders module to improve genericity #2165

Merged
merged 13 commits into from
Dec 11, 2024
Merged

Refactor: rework snapshot_uploaders module to improve genericity #2165

merged 13 commits into from
Dec 11, 2024

Conversation

dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Dec 10, 2024 •
edited
Loading

Content

This PR includes a refactor of the snapshot_uploaders module and its sub-modules. This is preparatory work to start implementing the sub-builders and uploaders in the issue #2151.
It involves renaming to remove references to snapshots. The GCP module has been revised to implement directly the FileUploader trait.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is 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
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Issue(s)

Relates to #2151

sfauvel
sfauvel approved these changes Dec 10, 2024
View reviewed changes
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

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 10, 2024 •
edited
Loading

Test Results

    4 files  ±0     52 suites  ±0   9m 44s ⏱️ -4s
1 428 tests  - 1  1 428 ✅  - 1  0 💤 ±0  0 ❌ ±0 
1 639 runs   - 1  1 639 ✅  - 1  0 💤 ±0  0 ❌ ±0 

Results for commit 460c9af. ± Comparison against base commit 4cd38e2.

This pull request removes 6 and adds 5 tests. Note that renamed tests count towards both. 
mithril-aggregator ‑ snapshot_uploaders::dumb_snapshot_uploader::tests::test_dumb_uploader
mithril-aggregator ‑ snapshot_uploaders::local_snapshot_uploader::tests::should_copy_file_to_target_location
mithril-aggregator ‑ snapshot_uploaders::local_snapshot_uploader::tests::should_extract_digest_to_deduce_location
mithril-aggregator ‑ snapshot_uploaders::remote_snapshot_uploader::tests::test_upload_snapshot_ko
mithril-aggregator ‑ snapshot_uploaders::remote_snapshot_uploader::tests::test_upload_snapshot_not_using_cdn_domain_ok
mithril-aggregator ‑ snapshot_uploaders::remote_snapshot_uploader::tests::test_upload_snapshot_using_cdn_domain_ok

mithril-aggregator ‑ file_uploaders::dumb_uploader::tests::test_dumb_uploader
mithril-aggregator ‑ file_uploaders::gcp_uploader::tests::get_location_not_using_cdn_domain_return_google_api_uri
mithril-aggregator ‑ file_uploaders::gcp_uploader::tests::get_location_using_cdn_domain_return_cdn_in_uri
mithril-aggregator ‑ file_uploaders::local_uploader::tests::should_copy_file_to_target_location
mithril-aggregator ‑ file_uploaders::local_uploader::tests::should_extract_digest_to_deduce_location

♻️ This comment has been updated with latest results.

@dlachaume dlachaume force-pushed the ensemble/2151/refactor-uploaders-module branch from 960ad53 to fc4da20 Compare December 10, 2024 15:48
@dlachaume dlachaume marked this pull request as ready for review December 10, 2024 15:56
@dlachaume dlachaume temporarily deployed to testing-sanchonet December 10, 2024 15:58 — with GitHub Actions Inactive
jpraynaud
jpraynaud reviewed Dec 10, 2024
View reviewed changes
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.

Overall looks good 👍

Can you avoid the cumbersome repetition of _file_uploaders.rs in the files of the file_uploaders module and rename file_uploader.rs to interface.rs?

mithril-aggregator/src/file_uploaders/remote_file_uploader.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/file_uploaders/file_uploader.rs Outdated Show resolved Hide resolved
dlachaume and others added 11 commits December 11, 2024 15:58
@dlachaume @sfauvel
refactor: rename snapshot_uploaders module to file_uploaders
ad0cb4c
@dlachaume @sfauvel
refactor: rename snapshot_uploader module to file_uploader
e58be84
@dlachaume @sfauvel
refactor: rename snapshot to file in file_uploader module
171fb97
@dlachaume @sfauvel
refactor: rename uploader implementation modules for consistency
9da14ea
@dlachaume @sfauvel
refactor: rename DumbSnapshotUploader to DumbFileUploader and rem…
e2cfd5f 
…ove `snapshot` references
@dlachaume @sfauvel
refactor: rename LocalSnapshotUploader to LocalFileUploader and r…
3308c69 
…emove `snapshot` references

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume @sfauvel
refactor: make GcpFileUploader implement FileUploader
95693bf 
Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume @sfauvel
refactor: rename remote_file_uploader to gcp_file_uploader
8268e5f
@dlachaume @sfauvel
refactor: move gcp_file_uploader to file_uploaders module
424f6d3
@sfauvel
refactor: Remobe file from file uploaders modules
0084fae
@sfauvel
refactor: remove RemoteUploader
8d5e1c7
@sfauvel sfauvel force-pushed the ensemble/2151/refactor-uploaders-module branch from d7d93fe to eaa1fca Compare December 11, 2024 15:04
@dlachaume dlachaume requested a review from jpraynaud December 11, 2024 15:05
@sfauvel sfauvel force-pushed the ensemble/2151/refactor-uploaders-module branch 2 times, most recently from 4c75959 to fd971e8 Compare December 11, 2024 15:12
@sfauvel sfauvel temporarily deployed to testing-sanchonet December 11, 2024 15:22 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Dec 11, 2024
View reviewed changes
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 👍

mithril-aggregator/src/file_uploaders/gcp_uploader.rs
#[async_trait]
impl FileUploader for GcpUploader {
async fn upload(&self, filepath: &Path) -> StdResult<FileUri> {
if env::var("GOOGLE_APPLICATION_CREDENTIALS_JSON").is_err() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we will have to do this in the dependency builder in a future PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, that could be a good idea.

mithril-aggregator/src/file_uploaders/interface.rs Show resolved Hide resolved
@sfauvel sfauvel temporarily deployed to testing-sanchonet December 11, 2024 15:45 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the ensemble/2151/refactor-uploaders-module branch from 4762b07 to 460c9af Compare December 11, 2024 15:45
@sfauvel sfauvel temporarily deployed to testing-sanchonet December 11, 2024 15:54 — with GitHub Actions Inactive
@sfauvel sfauvel merged commit 404212a into main Dec 11, 2024
51 of 53 checks passed
@sfauvel sfauvel deleted the ensemble/2151/refactor-uploaders-module branch December 11, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpraynaud jpraynaud jpraynaud approved these changes

@sfauvel sfauvel sfauvel approved these changes

@Alenar Alenar Alenar approved these changes

Assignees

@sfauvel sfauvel

@dlachaume dlachaume

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlachaume @Alenar @sfauvel @jpraynaud