Add support for endpoint_url, user_agent_extra, and sdk_ua_app_id.#653
Add support for endpoint_url, user_agent_extra, and sdk_ua_app_id.#653ubaskota wants to merge 2 commits intosmithy-lang:config_resolution_mainfrom
Conversation
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
Had a few comments. I also noticed there are no validators for any of these options. I know at least the user agent app ID will need it:
Valid values: String with maximum length of 50. Letters, numbers and the following special characters are allowed: !,#,$,%,&,',*,+,-,.,^,_,`,|,~.
| .build(); | ||
|
|
||
| public static final ConfigProperty USER_AGENT_EXTRA = ConfigProperty.builder() | ||
| .name("user_agent_extra") |
There was a problem hiding this comment.
Why was this config option added? This config option isn't standardized across all SDKs (source). By adding it here we're now reading AWS_USER_AGENT_EXTRA from the environment which we shouldn't be doing.
| .documentation("A unique and opaque application ID that is appended to the User-Agent header.") | ||
| .nullable(false) | ||
| .useDescriptor(true) | ||
| .defaultValue("''") |
There was a problem hiding this comment.
Is there any limitation with keeping the default to None? This feels weird to me.
There was a problem hiding this comment.
I thought users would be able to add it without first checking if the value is None if the default is an empty string. But I see why returning None would be a better choice. I will update it.
| .defaultValue("''") | ||
| .build(); | ||
|
|
||
| public static final ConfigProperty ENDPOINT_URL = ConfigProperty.builder() |
There was a problem hiding this comment.
I haven't tested but it looks like we now have endpoint_uri and endpoint_url. Since there are not other changes, if someone sets export AWS_ENDPOINT_URL="...", then config.endpoint_url will resolve correctly, but it won't actually be used by the SDK since we're using endpoint_uri still.
This one is a bit more complicated since smithy python stuff uses endpoint_uri. Can we leave the existing endpoint_uri but map to endpoint_url config options?
| .build()) | ||
| .documentation("The retry strategy or options for configuring retry behavior. Can be either a configured RetryStrategy or RetryStrategyOptions to create one.") | ||
| .build(), | ||
| ConfigProperty.builder() |
There was a problem hiding this comment.
This will add all of these to generic clients; can we keep them as AWS specific for now?
| .type(Symbol.builder().name("str").build()) | ||
| .documentation("A unique and opaque application ID that is appended to the User-Agent header.") | ||
| .build(), | ||
| ConfigProperty.builder() |
There was a problem hiding this comment.
endpoint_url and endpoint_uri are effectively the same. We should address this in the long run since the SDKs have standardized on endpoint_url, but we currently only accept endpoint_uri.
For now, let's just remove the duplicate endpoint_url.
ubaskota
force-pushed
the
new_config_vars
branch
from
36f41f8 to
7146d45
Compare
3 participants
Issue #, if available:
Description of changes:
Add codegen support for three new config properties:
endpoint_url,user_agent_extra, andsdk_ua_app_id. For AWS SDKs, these are generated as descriptor-based properties with config resolver and default values (empty string for the user agent properties, None for endpoint_url). For generic SDKs, they are generated as plain properties without descriptors or resolution logic, consistent with how retry_strategy is handled.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.