Rollup of #1498 and #1499 #1500
Closed
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's some minor differences in behavior here -- We're no longer setting the `Date` header, as clients aren't supposed to send it for requests without bodies (and even for post/put it's optional). We're also not manually setting the host header because there's literally no reason for us to be doing so.
This behavior was split into two functions so that one of the 5 places we're calling it from could intercept a 404 response code. Since the happy path otherwise is just calling `.json`, this felt really awkward to me. Instead I've opted to return `NotFound`, and downcast to it in the error path for that one place. I expected to just be able to call `Any::is`, but it turns out this method is an inherent method not a trait method (which makes sense, otherwise `Any` wouldn't be object safe). However, a side effect of that is that even though `Any` is a supertrait of `CargoError`, we can't call `Any::is` since `dyn CargoError` can't be cast to `dyn Any`. This may change at some point in the future (see rust-lang/rfcs#2035), but we have to duplicate the body of `Any::is` for now. Except we can't even just duplicate the body of `Any::is`, because the only trait method for `Any` is unstable so we have to duplicate that method, too.......... Other notable changes: - We're no longer sending `Proxy-Connection: Keep-Alive`. According to Uncyclopedia, this is "Implemented as a misunderstanding of the HTTP specifications. Common because of mistakes in implementations of early HTTP versions". Firefox recently removed it, as have recent versions of curl. - We will accept gzipped responses now. This is good. - We send an actual user agent, instead of "hello!"
Notable differences in behavior: - We now send "User-Agent: reqwest/0.9.1" to S3 - We no longer send "Proxy-Connection: Keep-Alive" (which was useless) - We now send "Accept-Encoding: gzip", which has no effect on these requests, as we do not receive any response body We are no longer manually specifying the `Date` header, since this is at best completely pointless, and at worse a latent bug waiting to crop up. We are still setting the `Date` header, as S3 requires either that header or `x-amz-date` when `Authorization` is set, since the date is part of the signature used. This commit has the largest effect from removing curl, as this was the oldest code using it, had accumulated the most cruft, and had an API most affected by its use. Going through this code was a fun jump through history. There were a few notable interesting things about the old code. The return type of `Transfer` appeared to be to work around some lifetime issues from taking a slice. Notably, `reqwest` actually requires a static lifetime on the request body (due to the API it wants to expose, and the fact that it's backed by tokio, it cannot deal with non-static lifetimes). Luckily, there were no callers that actually needed to pass a slice. My favorite part of this code though, was using a curl method that has no purpose other than to opt into `Expect: 100-continue`, and then explicitly overriding the `Expect:` header due to not understanding it's purpose. (For the record, `Expect: 100-continue` is used when sending the request body is "inappropriate or inefficient", and specifies that the server should reply with 100 CONTINUE before the client will send the request body). I'm actually surprised this interfered with the recorder. The HTTP/1.1 specification points out that older servers may not recognize the `Expect` header, and that clients should not wait an indefinite period of time before sending the request body.
And for our final replacement, we fix some code that never actually runs, and would fail if it did because running it requires credentials for two specific github users that none of us have! Hooray!
Right our pagerduty notifications come from heroku email alerts and a pingdom integration. We want to start paging on more specific metrics. So far we've discussed: - When no crate is uploaded for N period of time (likely 4 hours) - When a background job has failed N times (likely 5) - When the background queue has N jobs (likely 5) In order to do this we need to have some code in place that actually starts an incident and pages whoever is on call. We'd also like to test this code before we start relying on it, so I've added a binary we can use to make sure everything is configured correctly.
sgrif
force-pushed
the
sg-pagerduty-curl
branch
from
7927c08 to
57036ba
Compare
jtgeibel
approved these changes
Sep 26, 2018
There was a problem hiding this comment.
I've reviewed both PRs and this looks good to me. The reqwest API is much nicer to read, and it looks like there were a lot of good cleanups in the headers we send and response handling.
I'm waiting a bit to r+ just to make sure @carols10cents agrees with your response to her feedback, but since you confirmed this won't be used from the main server process I think we're good there.
I'll merge tomorrow afternoon, unless you want to merge and build on top of this sooner.
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.
#1498 uses curl. #1499 removes curl. This PR fixes that. Don't review this PR, review #1498 and #1499 separately. And then review the last commit of this PR if you want to make sure I did the replacement properly.
But please merge this PR instead of those two.
Close #1498.
Close #1499.