chore(deps): update rocket, failure, rusoto and rust versions #160
Conversation
| @@ -97,9 +97,13 @@ impl Factory for Queue { | |||
| let client: Box<Sqs> = if let Some(ref keys) = settings.aws.keys { | |||
| let creds = | |||
| StaticProvider::new(keys.access.0.clone(), keys.secret.0.clone(), None, None); | |||
| Box::new(SqsClient::new(RequestDispatcher::default(), creds, region)) | |||
| Box::new(SqsClient::new_with( | |||
| HttpClient::new().expect("Couldn't start HTTP Client."), | |||
There was a problem hiding this comment.
I'm not really sure about this expect here, what do you think @philbooth ? Should I change this function to return a Result and handle the errors or panicking is alright in this case?
There was a problem hiding this comment.
I think panicking in this case is fine because it's initialisation code, so the process will abort on startup rather than later when we think everything is working correctly.
| rusoto_credential = { git = "https://github.com/rusoto/rusoto/", rev="9e22eb3876a02375b3dbe14d0340f9b5c1fc55e9" } | ||
| rusoto_mock = { git = "https://github.com/rusoto/rusoto/", rev="9e22eb3876a02375b3dbe14d0340f9b5c1fc55e9" } | ||
| rusoto_ses = { git = "https://github.com/rusoto/rusoto/", rev="9e22eb3876a02375b3dbe14d0340f9b5c1fc55e9" } | ||
| rusoto_sqs = { git = "https://github.com/rusoto/rusoto/", rev="9e22eb3876a02375b3dbe14d0340f9b5c1fc55e9" } |
There was a problem hiding this comment.
It mildly concerns me that our dependence on git hashes instead of released versions of crates appears to be growing rather than shrinking. Are we really so desperate for the targeted features (including for the preceding one that caused the initial Rocket dependency on a commit hash) that we can't wait for them to show up in official releases?
There was a problem hiding this comment.
Yes, I don't like that either... in this case we are pointing to the 0.33.0 release commit, so it should be fine. Also I will change this to an actual version as soon as rusoto/rusoto#1089 gets done.
We probably could have waited for them to release the rusoto version with the fixes that we need for rusoto_mock, but since we wanna test ASAP if the new release fixes our SES problems (#159) I just pointed to the revision for now.
The rocket revision concerns me more because that is almost a random commit, but apparently they are going to release the new version before RustConf or during RustConf, anyways, we will be able to change that soon :)
| @@ -3,7 +3,7 @@ services: | |||
| - redis-server | |||
| cache: cargo | |||
| rust: | |||
| - nightly-2018-07-14 | |||
| - nightly-2018-08-06 | |||
There was a problem hiding this comment.
Dockerfile also needs updating to match this.
There was a problem hiding this comment.
Apart from the Dockerfile update, I have no hard objections to this so r+ modulo that.
I am a little bit uncomfortable that dependening on git hashes is becoming more prevalent, but I'll defer to your and @vladikoff's judgement on that point.
My concern is mostly that we're setting ourselves up for potentially unnecessary churn by depending on details that haven't settled down as part of an official release yet. That applies to both details we know about (the things we're targeting those commit hashes for) and possibly ones we don't know about too (other commits in the tree that we're pulling in with those hashes).
brizental
force-pushed
the
brizental/updates
branch
from
cdef829
to
9854cc9
Compare
|
Thanks, @philbooth ! I made the |
Yeah agreed, we can file follow up issues to get rid of |
Fixes #156
Fixes #158
Here I update failure, rocket, rusoto and rust to the latest versions.
I had to point rusoto to a revision, because
rusoto_mockstopped working after the update. I opened an issue about that rusoto/rusoto#1089.Because I updated the rust version, there are a bunch of formatting changes in this PR, because fmt was updated as well.
r? @philbooth @vladikoff
ps.: when this gets merged we can't forget to change the override in
fxa-local-devhttps://github.com/mozilla/fxa-local-dev/blob/master/_scripts/install_all.sh#L42