fix: account ID based endpoints #1535
Conversation
0marperez
added
the
no-changelog
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
| @@ -106,6 +106,14 @@ fun renderBindAwsBuiltins(ctx: ProtocolGenerator.GenerationContext, writer: Kotl | |||
| AwsRuntimeTypes.Config.Endpoints.resolveAccountId, | |||
| AccountIdEndpointBuiltinCustomization.AccountIdEndpointModeProp.propertyName, | |||
| ) | |||
|
|
|||
| AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> { | |||
There was a problem hiding this comment.
The comment at the top of this function says "In practice, all of these values are sourced from the client config."
Is that still true? Isn't AccountId being sourced exclusively from endpoint rules in your manual test cases?
There was a problem hiding this comment.
Is that still true? Isn't AccountId being sourced exclusively from endpoint rules in your manual test cases?
Account ID isn't being sourced exclusively from endpoint rules. It's sourced from client config as a built in and from the operation context params using JMESPath.
The endpoint rules use both values for different conditions
|
|
||
| AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> { | ||
| writer.write( | ||
| "#L = config.#L.toString().lowercase()", // DDB endpoint rules assume lowercase value |
There was a problem hiding this comment.
Remove comment about DDB since this rule applies for all AWS endpoints
|
|
||
| AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> { | ||
| writer.write( | ||
| "#L = config.#L.toString().lowercase()", // DDB endpoint rules assume lowercase value |
There was a problem hiding this comment.
Nit: DDB isn't adding any special constraints here. The spec says:
The value will be propagated to endpoint rules as the built-in endpoint parameter
AWS::Auth::AccountIdEndpointMode. This setting is an enum with the valuesrequired,disabledandpreferred.
We should tweak this comment so that we don't get confused later about which services require lowercase values.
There was a problem hiding this comment.
I think we can just remove the comment in that case
There was a problem hiding this comment.
I think it's fine to mention that the enum values are expected to be lowercase
There was a problem hiding this comment.
Especially since it's different from how we treat basically all other enums in Kotlin.
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
|
|
A new generated diff is ready to view. |
Issue #
N/A
Description of changes
Routes
accountIdEndpointModeto AWS built in endpoint parametersBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.