Aggregator: Re-implement the CloudBackendUploader with gcloud-storage crate #2475
+698
−800
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR re-implements the Google Cloud Storage uploader to use the actively maintained gcloud-storage crate, replacing the legacy cloud-storage implementation. Key changes include renaming and reorganizing modules within the gcp_uploader directory, adapting asynchronous build functions in dependency injection, and updating the Cargo.toml dependency.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/file_uploaders/mod.rs | Renamed import from GcpBackendUploader to GCloudBackendUploader to reflect the new implementation. |
| src/file_uploaders/gcp_uploader/mod.rs | Organized the gcp_uploader module into separate sub-modules. |
| src/file_uploaders/gcp_uploader/interface.rs | Introduced the CloudRemotePath type and the CloudBackendUploader trait with tests for URI generation. |
| src/file_uploaders/gcp_uploader/gcloud_backend.rs | Implemented the new GCloudBackendUploader using the gcloud-storage client with async operations and error contexts. |
| src/file_uploaders/gcp_uploader/api.rs | Provided the GcpUploader interactor that wraps the new uploader and added comprehensive tests. |
| src/dependency_injection/builder/protocol/artifacts.rs | Updated async build functions to create instances of GCloudBackendUploader and adjusted related uploader builders. |
| Cargo.toml | Removed the deprecated cloud-storage dependency and added gcloud-storage. |
Test Results 3 files ±0 57 suites ±0 11m 35s ⏱️ +3s Results for commit 3c9f808. ± Comparison against base commit 9df5a56. This pull request removes 9 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
jpraynaud
approved these changes
May 13, 2025
mithril-aggregator/src/file_uploaders/gcp_uploader/gcloud_backend.rs
Outdated
Show resolved
Hide resolved
Alenar
force-pushed
the
djo/2460/reimpl_gcp_uploader
branch
from
bf257e4 to
e78e57c
Compare
* promoted to a directory * splited into several part in order to isolate the old backend since it will be removed soon
…o `CloudRemotePath` * Renamed to `to_gcloud_storage_location` * This allow easy reuse of this code with the new backend
Alenar
force-pushed
the
djo/2460/reimpl_gcp_uploader
branch
from
e78e57c to
0972218
Compare
As its components are not tied to GCP but only the backend.
* mithril-aggregator from `0.7.47` to `0.7.48`
Alenar
force-pushed
the
djo/2460/reimpl_gcp_uploader
branch
from
0972218 to
3c9f808
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Content
This PR re-implement the GCP Storage
CloudBackendUploaderusinggcloud-storage, replacing the previous implementation that was usingcloud-storage.cloud-storagehave not seen any update in three years and, since it make external requests, can pose security risks. The newgcloud-storageis more "bare metal" but is actively maintained.Changes
gcp_uploadermodule:cloud_uploaderas it's the backend that ties to a specific providerget_locationfrom the legacy uploader to theCloudRemotePath(renamed toto_gcloud_storage_location) since we need to reuse it in the new uploaderGcpBackendUploaderLegacythen delete it and remove thecloud-storagedependencyCloudBackendUploaderusinggcloud-storage: it should be functionally equivalent (I made extra care to ensure the same api paths are called with the same parameters), but since there's no easy way to unit test it we will have to do it directly ontesting-previewPre-submit checklist
Comments
If the new implementation crash on
testing-previewand can't be easily fixed, revert the commit namedchore(aggregator): remove legacy gcp storage uploader backendand rewire theGcpBackendUploaderLegacyin dependency injection.Issue(s)
Relates to #2460