Skip to content

Conversation

@lauzadis
Copy link
Member

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aajtodd and others added 30 commits February 13, 2024 09:04
* fix glibc version to 2.19 by using manylinux images.
* fix aarch64 to not use `moutline-atomics` by using the same toolchain that K/N is using
@github-actions

This comment has been minimized.

uses: ./.github/actions/setup-build
- name: Configure Docker Images
run: |
./docker-images/build-images.sh linux-x64 linux-arm64
Copy link
Member Author

@lauzadis lauzadis Aug 14, 2025

Choose a reason for hiding this comment

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

note: @ianbotsf We are still using Docker for Linux CI, I will need to think about how we remove Docker later.

private val EXPECTED_CREDENTIALS = Credentials("access_key_id", "secret_access_key", "session_token")

class CredentialsProviderTest : CrtTest() {
@Ignore // FIXME Enable when Kotlin/Native implementation is complete
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we haven't implemented Native credentials providers with bindings to CRT (and probably never will?) since we have common implementations in smithy-kotlin

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you mention that in the comment please, unless it's documented somewhere else

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

Comment on lines 1 to 5
{
"id": "5b6cf8f2-eff6-4def-a335-1de5052a810a",
"type": "feature",
"description": "Add support for Kotlin/Native"
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Link to public issue

Comment on lines 1 to 5
{
"id": "5b6cf8f2-eff6-4def-a335-1de5052a810a",
"type": "feature",
"description": "Add support for Kotlin/Native"
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: "requiresMinorVersionBump": true

Comment on lines 68 to 75
@Ignore // FIXME This test is broken since switching runSuspendingTest to runTest
@Test
fun testHttpGet() = runSuspendTest {
fun testHttpGet() = runTest {
testSimpleRequest("GET", "/get", 200)
testSimpleRequest("GET", "/post", 405)
testSimpleRequest("GET", "/put", 405)
testSimpleRequest("GET", "/delete", 405)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What's the path to getting these ignored tests working again?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to investigate why they no longer work after moving away from runSuspendingTest, I made a backlog task SDK-KT-774 if that works for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you stop using runSuspendTest because it was basically a copy of runTest? It's a little concerning to leave that test disabled. Can you temporarily enable it by switching back to runSuspendTest or some other alternative. Unless it's related to something in our native implementation I think it should fine to use runSuspendTest for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I replaced it because there was a TODO saying we should. runSuspendTest does not exist anymore. I just updated this to use runBlocking (which is what runSuspendTest used before). It works fine now

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, thanks for capturing a backlog item.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it was fixed thanks to @0marperez question, I decided to use runBlocking and everything works now

@github-actions

This comment has been minimized.

@lauzadis lauzadis added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Aug 14, 2025
@lauzadis lauzadis requested a review from ianbotsf August 14, 2025 18:58
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@lauzadis lauzadis requested a review from 0marperez August 14, 2025 21:14
Comment on lines 68 to 75
@Ignore // FIXME This test is broken since switching runSuspendingTest to runTest
@Test
fun testHttpGet() = runSuspendTest {
fun testHttpGet() = runTest {
testSimpleRequest("GET", "/get", 200)
testSimpleRequest("GET", "/post", 405)
testSimpleRequest("GET", "/put", 405)
testSimpleRequest("GET", "/delete", 405)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you stop using runSuspendTest because it was basically a copy of runTest? It's a little concerning to leave that test disabled. Can you temporarily enable it by switching back to runSuspendTest or some other alternative. Unless it's related to something in our native implementation I think it should fine to use runSuspendTest for now.

@lauzadis lauzadis requested a review from 0marperez August 14, 2025 21:58
@sonarqubecloud
Copy link

@github-actions
Copy link

Affected Artifacts

Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
aws-crt-kotlin-jvm.jar 221,683 216,897 4,786 2.21%

@lauzadis lauzadis merged commit f930be4 into main Aug 15, 2025
96 of 103 checks passed
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.

4 participants