Use code-generated endpoint rules for all services, and remove the customization. #5410
Conversation
…stomization.
Additional necessary changes to facilitate this:
1. Fixed an issue where any region parameter was assumed to be the endpoint region. The fix also removes the region as a local variable and instead relies on reading the region from the parameters. A "{name}Id" method has been added to the endpoint and auth resolver's region parameters, which allows readingthe region ID as a string.
2. Removed "auth scheme strategy" classes from code generated to just being in S3, because they're not used anywhere else, now. In the future, it would be nice to simplify this S3 logic.
| import software.amazon.awssdk.utils.CompletableFutureUtils; | ||
| import software.amazon.awssdk.utils.Logger; | ||
| import software.amazon.awssdk.utils.Validate; | ||
|
|
||
| public class EndpointProviderSpec implements ClassSpec { |
There was a problem hiding this comment.
It looks like there are a lot of changes here, but this is actually just EndpointProviderSpec2, which has already been code reviewed and is listed as a deletion below. You can skip reviewing this class if you want, since it wasn't changed.
There was a problem hiding this comment.
Looks like it wasnt merged in ? Would be great if you could provide a PR link
There was a problem hiding this comment.
https://github.com/aws/aws-sdk-java-v2/blame/9a3c99e5b70f608d20c18248aa9bdb5c8d98b684/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/EndpointProviderSpec2.java is the file already in the repo.
Original PR for it was db355d3
| * permissions and limitations under the License. | ||
| */ | ||
|
|
||
| package software.amazon.awssdk.services.s3.endpoints.internal; |
There was a problem hiding this comment.
These s3.endpoints.internal files look new, but they are just copies of the .java.resource files that used to be code generated. I just copied them into S3 because this is the only service that uses them.
You can skip reviewing these files if you want, because they were already reviewed. I didn't change them.
|
|
||
| SymbolTable(Builder builder) { | ||
| this.params = Collections.unmodifiableMap(new LinkedHashMap<>(builder.params)); | ||
| this.locals = Collections.unmodifiableMap(new LinkedHashMap<>(builder.locals)); | ||
| this.regionParamName = builder.regionParamName; | ||
| this.builtInParamTypes = Collections.unmodifiableMap(new LinkedHashMap<>(builder.builtInParamTypes)); |
There was a problem hiding this comment.
Do we need the overhead of maintaining a list with the LinkedHashMap ? I do not see us needing element ordering here in this class. Could we just do HashMap instead ?
There was a problem hiding this comment.
Seems reasonable. Let me see if there's any tests that assume order.
| .forEach((k, v) -> { | ||
| b.property(k, Value.fromNode(v)); | ||
| }); | ||
| .forEach((k, v) -> { |
There was a problem hiding this comment.
Do we want indentation changes like these to go through ? Feels like we have a lot to indent if we start looking into our entire codebase. Would it be better to leave them as they are ? It also shows up in the "git blame" section
There was a problem hiding this comment.
It just did that since it was a copy+paste of code, which reformats things automatically in Intellij. I didn't do it intentionally. I can try to revert these types of changes. Hopefully I don't miss any.
There was a problem hiding this comment.
I actually don't need all of these files! I've deleted a lot of them.
| .map(Arn::accountId) | ||
| .orElse(null); | ||
| RuleArn arn = RuleArn.parse(assumedRoleUser.arn()); | ||
| if (arn == null) { |
There was a problem hiding this comment.
nit : would be great to extract this into a method arnAccountId(RuleArn arn);
RuleArn arn = RuleArn.parse(assumedRoleUser.arn()); //or
RuleArn arn = RuleArn.parse(federatedUser.arn());
return arnAccountId(RuleArn arn);
There was a problem hiding this comment.
Since they're in different classes (so the code is harder to share) and there's only 2 places it's done like this (so the rule of three isn't satisfied), I'd usually leave it like this. I don't feel strongly about it, though, so LMK if you'd prefer it to be shared. I'd probably do a convenience method on RuleArn, like RuleArn.accountId(arn).
|
Additional necessary changes to facilitate this:
This PR is split between the
customization.configcleanups, and the codegen changes, to hopefully simplify the review.