-
-
Notifications
You must be signed in to change notification settings - Fork 92
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 ability to pass parameters to the botocore session. #52
base: master
Are you sure you want to change the base?
Add ability to pass parameters to the botocore session. #52
Conversation
aws_requests_auth/boto_utils.py
Outdated
@@ -31,7 +31,7 @@ def get_credentials(credentials_obj=None): | |||
|
|||
class BotoAWSRequestsAuth(AWSRequestsAuth): | |||
|
|||
def __init__(self, aws_host, aws_region, aws_service): | |||
def __init__(self, aws_host, aws_region, aws_service, session_params={}): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the pull request @dashstander . Mutable default kwargs are a bit of a Python gotcha e.g.
In [1]: def f(l=[]):
...: l.append(1)
...: print(l)
...:
In [2]: f()
[1]
In [3]: f()
[1, 1]
In [4]: f()
[1, 1, 1]
In [5]: f()
[1, 1, 1, 1]
In [6]: f()
[1, 1, 1, 1, 1]
The preferred pattern (as described by core dev Raymond Hettinger) looks something like...
def f(l=None):
if l is None:
l = []
allows things like passing in regions, profiles, etc.
Since aws_region
is "hardcoded" in BotoAwsRequestsAuth
and AWSRequestsAuth
, I do not think aws_region
will be an appropriate target for us to dynamically override.
I'm not sure if this is true, but does the boto3
profile_name
kwarg (e.g. boto3.session.Session(profile_name="my-profile")
) restrict itself to only affectingaws_access_key_id
, aws_secret_access_key
, aws_session_token
?
If profile_name
indeed only can possibly affect auth, then we might consider allowing profile_name
(and only profile_name
) to be optionally provided instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the response @DavidMuller , this is the first time I've made a PR against a public repo.
Having the default be mutable was definitely an oversight on my part and I can change the PR.
According to the docs I'm looking at, there are other parameters (including aws_region
) that can have defaults configured by different profiles. But I don't think that that would interfere with having it aws_region
hardcoded. The Session().get_credentials()
call won't ever pull anything except the aws_access_key_id
, aws_secret_access_key
, aws_session_token
, so there's no danger of the aws_region
getting overridden or anything like that.
Which is all another way of saying that adding an optional profile_name
parameter works for me as well.
Would benefit from this getting merged and released at some point. What is left? I think I like using kwarg |
I do not have plans for a new release soon - my company has stopped using this package. In the meantime, a workaround is to use the class MyAuth(AWSRequestsAuth):
def get_aws_request_headers_handler(self, r):
return self.get_aws_request_headers(
r=r,
aws_access_key=# any code you like,
aws_secret_access_key=# any code you like,
aws_token=# any code you like,
) |
Sounds good and thanks @DavidMuller. If anyone else is looking into this I also did |
@ethan-deng Will this be merged if you've approved it? seems like a helpful addition |
Adds
session_params
parameter toBotoAwsRequestsAuth
that gets unpacked into the botocore session object. This allows things like passing in regions, profiles, etc... that are outside the default way botocore picks credentials. Solves #51