TCP Keep-Alive Environment Variable Support #3579
Conversation
|
Tests and Configuration Documentation edit will be added. |
adev-code
force-pushed
the
tcp-keepalive
branch
from
e695aad to
0eb05d0
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3579 +/- ##
===========================================
- Coverage 93.17% 93.15% -0.02%
===========================================
Files 68 68
Lines 15411 15411
===========================================
- Hits 14359 14356 -3
- Misses 1052 1055 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Created a PR for Doc change to add |
|
Changelog added |
There was a problem hiding this comment.
Thanks for this PR Adrian!
I left two comments regarding backwards compatibility with the changes to _compute_socket_options and the testing bridge. Curious to see what Jonathan thinks.
Let's also make sure to merge this PR at the same time as the docs change in boto3.
| service_name, region_name, endpoint_url, is_secure | ||
| ) | ||
|
|
||
| def _compute_socket_options(self, scoped_config, client_config=None): |
There was a problem hiding this comment.
I know this is a private API so it shouldn't be used by anyone, but are we confident this will not break any customers?
I did a quick search and it doesn't look like we use this elsewhere in our repo.
There was a problem hiding this comment.
As per checking our code, the _compute_socket_options is only used in return statement of the function get_client_args(). Before, the scoped_keepalive checks the config file. Now it is taking care of tcp_keepalive as it also checks the config file. I suppose this should not break as the functionality of scoped_keepalive is now covered by tcp_keepalive.
tests/unit/test_args.py
Outdated
| } | ||
| call_kwargs.update(**override_kwargs) | ||
|
|
||
| # Bridge test's scoped_config to config_store for new _compute_socket_options implementation |
There was a problem hiding this comment.
I understand this bridge avoids modifying existing tests to keep backwards compatibility, but since we are breaking _compute_socket_options, it may make sense to refactor all the tcp keepalive tests to use config_store instead of scoped_config.
This can cause confusion for future developers that see a mismatch in what the functions accepts (which is only client_config) and what is used in tests (scoped_config). They may lack the historical context to understand why this bridge is necessary.
Other tests in this file useconfig_store.set_config_variable to set the config value, we should be able to use this here too.
There was a problem hiding this comment.
Yes that make sense. I will refactor (next commit) all the TCP keepalive tests to use config_store.set_config_variable() directly and removed the bridge entirely. The tests are now consistent with the rest and match what _compute_socket_options actually expects. Thanks for pointing this out as it may cause confusion in the future for developers.
3 participants
Currently, TCP Keep-Alive can be configured using
Configobjectand
~/.aws/configfile as per Configuration Doc.This PR adds environment variable support via
BOTOCORE_TCP_KEEPALIVE.Example Usage:
export BOTOCORE_TCP_KEEPALIVE=True or true
or
os.environ['BOTOCORE_TCP_KEEPALIVE'] = 'True' or 'true'
Addresses: #2268