-
Notifications
You must be signed in to change notification settings - Fork 13
s3 rusoto and general futures 0.3 migration of blobstore #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged as it's green in mozilla/sccache#869
Are we using s3 storage for our setup? |
Nope, that would be quite slow and expensive. |
Hence I am asking, minio scales pretty well. |
* change (CI): initial CI * change (CI): initial CI * change (CI): add clippy and allow failure
19fef99
to
113609b
Compare
PRs labelled `autorebase:opt-in` will be rebased automatically after updates in `master`.
a66c5c9
to
2124aae
Compare
cd208f1
to
1d2d464
Compare
]; | ||
let profile_provider = | ||
ProfileProvider::with_configuration(home.join(".aws").join("credentials"), "default") | ||
// //TODO: this is hacky, this is where our mac builders store their |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double checking : are those double '//' '//' introduced intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not at all.
runtime.block_on(server.with_graceful_shutdown(async move { | ||
let _ = shutdown_signal; | ||
})) | ||
// .map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to leave this code commented?
is it ok to not handle this in new code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a bit of a TODO..
We migrated to futures 0.3 in #68 however the simples3 implementation was translated directly, to reduce the review surface for the original PR at mozilla/sccache#985. Are we still interested in Rusoto? It's worth noting that the project entered maintenance mode and it seems that the new official AWS SDK for Rust was recently announced and is in alpha stage now: https://aws.amazon.com/blogs/developer/a-new-aws-sdk-for-rust-alpha-launch/ (repo: https://github.com/awslabs/aws-sdk-rust) |
Reducing the API surface was a very good call (thanks!). Not sure how to proceed. rusoto has (I think) a few nice to have features. I am bit wary of migrating to anything super-alpha mostly due to the potential breakage up the road, rusoto is likely going to be stable and stick around for quite some time. It's your call essentially, not sure if other PRs like clean bucket depend on it. Slight tendency to for rusoto, to simplify away some internal http chore we don't really care about. |
Bump. Can this be merged? |
This PR needs to be condensed down to the |
@drahnr can you take a look at https://github.com/paritytech/cachepot/tree/igor-s3-rusoto? I adapted the changes from https://github.com/diem/sccache/tree/rusoto which AIUI the originally mentioned mozilla/sccache#869 uses. I didn't force update the branch in case there's something else we might need to port as well. |
mozilla/sccache#869