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

Allow keepalive_timeout to be None #933

Conversation

dnlserrano
Copy link

@dnlserrano dnlserrano commented May 3, 2022

There was a bug in the library in that we could not both:

  • set keepalive_timeout as None
  • set force_close to True

Validation was wrong in that it expects keepalive_timeout to have to be
one of float or int, but it should also allow None since aiohttp
requires keepalive_timeout to be None if force_close is set to
True.

Closes #932

Description of Change

Started to allow None as value for keepalive_timeout. Added tests accordingly.

Assumptions

aiohttp requires keepalive_timeout to be None when force_close is True, so assumed validation in aiobotocore needed to reflect that.

Checklist for All Submissions

Closes #932

There was a bug in the library in that we could not both:

  * set keepalive_timeout as None
  * set force_close to True

Validation was wrong in that it expects `keepalive_timeout` to have to be
one of `float` or `int`, but it should also allow `None` since `aiohttp`
requires `keepalive_timeout` to be `None` if `force_close` is set to
`True`.

Closes aio-libs#932
@thehesiod thehesiod self-requested a review May 5, 2022 18:47
@@ -1,5 +1,9 @@
Changes
-------
2.2.1 (2022-05-03)
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to bump to today but I can change later

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, forgot to update version in __init__.py

Copy link
Author

Choose a reason for hiding this comment

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

Shall I leave it to you to fix @thehesiod or what do you need me to do? 😄 Thanks in advance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll roll this into my current PR

@thehesiod
Copy link
Collaborator

ty!

thehesiod added a commit that referenced this pull request May 6, 2022
@thehesiod
Copy link
Collaborator

rolled into #935, thanks!

@thehesiod thehesiod closed this May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set both keepalive_timeout to None and force_close to True
2 participants