-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Bump botocore
dependency specification
#1221
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1221 +/- ##
==========================================
- Coverage 86.16% 86.05% -0.12%
==========================================
Files 64 64
Lines 5986 5995 +9
==========================================
+ Hits 5158 5159 +1
- Misses 828 836 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6eb27a2
to
04135d5
Compare
Notes to reviewers:
|
7da78cb
to
de1faf9
Compare
ya I'm not sure if |
I think it's populated here: aiobotocore/aiobotocore/endpoint.py Lines 57 to 65 in 8a50b61
|
Exactly: Case 1 and 4 result in bytes, case 3 cannot be reached in this context, but case 2 on line 60 is tricky. I have no idea under what condition this is reached, if at all, and whether it results in something that needs to be awaited. Beats me! |
I think here's an example: https://botocore.amazonaws.com/v1/documentation/api/latest/reference/eventstream.html |
isn't line 63 also a coroutine? That's like a s3 get-object body i think |
ideally we could add tests for this with moto |
This comment was marked as resolved.
This comment was marked as resolved.
There's a guard function |
This comment was marked as resolved.
This comment was marked as resolved.
I could give it a shot towards the end of the week. Thanks for looking into it! |
Actually, I am suspecting that this guard function also prevents case 2 / line 60 from being reached. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
de1faf9
to
12b7fc3
Compare
I've included all pending botocore updates into this PR, including support for DSQL auth and all the other new and updated AWS services. We still need include unit tests, in particular with regards to the issue discussed above. Hopefully this weekend... |
Well, that example doesn't work for me: AWS no longer accepts new users for S3 Select. I will need to find another service that uses event streams. |
looks like bedrock supports eventstreams as well: https://docs.aws.amazon.com/bedrock/latest/userguide/agents-test.html |
oh duh, we should just port their tests, lemme c if I can do it |
@jakob-keller see latest change to #1232. Wasn't sure how to push to your branch. If you give me rights can push there or feel free to cherry pick it |
seems like it's fairly easy to pull their tests in now, perhaps we should try to pull the whole thing over |
That sounds fantastic, thanks! I have to admit I'm still not familiar with our test suite. Wouldn't it make sense to separate your test related work into its own PR and then revisit this PR? |
Description of Change
This PR intends to improve general compatibility of
aiobotocore
within the Python ecosystem by bumping the dependency specification ofbotocore
, as well asboto3
andawscli
.Assumptions
Upstream diff contains changes that require adjustments to the aiobotocore codebase.
Checklist for All Submissions
botocore
dependency #1229Checklist when updating botocore and/or aiohttp versions