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.

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

fix: apply clock skew interceptor to clients created via invoke #1284

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

ianbotsf
Copy link
Contributor

@ianbotsf ianbotsf commented Apr 16, 2024

Issue #

Addresses #1211

Description of changes

This change adds the clock skew interceptor to clients created via invoke (as opposed to fromEnvironment). It pulls in upstream changes from smithy-kotlin which add a finalizeConfig method to the abstract superclass from which AbstractAwsSdkClientFactory extends. This causes a naming conflict so the SDK's suspend finalizeConfig method has been renamed finalizeEnvironmentalConfig.

Companion PR: smithy-lang/smithy-kotlin#1067

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

Copy link

A new generated diff is ready to view.

Copy link

A new generated diff is ready to view.

@ianbotsf ianbotsf added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Apr 16, 2024
@ianbotsf
Copy link
Contributor Author

FYI I added the ackhowledge-api-break label to this PR because I think the breaks are safe but please chime in if you have differing opinions.

Copy link
Member

@lauzadis lauzadis left a comment

Choose a reason for hiding this comment

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

Approved, but let's talk about the API break

@@ -23,18 +23,14 @@ class ServiceClientCompanionObjectWriterTest {
ServiceClientCompanionObjectWriter().write(writer, null)

val expected = """
public companion object : AbstractAwsSdkClientFactory<Config, Config.Builder, TestGeneratorClient, Builder>() {
Copy link
Member

Choose a reason for hiding this comment

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

This is a test for the companion object but there's no longer a companion object being generated, is that because there's no protocol? Can we add a protocol or maybe rename the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no longer ServiceClientCompanionObjectWriter's job to create the companion object, merely to append things to sections of it, which the updated test reflects. In my view the class and test names are still correct.

@ianbotsf ianbotsf changed the base branch from main to v1.2 April 17, 2024 21:30
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ianbotsf
Copy link
Contributor Author

Discussed offline and this change is breaking enough that we don't want to push it directly to main. Updated base branch to v1.2. Will pursue a temporary fix strictly within aws-sdk-kotlin codegen until such time as we have another minor version bump.

@ianbotsf ianbotsf merged commit a0c8ec5 into v1.2 Apr 17, 2024
6 of 16 checks passed
@ianbotsf ianbotsf deleted the fix-clockskew-application branch April 17, 2024 22:14
lauzadis added a commit that referenced this pull request Apr 27, 2024
* fix: apply clock skew interceptor to clients created via `invoke` (#1284)

* Add new changelog

* Upgrade Kotlin version to match smithy-kotlin

---------

Co-authored-by: Ian Botsford <83236726+ianbotsf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock skew interceptor is only applied when constructing clients via fromEnvironment
2 participants