Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add endpoint plugins #1362

Merged
merged 2 commits into from
Mar 1, 2024
Merged

feat: add endpoint plugins #1362

merged 2 commits into from
Mar 1, 2024

Conversation

AndrewFossAWS
Copy link
Contributor

@AndrewFossAWS AndrewFossAWS commented Feb 22, 2024 •
edited
Loading

Description of changes

New/existing dependencies impact assessment, if applicable

No new dependencies were added to this change.

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

@AndrewFossAWS AndrewFossAWS changed the title Runtime plugins Endpoint Plugins Feb 22, 2024
@AndrewFossAWS AndrewFossAWS changed the title Endpoint Plugins feat: add endpoint plugins Feb 22, 2024
@AndrewFossAWS AndrewFossAWS marked this pull request as ready for review February 22, 2024 22:45
sichanyoo
sichanyoo reviewed Feb 23, 2024
View reviewed changes
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

Left a couple questions.

Sources/Services/AWSXRay/XRayClient.swift Outdated
Comment on lines 130 to 152
public class EndpointPlugin: Plugin {
private var endpointResolver: EndpointResolver
public init(endpointResolver: EndpointResolver) {
self.endpointResolver = endpointResolver
}
public func configureClient(clientConfiguration: ClientRuntime.ClientConfiguration) throws {
if var config = clientConfiguration as? XRayClient.XRayClientConfiguration {
config.endpointResolver = self.endpointResolver
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: So if end-users wanted to customize endpoint resolver for XRayClient service via plugins, would they override this class and assign their custom endpoint resolver to config.endpointResolver? I.e., is this the extension point for end-user customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

End users can pass in their own customized endpoint resolver via the EndpointPlugin constructor, or they can just implement their own plugin similar to this.

...t-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/plugins/EndpointPlugin.kt Outdated Show resolved Hide resolved
syall
syall approved these changes Feb 27, 2024
View reviewed changes
...t-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/plugins/EndpointPlugin.kt Outdated
Comment on lines 19 to 20
override val isDefault: Boolean
get() = false
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I noticed isDefault defaults to true, but maybe we should default it to false in Plugin so customers implementing their own plugins won't have to override this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Plugin interface requires the implementations to specify whether it's default or not.

...t-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/plugins/EndpointPlugin.kt
Comment on lines +25 to +24
writer.openBlock("public init(endpointResolver: \$L) {", "}", AWSServiceTypes.EndpointResolver) {
writer.write("self.endpointResolver = endpointResolver")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should endpoint also be configurable in EndpointPlugin? e.g.

aws-sdk-swift/Sources/Services/AWSACM/ACMClient.swift

Line 64 in e8da6aa

public var endpoint: Swift.String?

If it is, maybe a second initializer can be added, and the configureClient() call would only set the values on the config if it's not nil.

I'm ok with excluding it though.

@sichanyoo sichanyoo self-requested a review February 29, 2024 18:17
sichanyoo
sichanyoo approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

LGTM, should be good to go after merging changes from main & having CI pass.

@AndrewFossAWS AndrewFossAWS temporarily deployed to Codegen-Build-Test March 1, 2024 04:07 — with GitHub Actions Inactive
@AndrewFossAWS AndrewFossAWS merged commit 89d48c0 into main Mar 1, 2024
17 checks passed
@AndrewFossAWS AndrewFossAWS deleted the runtime-plugins branch March 1, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sichanyoo sichanyoo sichanyoo approved these changes

@syall syall syall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrewFossAWS @syall @sichanyoo