-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
User Agent 2.0 #2955
User Agent 2.0 #2955
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #2955 +/- ##
===========================================
+ Coverage 93.29% 93.32% +0.03%
===========================================
Files 64 65 +1
Lines 13588 13742 +154
===========================================
+ Hits 12677 12825 +148
- Misses 911 917 +6
☔ View full report in Codecov by Sentry. |
botocore/args.py
Outdated
@@ -66,13 +69,14 @@ def __init__( | |||
loader, | |||
exceptions_factory, | |||
config_store, | |||
user_agent_creator, |
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 think this needs to be optional or we may end up breaking current callers. That may change how we use the attribute below.
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.
bda23a9 is a refactor that prioritizes backwards compatible interfaces over everything (especially over avoiding duplication).
@@ -249,7 +254,6 @@ def compute_client_args( | |||
return { | |||
'service_name': service_name, | |||
'parameter_validation': parameter_validation, | |||
'user_agent': user_agent, |
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.
This may also be problematic, I don't immediately know how we do this cleanly though.
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.
We can bring back this line together with lines 196-203 above where its value gets computed using the legacy method. There is no harm in returning the user_agent
key in the dict, it simply won't get used by botocore code. Do you think that's preferable? I was worried the confusion caused by the dead code existing and an unused value being returned might outweigh any benefit to any third-party that may be relying on the user_agent
dict key being present.
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.
Looking at other libraries that may have issues with this, I can't find one that relies on user_agent. I agree this may cause unnecessary confusion, so let's opt to remove it.
tests/functional/test_useragent.py
Outdated
|
||
|
||
@contextmanager | ||
def uacap_client(client): |
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.
Does this give us a testing efficiency over the HTTPStubber?
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.
Somehow I couldn't figure out how to use HTTPStubber for this. Not sure if my requirements changed or I just got smarter since then, but your comment made me look at it again and it was straightforward. Done in eb7ded6.
tests/functional/test_useragent.py
Outdated
assert 'app/1234-' in uafields | ||
|
||
|
||
def test_boto3_behavior(patched_session, stubbed_list_buckets): |
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.
nit; We may want to be more specific than behavior.
def test_boto3_behavior(patched_session, stubbed_list_buckets): | |
def test_boto3_user_agent(patched_session, stubbed_list_buckets): |
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.
Done in eb7ded6
ua_client = ua_session.with_client_config(Config(...)) | ||
ua_string = ua_request.to_string() |
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.
There seems to be inconsistent naming here. Both should be ua_client
or ua_request
.
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 don't disagree with the naming being somewhat confusing. But I don't think your suggestions would clarify things. In fact your suggestions might be a sign that I successfully confused you!
ua_session
is aUserAgentString
object with session-level information only.ua_client
is aUserAgentString
object with session-level and client-level information.ua_string
is astr
.
tests/functional/test_useragent.py
Outdated
myclient.operation() | ||
print(cap_client.captured_ua_string) | ||
""" | ||
client.captured_ua_string = None |
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.
Would there be any value to add or replace this with a list that captures sequential ua_string
's?
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 ended up replacing this entire fixture in eb7ded6 to address another comment above. To answer the question though: I didn't see a reason to do more than one request in a single test because each request's UA is independent of the others.
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.
2 minor comments, otherwise I think this looks ready to go. Thanks @jonemo!
@@ -249,7 +254,6 @@ def compute_client_args( | |||
return { | |||
'service_name': service_name, | |||
'parameter_validation': parameter_validation, | |||
'user_agent': user_agent, |
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.
Looking at other libraries that may have issues with this, I can't find one that relies on user_agent. I agree this may cause unnecessary confusion, so let's opt to remove it.
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.
This reverts commit e5d52ae.
This PR implements an updated User-Agent header format that is standardized across AWS SDKs. For example, the AWS Ruby SDK's equivalent PR is aws/aws-sdk-ruby#2854.
An example User-Agent header value before the change is:
An example User-Agent header value after the change is:
Various notes:
AWS_SDK_UA_APP_ID
config: Support for sdk_ua_app_id config setting #2942Config.user_agent
will not see a change in their User-Agent header stringSession.user_agent_extra
andConfig.user_agent_extra
continue to be appear at the end of the User-Agent headerSession.user_agent()
is no longer used internally in Botocore but remains in place and unchanged for any third party users