Skip to content
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 to 1.29.76 #999

Merged
merged 2 commits into from
Mar 7, 2023
Merged

bump botocore to 1.29.76 #999

merged 2 commits into from
Mar 7, 2023

Conversation

jakob-keller
Copy link
Collaborator

@jakob-keller jakob-keller commented Feb 22, 2023

Description of Change

The botocore dependency has not been updated in a while, causing a number of downstream issues and incompatibilities.

Assumptions

Diff between previous and new botocore sources: boto/botocore@1.27.59...1.29.76

Checklist for All Submissions

Checklist when updating botocore and/or aiohttp versions

  • I have read and followed CONTRIBUTING.rst
  • I have updated test_patches.py where/if appropriate (also check if no changes necessary)
  • I have ensured that the awscli/boto3 versions match the updated botocore version

@jakob-keller jakob-keller mentioned this pull request Feb 22, 2023
@jakob-keller jakob-keller marked this pull request as ready for review February 22, 2023 14:20
@thehesiod
Copy link
Collaborator

see work I started in #974, we need to research new codepath: SSOTokenProvider._refresher

@thehesiod
Copy link
Collaborator

oh, I see something along those lines, does this PR account for that?

@jakob-keller

This comment was marked as outdated.

@jakob-keller
Copy link
Collaborator Author

oh, I see something along those lines, does this PR account for that?

I just pushed a commit that supports async token refresh. Please note that this is currently untested. Is anyone able to give it a try and/or suggest unit tests?

@thehesiod
Copy link
Collaborator

aha, @jakob-keller you da man. Ok for stuff like this I typically pulled tests from botocore. You can see some of our adaptations here: https://github.com/aio-libs/aiobotocore/tree/master/tests/boto_tests. where possible keeping the tests as similar as possible

@jakob-keller
Copy link
Collaborator Author

aha, @jakob-keller you da man. Ok for stuff like this I typically pulled tests from botocore. You can see some of our adaptations here: https://github.com/aio-libs/aiobotocore/tree/master/tests/boto_tests. where possible keeping the tests as similar as possible

OK, I will give it a shot.

I just discovered another new blocking codepath in regions.py. Will address that, too.

@thehesiod
Copy link
Collaborator

oops, forgot probably easier doing it in https://github.com/aio-libs/aiobotocore/tree/master/tests/python3.8/boto_tests, no reason to make it 3.6 compatible

@jakob-keller
Copy link
Collaborator Author

OK, I addressed the failing unit tests and the second blocking codepath. It's working locally on my machine. Could you trigger CI again, please?

I haven't gotten around to adding new tests yet. Will try to do that in the coming days.

@jakob-keller
Copy link
Collaborator Author

@thehesiod Could you trigger CI again, please? I want to make sure existing tests are fine before I add anything new. Thanks!

@thehesiod
Copy link
Collaborator

@jakob-keller thanks again, sorry for delay, work + fam have been non-stop.

@jakob-keller
Copy link
Collaborator Author

OK, I had a look at the unit tests from upstream and added and/or modified those that I believe are relevant.

@thehesiod: Could you please run CI again to check if there are any remaining issues?

@jakob-keller
Copy link
Collaborator Author

Well it looks like test coverage is subpar. Is that a hard requirement? I am not terribly familiar with the test suits of this repo nor upstream, which makes me rather doubtful that I can contribute much more without specific guidance. Any input? Thanks!

@thehesiod
Copy link
Collaborator

I think the tests may not be running because they're not marked moto: https://github.com/aio-libs/aiobotocore/blob/master/tests/test_monitor.py#L6. Right now we only run moto tests: https://github.com/aio-libs/aiobotocore/blob/master/Makefile#L25 you can run the tests locally as well to check

@tibbe
Copy link

tibbe commented Mar 2, 2023

I would love to see this. I'm trying to decide whether to use aioboto3 in my project and it being ~7 months behind boto3 worries me a bit. For example, if there's a critical bug fix to boto3 and it takes long for it to be picked up by aioboto3 that would be a cause for concern.

I of course understand this is open source and people have limited time. Just wanted to share my thoughts of adopting this package as a potential user.

@jakob-keller
Copy link
Collaborator Author

I think the tests may not be running because they're not marked moto: https://github.com/aio-libs/aiobotocore/blob/master/tests/test_monitor.py#L6. Right now we only run moto tests: https://github.com/aio-libs/aiobotocore/blob/master/Makefile#L25 you can run the tests locally as well to check

OK, I believe it's ready for another spin. Awaiting CI run...

@jakob-keller
Copy link
Collaborator Author

Well, this time, I believe we are pretty close. Anything else I can do to move this forward?

@aio-libs aio-libs deleted a comment from codecov bot Mar 4, 2023
@jakob-keller
Copy link
Collaborator Author

@thehesiod What do you think about the state of this PR? Anything else that I can contribute or need to address?

@thehesiod
Copy link
Collaborator

@jakob-keller I think it's great, just need a sec to look over it

@thehesiod
Copy link
Collaborator

looking now

@@ -167,6 +171,7 @@ async def _create_client(
)
else:
credentials = await self.get_credentials()
auth_token = self.get_auth_token()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work piping this through!

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #999 (36940a0) into master (22a1a4a) will decrease coverage by 15.55%.
The diff coverage is 37.86%.

❗ Current head 36940a0 differs from pull request most recent head 3c8ee05. Consider uploading reports for the commit 3c8ee05 to get more accurate results

@@             Coverage Diff             @@
##           master     #999       +/-   ##
===========================================
- Coverage   86.86%   71.32%   -15.55%     
===========================================
  Files          55       55               
  Lines        5391     5287      -104     
===========================================
- Hits         4683     3771      -912     
- Misses        708     1516      +808     
Flag Coverage Δ
unittests 71.32% <37.86%> (-15.55%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/test_mturk.py 44.44% <0.00%> (-5.56%) ⬇️
aiobotocore/tokens.py 26.13% <26.13%> (ø)
aiobotocore/signers.py 54.48% <31.57%> (-40.69%) ⬇️
aiobotocore/utils.py 47.22% <32.72%> (-29.45%) ⬇️
aiobotocore/credentials.py 80.73% <62.50%> (-8.04%) ⬇️
aiobotocore/httpchecksum.py 65.47% <66.66%> (-0.38%) ⬇️
aiobotocore/client.py 84.29% <70.58%> (-1.98%) ⬇️
aiobotocore/args.py 100.00% <100.00%> (ø)
aiobotocore/session.py 77.45% <100.00%> (+0.92%) ⬆️
tests/boto_tests/test_credentials.py 96.86% <100.00%> (-0.38%) ⬇️
... and 35 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thehesiod
Copy link
Collaborator

looks great! Thanks so much for all your hard work! This was one of the more complicated merges. I'll get this out today

@thehesiod
Copy link
Collaborator

we should point to this PR why bumping is not so simple :)

@thehesiod
Copy link
Collaborator

I would love to see this. I'm trying to decide whether to use aioboto3 in my project and it being ~7 months behind boto3 worries me a bit. For example, if there's a critical bug fix to boto3 and it takes long for it to be picked up by aioboto3 that would be a cause for concern.

I of course understand this is open source and people have limited time. Just wanted to share my thoughts of adopting this package as a potential user.

ya I have limited time now with kids and start-up life, have any other volunteers for help with getting releases out sooner? This one took longer than expected due to the complications you can see from this PR. They added several new codepaths that needed to be async'ified

@thehesiod thehesiod merged commit 72b8dd5 into aio-libs:master Mar 7, 2023
@jakob-keller jakob-keller deleted the bump-botocore branch March 7, 2023 06:05
@jakob-keller
Copy link
Collaborator Author

ya I have limited time now with kids and start-up life

I feel you!

have any other volunteers for help with getting releases out sooner?

Count me in

@paked
Copy link

paked commented Mar 8, 2023

Really appreciate the work here guys. Thanks so much!

@martindurant
Copy link

Is a release planned with this or more recent botocore dependency? s3fs is being hit by the large number of releases botocore has had since the last 2.5.0 dask/dask#10276

@jakob-keller
Copy link
Collaborator Author

Is a release planned with this or more recent botocore dependency?

This has been released as aiobotocore 2.5.0 on Mar 7, 2023. Which minimum release of botocore would you require?

@martindurant
Copy link

Er, enough to convince conda that downgrading boto3 is preferable to downgrading s3fs by several years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants