Fix S3 URL Handler for good #19285
Merged
Fix S3 URL Handler for good #19285
Conversation
This file contains 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
thejcannon
added
the
category:internal
huonw
approved these changes
Jun 9, 2023
There was a problem hiding this comment.
Yay! Approved because it supposedly works, but... I don't understand the moving parts, so I'm surprised that this is:
- importing from
httprather thanbotocore - no longer explicitly setting the header as previously suggested was necessary as a workaround
Could you clarify those?
I also suspect this needs cherry-picking tagging?
|
At first I was hesitant to use the botocore type, but I decided to go for it. For 2, now that we're using the correct type, we no longer are hitting the previous bug (the real type is mapping-like but not a dict. Using a dict made us need to workaround. Using the real type responds exactly as expected because, well, it's the real type). |
|
Makes sense, thanks for explaining! 👍 |
github-actions bot
pushed a commit
that referenced
this pull request
Jun 10, 2023
See https://github.com/boto/botocore/blob/0c833f28b1d87883c7aeb4aed8a05860af1ec34a/botocore/compat.py#L37 this type should work in all cases. I tried both locally and in my CI (token auth) Also UGH 😓
github-actions bot
pushed a commit
that referenced
this pull request
Jun 10, 2023
See https://github.com/boto/botocore/blob/0c833f28b1d87883c7aeb4aed8a05860af1ec34a/botocore/compat.py#L37 this type should work in all cases. I tried both locally and in my CI (token auth) Also UGH 😓
thejcannon
added a commit
that referenced
this pull request
Jun 10, 2023
See https://github.com/boto/botocore/blob/0c833f28b1d87883c7aeb4aed8a05860af1ec34a/botocore/compat.py#L37 this type should work in all cases. I tried both locally and in my CI (token auth) Also UGH 😓 Co-authored-by: Joshua Cannon <[email protected]>
thejcannon
added a commit
that referenced
this pull request
Jun 10, 2023
See https://github.com/boto/botocore/blob/0c833f28b1d87883c7aeb4aed8a05860af1ec34a/botocore/compat.py#L37 this type should work in all cases. I tried both locally and in my CI (token auth) Also UGH 😓 Co-authored-by: Joshua Cannon <[email protected]>
Merged
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.
See https://github.com/boto/botocore/blob/0c833f28b1d87883c7aeb4aed8a05860af1ec34a/botocore/compat.py#L37 this type should work in all cases.
I tried both locally and in my CI (token auth)
Also UGH 😓