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

Develop #1157

Merged
merged 2 commits into from Dec 21, 2012

Conversation

Projects
None yet
4 participants
@jimfulton
Contributor

jimfulton commented Dec 3, 2012

This pull request provides keyring integration.

See` #1154

@@ -246,6 +246,11 @@ def get_credentials(self, access_key=None, secret_key=None):
self.secret_key = os.environ[secret_key_name.upper()]
elif config.has_option('Credentials', secret_key_name):
self.secret_key = config.get('Credentials', secret_key_name)
elif config.has_option('Credentials', 'keyring'):
keyring_name = config.get('Credentials', 'keyring')
import keyring

This comment has been minimized.

@garnaat

garnaat Dec 3, 2012

Member

I think we should either add keyring as a dependency in setup.py or put a try/except block here to give the user a readable warning message. Thoughts?

@garnaat

garnaat Dec 3, 2012

Member

I think we should either add keyring as a dependency in setup.py or put a try/except block here to give the user a readable warning message. Thoughts?

This comment has been minimized.

@kopertop

kopertop Dec 4, 2012

Member

I usually prefer to do a try/catch so its optional.


Sent from my iPhone
Chris Moyer

On Dec 3, 2012, at 5:50 PM, Mitch Garnaat notifications@github.com wrote:

In boto/provider.py:

@@ -246,6 +246,11 @@ def get_credentials(self, access_key=None, secret_key=None):
self.secret_key = os.environ[secret_key_name.upper()]
elif config.has_option('Credentials', secret_key_name):
self.secret_key = config.get('Credentials', secret_key_name)

  •    elif config.has_option('Credentials', 'keyring'):
    
  •        keyring_name = config.get('Credentials', 'keyring')
    
  •        import keyring
    
    I think we should either add keyring as a dependency in setup.py or put a try/except block here to give the user a readable warning message. Thoughts?


Reply to this email directly or view it on GitHub.

@kopertop

kopertop Dec 4, 2012

Member

I usually prefer to do a try/catch so its optional.


Sent from my iPhone
Chris Moyer

On Dec 3, 2012, at 5:50 PM, Mitch Garnaat notifications@github.com wrote:

In boto/provider.py:

@@ -246,6 +246,11 @@ def get_credentials(self, access_key=None, secret_key=None):
self.secret_key = os.environ[secret_key_name.upper()]
elif config.has_option('Credentials', secret_key_name):
self.secret_key = config.get('Credentials', secret_key_name)

  •    elif config.has_option('Credentials', 'keyring'):
    
  •        keyring_name = config.get('Credentials', 'keyring')
    
  •        import keyring
    
    I think we should either add keyring as a dependency in setup.py or put a try/except block here to give the user a readable warning message. Thoughts?


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@jimfulton

jimfulton Dec 4, 2012

Contributor

A warning would be good. Given the technical audience of boto and the fact that this wouldn't come into play unless someone added a keyring option to their configuration, I thought the import error would be pretty clear, but I'm happy to add a try/catch with a message.

Whether you add a dependency is up to you. Lots of people get cranky about additional dependencies.

Let me know which you prefer.

@jimfulton

jimfulton Dec 4, 2012

Contributor

A warning would be good. Given the technical audience of boto and the fact that this wouldn't come into play unless someone added a keyring option to their configuration, I thought the import error would be pretty clear, but I'm happy to add a try/catch with a message.

Whether you add a dependency is up to you. Lots of people get cranky about additional dependencies.

Let me know which you prefer.

This comment has been minimized.

@jamesls

jamesls Dec 21, 2012

Member

I think that's reasonable. One of the changes I've added to the provider module was to log where the credentials are coming from, so I went ahead and added log messages for keyring credentials as well. I also added a log message when the keyring module can't be imported, but still let the ImportError propogate.

Thanks for the pull request.

@jamesls

jamesls Dec 21, 2012

Member

I think that's reasonable. One of the changes I've added to the provider module was to log where the credentials are coming from, so I went ahead and added log messages for keyring credentials as well. I also added a log message when the keyring module can't be imported, but still let the ImportError propogate.

Thanks for the pull request.

@jamesls jamesls merged commit 86c0e28 into boto:develop Dec 21, 2012

@sopel sopel referenced this pull request Dec 9, 2013

Closed

Keyring support in botocore #72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment