-
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
update user agent to 2.1 #3238
base: main
Are you sure you want to change the base?
update user agent to 2.1 #3238
Conversation
@@ -48,7 +48,7 @@ Aws::String ComputeOSVersionString() | |||
if(success >= 0) | |||
{ | |||
Aws::StringStream ss; | |||
ss << name.sysname << "/" << name.release; |
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 was actually broken before too
os/Darwin/23.6.0
-> os/Darwin#23.6.0
which is actually the correct format and actually how CLI has it in their user agent string
aws-cli/2.16.5
md/awscrt#0.20.11
ua/2.0
os/linux#5.10.230-202.885.amzn2int.x86_64
md/arch#x86_64
lang/python#3.11.8
md/pyimpl#CPython
cfg/retry-mode#standard
md/installer#exe
md/distrib#amzn.2
md/prompt#off
md/command#ec2.describe-instances
eb6bad6
to
8c6d6f6
Compare
src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h
Outdated
Show resolved
Hide resolved
src/aws-cpp-sdk-core/include/smithy/client/features/UserAgentInterceptor.h
Outdated
Show resolved
Hide resolved
8c6d6f6
to
d619b4c
Compare
m_retryStrategyName{retryStrategyName}, | ||
m_execEnv{FilterUserAgentToken(Aws::Environment::GetEnv(EXEC_ENV_VAR).c_str())}, | ||
m_appId{FilterUserAgentToken(clientConfiguration.appId.c_str())} | ||
#if defined(AWS_USER_AGENT_CUSTOMIZATION) |
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.
personally I would not use macros unless necessary
an alternative perhaps . Not a blocker just food for thought
#if defined(AWS_USER_AGENT_CUSTOMIZATION) constexpr const char* awsUserAgentCustomization = AWS_USER_AGENT_CUSTOMIZATION; #else constexpr const char* awsUserAgentCustomization = ""; #endif
and then ,
m_customizations{FilterUserAgentToken(awsUserAgentCustomization)}
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 is copied existing code, that we cannot remove for backwards compatible reasons.
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.
constexpr const char* awsUserAgentCustomization = AWS_USER_AGENT_CUSTOMIZATION;
won't compile because, considering macro defined as -DmyCustomUserAgentToken
, will be substituted by preprocessor to
constexpr const char* awsUserAgentCustomization = myCustomUserAgentToken; /* not a string literal! */
double macro is a standard approach here to convert macro value to a string literal.
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 was going by the existing definition which is quoted. Without that yes it will fail. Yes, usually we do double macro for cmdline flags . this was just food for thought for the internal ones that we set up /control as strings like this in cmake
set(AWS_USER_AGENT_CUSTOMIZATION "" CACHE STRING "User agent extension")
src/aws-cpp-sdk-core/include/smithy/client/features/UserAgentInterceptor.h
Show resolved
Hide resolved
d619b4c
to
107f3ed
Compare
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.
Approving with comments on empty str checks , const correctness, copy constructibility . Functionally looks ok to me. Revisions look good
107f3ed
to
a85fae9
Compare
Description of changes:
Updates the user agent string to be compliant with the 2.1 revision. all SDKs will eventually update to this version. For example go v2 already has in this commit.
example before:
example after:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.