Rename client runtime packages to aws.sdk.kotlin.runtime. #54
Conversation
aajtodd
left a comment
aajtodd
left a comment
There was a problem hiding this comment.
There are some things that need addressed first. Let me know if you have questions.
I'd like to get a consensus on package names for CRT before moving forward, see my comment on that specifically.
client-runtime/auth/common/src/aws/sdk/kotlin/runtime/auth/AwsSigv4Signer.kt
Outdated
Show resolved
Hide resolved
client-runtime/aws-client-rt/common/src/aws/sdk/kotlin/runtime/Exceptions.kt
Outdated
Show resolved
Hide resolved
client-runtime/aws-client-rt/common/src/aws/sdk/kotlin/runtime/endpoint/EndpointResolver.kt
Outdated
Show resolved
Hide resolved
client-runtime/crt-util/common/src/aws/sdk/kotlin/runtime/crt/Http.kt
Outdated
Show resolved
Hide resolved
|
|
||
| private val serviceId: String = requireNotNull(config.serviceId) { "ServiceId must not be null" } | ||
| private val resolver: EndpointResolver = requireNotNull(config.resolver) { "EndpointResolver must not be null" } | ||
| private val resolver: aws.sdk.kotlin.runtime.endpoint.EndpointResolver = requireNotNull(config.resolver) { "EndpointResolver must not be null" } |
There was a problem hiding this comment.
style
Symbol is already imported...we don't need the package name prefix here.
I'm not going to call them all out in every file, but please address this everywhere. (symbols are already imported or are from same package shouldn't need the package name explicitly)
client-runtime/auth/common/src/aws/sdk/kotlin/runtime/auth/DefaultChainCredentialsProvider.kt
Outdated
Show resolved
Hide resolved
|
@aajtodd well that was a bit sloppy on my part. Thanks for having close 👀 on the diffs and keeping things clean! 😄 |
… add some lingering copyright header fixes missed in previous commit.
… for some reason.
|
|
||
| /** | ||
| * HTTP request pipeline middleware that signs outgoing requests | ||
| */ | ||
| @InternalSdkApi |
There was a problem hiding this comment.
I think this one was actually there already. It's fine we need to comb through the runtime anyway at some point and mark API's as internal or not depending on what customers shouldn't be using (but need to be available for generated SDK's to consume)
There was a problem hiding this comment.
I put this one back before push
| val experimentalAnnotations = listOf("kotlin.RequiresOptIn", "software.aws.clientrt.util.InternalAPI", "software.aws.kotlinsdk.InternalSdkApi") | ||
| val experimentalAnnotations = listOf( | ||
| "kotlin.RequiresOptIn", | ||
| "software.aws.clientrt.util.InternalAPI", |
There was a problem hiding this comment.
no-op
It's on my TODO list to fix API to Api someday so they match but I just haven't gotten around to it...
* Rename package prefix from software.amazon.awssdk.kotlin.crt to aws.sdk.kotlin.runtime.crt * Rename root package prefix from aws.sdk.kotlin.runtime.crt to aws.sdk.kotlin.crt based on aws/aws-sdk-kotlin#54 (comment). * Cleanup from PR feedback * ktlint cleanup and resolve some warnings
* Rename client runtime packages to aws.sdk.kotlin.runtime. * Fix copyright headers from bad local config. * Package namespace and annotation cleanup from PR feedback * Update references to aws-crt-kotlin to use updated package namespace, add some lingering copyright header fixes missed in previous commit.
from previous PR renamed in aws/aws-sdk-kotlin#54. (#56)
3 participants
Issue #, if available: /story/show/176265266
Companion PR: aws/aws-crt-kotlin#4
Concerns
@InternalSdkApiannotation be added in several places. I took a like at the annotation implementation but couldn't find a reason why the name change would result in this new requirement.Description of changes:
Rename packages to
aws.sdk.kotlin.runtime.*, as this is the final package namespace we decided uponTesting Done
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.