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

Support for sdk_ua_app_id config setting #2942

Closed
wants to merge 3 commits into from

Conversation

jonemo
Copy link
Contributor

@jonemo jonemo commented May 16, 2023

This PR adds support for the AWS_SDK_UA_APP_ID environment variable and sdk_ua_app_id setting in the shared configuration file. This is already supported by some other AWS SDKs today and will in future be the recommended way for third party apps that use botocore to inject an identifier into the User-Agent header. Botocore will also continue supporting the user_agent and user_agent_extra config settings for backwards compatibility.

For test coverage, this PR introduces new functional testing for all user agent header related config settings. While there are existing tests for user agent settings in unit/test_client.py, unit/test_session.py, and unit/test_session_legacy.py, they either test session-level settings (which get overridden or modified by client-level config) or mock the legacy endpoint resolver.

Open questions where for which I'd be particularly interested to hear feedback:

  1. Naming of the config variable. Right now I made the config variable name similar to existing config variables but different from the config file setting name.
  2. Test fixtures: I wonder if there is an easier way to stub the requests that requires less setup and simply captures all outgoing requests. Probably not because it would require somehow aborting the request before the response is parsed...

@jonemo jonemo requested a review from nateprewitt May 16, 2023 20:59
:param user_agent_appid: A value that gets included in the User-Agent
string in the format "app/<user_agent_appid>" which is consistent
across all AWS SDKs. Allowed characters are ASCII alphanumerics and
``!$%&'*+-.^_`|~``. All other characters will be replaced by a ``-``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we enforcing this currently?

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8bd1a4d) 93.29% compared to head (f3d1452) 93.30%.

❗ Current head f3d1452 differs from pull request most recent head e3efab1. Consider uploading reports for the commit e3efab1 to get more accurate results

❗ 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    #2942   +/-   ##
========================================
  Coverage    93.29%   93.30%           
========================================
  Files           64       65    +1     
  Lines        13588    13600   +12     
========================================
+ Hits         12677    12689   +12     
  Misses         911      911           
Impacted Files Coverage Δ
botocore/config.py 100.00% <ø> (ø)
botocore/configprovider.py 94.09% <ø> (ø)
botocore/args.py 100.00% <100.00%> (ø)
botocore/useragent.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jonemo
Copy link
Contributor Author

jonemo commented May 17, 2023

Closing this in favor of a larger PR with more user agent header changes. Just too many interconnected pieces to cleanly separate this into smaller pieces...

@jonemo jonemo closed this May 17, 2023
@jonemo jonemo mentioned this pull request May 26, 2023
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