Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Nov 16, 2021

Issue #

fix #423

Description of changes

Ensure a default endpoint resolver is still registered for machine learning. The custom resolver for predict now runs after the default resolver to override the endpoint. We don't currently have a way to replace/override middleware per/operation in codegen.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-machine-learning

Copy link
Contributor

@kggilmer kggilmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you briefly mention testing steps here as I believe we don't have test coverage for this?

super
.customizeMiddleware(ctx, resolved)
.replace(endpointResolverMiddleware) { it is ResolveAwsEndpointMiddleware }
): List<ProtocolMiddleware> = resolved + endpointResolverMiddleware
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

the custom resolver runs after the default

is enforced here by the fact that endpointResolverMiddleware is the last term here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the endpoint resolver is there by default so this will always run after. The functionality is covered by a protocol test

@aajtodd
Copy link
Contributor Author

aajtodd commented Nov 16, 2021

Can you briefly mention testing steps here as I believe we don't have test coverage for this?

I generated the machine learning client and noticed not endpoint resolver was registered in any of the operations except Predict. I regenerated after the change and verified that (1) every operation has an endpoint resolver and (2) Predict has the customized resolver registered after (again also covered by a protocol test)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-machine-learning

@aajtodd aajtodd merged commit 6c725b1 into main Nov 17, 2021
@aajtodd aajtodd deleted the fix-machine-learning branch November 17, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Machine learning customization is generating client without a default resolver

3 participants