fix: account ID based endpoints #1535
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0marperez
added
the
no-changelog
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
lauzadis
approved these changes
Feb 20, 2025
| AccountIdEndpointBuiltinCustomization.AccountIdEndpointModeProp.propertyName, | ||
| ) | ||
|
|
||
| AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> { |
Member
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?
Contributor
Author
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 |
Member
There was a problem hiding this comment.
Remove comment about DDB since this rule applies for all AWS endpoints
ianbotsf
requested changes
Feb 20, 2025
Contributor
ianbotsf
left a comment
ianbotsf
left a comment
There was a problem hiding this comment.
Correctness: Please add unit test(s)
|
|
||
| AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> { | ||
| writer.write( | ||
| "#L = config.#L.toString().lowercase()", // DDB endpoint rules assume lowercase value |
Contributor
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.
Contributor
Author
There was a problem hiding this comment.
I think we can just remove the comment in that case
Member
There was a problem hiding this comment.
I think it's fine to mention that the enum values are expected to be lowercase
Contributor
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. |
This comment has been minimized.
This comment has been minimized.
ianbotsf
approved these changes
Feb 21, 2025
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.