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.

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

ref: Migrate from rusoto to aws-sdk-s3 #954

Merged
merged 73 commits into from
Jan 9, 2023
Merged

ref: Migrate from rusoto to aws-sdk-s3 #954

merged 73 commits into from
Jan 9, 2023

Conversation

Swatinem
Copy link
Member

Supercedes #849, CC @josb

Merges the latest changes from master, cleans up the error handling and adds back support for custom regions.

Fixes #575

Jos Backus and others added 15 commits November 25, 2022 06:47
Rusoto hardcoded the available regions, aws-sdk-rust does not. But
there's no way to obtain the current list of valid regions without
making API calls, which means having working credentials and either
defaulting to some region (like us-east-1) or having the user specify a
valid region anyway.
Unlike Rusoto, the Rust AWS SDK doesn't hardcode the currently known
regions, and no validation is provided. API requests with invalid region
names will fail.
@Swatinem Swatinem requested a review from a team December 28, 2022 12:34
@Swatinem Swatinem changed the title ref: Migrate from rusoto to aws-sdk-rust ref: Migrate from rusoto to aws-sdk-s3 Dec 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2022

Codecov Report

Base: 65.95% // Head: 66.05% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (d2743fa) compared to base (ac4e0a9).
Patch coverage: 45.50% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
+ Coverage   65.95%   66.05%   +0.09%     
==========================================
  Files          74       74              
  Lines       11754    11759       +5     
==========================================
+ Hits         7752     7767      +15     
+ Misses       4002     3992      -10     
Impacted Files Coverage Δ
.../symbolicator-service/src/services/download/gcs.rs 41.29% <0.00%> (ø)
...mbolicator-service/src/services/download/sentry.rs 18.40% <0.00%> (ø)
.../symbolicator-service/src/services/download/mod.rs 74.79% <33.33%> (ø)
...s/symbolicator-service/src/services/download/s3.rs 42.89% <37.69%> (+5.58%) ⬆️
crates/symbolicator-sources/src/sources/s3.rs 89.75% <71.42%> (-5.63%) ⬇️
...symbolicator-service/src/services/download/http.rs 82.53% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@josb
Copy link
Contributor

josb commented Dec 28, 2022

Thanks for pulling this across the finish line, @Swatinem! I was just about to push my own updates to deal with the latest SDK, which included similar changes to the error handling. I hadn't gotten quite as far cleaning it up and handling all the necessary cases as needed but I was getting there, albeit slowly, and enjoying the learning process. Since the SDK isn't yet GA I figured there was still time to finish this PR, because it would have been nice to complete this myself. But I understand and appreciate your work to land this.

@Swatinem
Copy link
Member Author

I should have communicated the intent here better.

We have already done a few refactors to all the downloader related things, and next up on that list would be to change the main downloader interface, in particular replacing DownloadError. That would probably have caused a lot of pain keeping up to date, so to avoid that, I wanted to land this prior to that next big step.

@josb
Copy link
Contributor

josb commented Dec 29, 2022

No worries, @Swatinem. Had I known I would have pushed my changes for the latest AWS SDK sooner, because I spent several hours trying to understand and address the error handling changes. And I was on my way to clean them up, but granted it would have taken me way more time than you. ☺ I had planned to address the endpoint issue, too; I would have figured it out eventually but it would have taken me a while, due to my lack of experience and time to devote to this project. Given the refactoring you describe, merging this soon does seem like the right approach.

@Swatinem Swatinem merged commit 14794aa into master Jan 9, 2023
@Swatinem Swatinem deleted the ref/aws-sdk branch January 9, 2023 12:46
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.

Migrate from rusoto to aws-sdk-rust
4 participants