Botocore timeout #629
Botocore timeout #629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
+ Coverage 94.96% 94.97% +0.01%
==========================================
Files 18 18
Lines 3236 3243 +7
Branches 421 422 +1
==========================================
+ Hits 3073 3080 +7
Misses 110 110
Partials 53 53
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good, just had a suggestion on how you could simplify the code by setting a default client config on the Session object.
EDIT: Looks like I didn't save my previous comment about the default client config. Expanding on my comment above, the botocore.Session has a set_default_client_config() method that allows you to set a default botocore.Config to use for all Session.create_client() calls. What I think we can do to simplify is just update the create_botocore_session() method in the cli factory to accept a new connection_timeout param, which if provided will create a botocore.Config() object and call set_default_client_config() on the Session. That should also mean you won't have to update awsclient.py with the botocore_config object because the session will already be configured with a default client config.
chalice/cli/__init__.py
Outdated
| @@ -121,17 +121,27 @@ def run_local_server(factory, host, port, stage, env): | |||
| help=('Name of the Chalice stage to deploy to. ' | |||
| 'Specifying a new chalice stage will create ' | |||
| 'an entirely new set of AWS resources.')) | |||
| @click.option('--botocore-timeout', | |||
There was a problem hiding this comment.
I'd prefer we just call this --connection-timeout.
|
Hey @jamesls, I made all of the requested changes. Thanks for the review, also good catch on the |
PR #629 * nplutt-botocore-timeoue: Add --connection-timeout to deploy command
Enhancement for issue #344.
Let me know if you would like me to change anything. I decided to go with the option of consuming the timeout value through the cli but I could also see if you want this to be consumed through the config file.