-
Notifications
You must be signed in to change notification settings - Fork 62
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
Support HTTP.jl 1.0 #560
Support HTTP.jl 1.0 #560
Conversation
bors try |
Oh, I just barely tagged GitHub.jl release w/ HTTP.jl 1.0, so we may need to wait 10-15 minutes until that release is public for things to work here. |
tryBuild failed: |
Your modified compat shows HTTP 0.9 as supported, but tests appear to fail with HTTP 0.9. |
Ok, pushed an update with better 0.9 support; and the GitHub.jl release is now public. |
bors try |
tryBuild failed: |
Ok, looks like I'm going to need to go thru the tests so me more and fix things up. |
bors try |
tryBuild failed: |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
bors try |
tryBuild succeeded: |
Woohoo! Tests pass! @iamed2, do you have any concerns on the Julia version bump? Or anyone else? |
Bump; anyone up for reviewing? Anyone mind if I merge next Monday and tag a new release? |
I can take a look and play around with it this weekend most likely! |
Maybe this got lost in an Internet blip, but I don't have any concerns there. I think we should check whether downstream packages like CloudWatchLogs.jl and AWSS3.jl continue to work under both backends, then I'm comfortable proceeding. @mattBrzezinski would that be something you could test? |
Sorry I've been busy last couple days with other stuff. Going to look into these changes now, I will test w/ |
Code changes LGTM! Going to play around with this for a bit, see if anything breaks and will report back. |
Just tested AWSS3.jl with this branch, everything went well. I forgot that CWL.jl requires a stack setup, just made a dummy PR to test this functionality, here. EDIT: Julia1.3 is failing, as expected, rest looks fine. 👍 |
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.
These changes LGTM, not sure if @iamed2 has any other thoughts?
Thanks @mattBrzezinski; yeah, if there are other downstream dependents, feel free to ping me and I can make PRs to upgrade or help review. AWS.jl is a big one, so it should unblock a lot of others updating. |
test/runtests.jl
Outdated
# backwards compat | ||
function HTTP.StatusError(status, resp) | ||
return HTTP.StatusError(status, resp.request.method, resp.request.target, resp) | ||
end | ||
|
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 is bad piracy; this actually gets called by submit_request
as far as I can tell.
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.
Ah yes, forgot to clean this up.
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.
Pushed a clean up
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bors try |
tryBuild succeeded: |
Alright I'll tag #562 as a patch and then merge and tag this as a minor |
bors r+ |
@@ -129,7 +133,7 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers | |||
if HTTP.header(response, "Location") != "" | |||
request.url = HTTP.header(response, "Location") | |||
else | |||
e = HTTP.StatusError(response.status, response) | |||
e = statuserror(response.status, response) |
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.
I think this broke some things for the Downloads backend, because it assumes the response is coming from HTTP.jl and has method
and target
fields, whereas before it just wrapped the response, which could be coming from Downloads.jl and may not have those fields. I guess the AWS tests aren't enough to check this case, but I saw when adding Downloads.jl tests to AWSS3: JuliaCloud/AWSS3.jl#262. See https://github.com/JuliaCloud/AWSS3.jl/runs/7409333758?check_suite_focus=true#step:6:388.
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.
Can the DownloadsBackend-created HTTP.Response
that's returned just include the request here?
No description provided.