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

Add retry and timeout as params to retrieving metadata service #202

Merged
merged 2 commits into from
Jan 9, 2014

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Jan 8, 2014

Fixes #198.

I did not add any config vars to the session to set these default values. However, I think it makes sense to add this (similar to how you can set these values in a boto config file). I'd like to send that as a separate pull request.

This only updates the low level metadata retrieval code so that it can be parameterized the number of retries and the connection timeouts.

The default has been changed to 1, which is now consistent with
boto and other AWS SDKs.
Note that this does not plumb anything into the session via
config vars.  I think that should be a separate pull request.
@@ -310,7 +310,7 @@ def set_credentials(self, access_key, secret_key, token=None):
token)
self._credentials.method = 'explicit'

def get_credentials(self, metadata=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to worry about back-compat here (from removing a parameter)? Would anyone else be calling this or is it (hopefully) largely just internal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Internal, especially given the "This is mainly used for unit testing." in the docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I didn't read the whole docstring you were removing. Shipit stands.

@toastdriven
Copy link
Contributor

LGTM otherwise. :shipit:

@garnaat
Copy link
Member

garnaat commented Jan 8, 2014

Would definitely like to see these pulled out as config vars but I'm fine with doing that as a separate PR. LGTM

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.

3 participants