-
Notifications
You must be signed in to change notification settings - Fork 180
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 missing features to user agent formatting #865
Conversation
A new generated diff is ready to view: __generated-main...__generated-jdisanti-user-agent |
A new generated diff is ready to view: __generated-main...__generated-jdisanti-user-agent |
1 similar comment
A new generated diff is ready to view: __generated-main...__generated-jdisanti-user-agent |
A new generated diff is ready to view: __generated-main...__generated-jdisanti-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.
Formatting and app name stuff looks good. We should probably switch to a builder for actually constructing the user agent, I'm sure it will have more fields in the future.
We'll also need some way of enabling the current use case for QLDB of setting fields directly, but we could use app name if we formatted it specially in User-Agent
(without the lib/
for now.
A new generated diff is ready to view: __generated-main...__generated-jdisanti-user-agent |
1 similar comment
A new generated diff is ready to view: __generated-main...__generated-jdisanti-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.
LGTM! thanks for cleaning up the docs
/// **Loads "my-app" as the app name** | ||
/// ```ini | ||
/// [default] | ||
/// sdk-ua-app-id = my-app |
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.
AFAICT, this isn't implemented in other SDKs yet. I'm not aware of any other kebab case properties, so we may want to try to get this changed.
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.
It looks like this has been implemented for the Javascript V3 SDK: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/modules/_aws_sdk_util_user_agent_node.html
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/UserAgentDecorator.kt
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view: __generated-main...__generated-jdisanti-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.
LGTM!
A new generated diff is ready to view: __generated-main...__generated-jdisanti-user-agent |
Description
This PR adds feature, config, framework, and app name formatting to user agents, and also adds the ability to set the app name from service config, shared config, profile config, and environment variables.
Testing
Checklist
CHANGELOG.md
aws/SDK_CHANGELOG.md
if applicableBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.