refactor!: require a resolved configuration

[aajtodd]

Issue #

fixes smithy-kotlin#484

sibling: smithy-lang/smithy-kotlin#504

Description of changes

This PR updates the way AWS SDK clients are constructed such that configuration MUST be resolved at instantiation. This means that properties like region, credentialsProvider, endpointResolver, etc are required and non-nullable. This PR adds several ways to get to this resolved state:

• Provide an explicit config (does not require suspend)
• Load a shared configuration and re-use across multiple clients (suspend)
• Load a client from the environment (suspend)

Example of constructing a client:

// fluent builder
val config = S3Client.Config.fluentBuilder()
    .region("us-east-2")
    .build()

val c1 = S3Client(config)

// explicit
val c2 = S3Client {
    region = "us-east-2"
    credentialsProvider = DefaultChainCredentialsProvider()
}

runBlocking {
    // load/resolve automatically
    val c3 = S3Client.loadFromEnvironment()

    // load/resolve with options for controlling the loading process
    val c4 = S3Client.loadFromEnvironment { region = "us-east-1" }

    val sconfig = AwsClientConfig.loadFromEnvironment()
    
    // shared config
    val c5 = S3Client(sconfig)

    // shared config + overrides
    val c6 = S3Client(sconfig) {
        sdkLogMode = SdkLogMode.LogRequest
    }
}

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

[kggilmer]

The cleanup around nullability in regions and CPs is great, nice work!

[kggilmer]

Nice!

[kggilmer]

suggestion

Perhaps this would be better to leave out the bit about efficiency and just say that config can be shared, as customers may have other reasons or ideas for why they want to share configurations unrelated to efficiency.

NOTE: If you are using multiple AWS service clients you may wish to share the configuration among them
by constructing an [#Q] and passing it to each client upon construction.

[ianbotsf]

Looks pretty good!

My chief remaining concern is the verbosity of loadFromEnvironment as a name. While I could live with it if no one else shares the concern, I'd like to propose the following shorter alternatives (which may have other downsides):

• fromEnvironment: matches the more typical factory method pattern of starting with "from"
• detect/automatic/autoDetect: implies hands-off configuration based on available local context
• default (😛): pragmatically acknowledges that this will be the predominant use case (esp. given EC2, Lambda, and other compute environments where config is present)

[ianbotsf]

Question: Why does AwsClientConfig need a public empty companion object?

[aajtodd]

It's extended in aws-config

[ianbotsf]

Question: Why not reuse the symbol in AwsRuntimeTypes?

[aajtodd]

I'm only human Ian, accidents do still happen 😛

Will fix

[ianbotsf]

Nit: Using dq may be cleaner:

propertyType = ClientConfigPropertyType.ConstantValue(sigv4ServiceName.dq())

[aajtodd]

Agreed, this was copy pasta, will update.

[aajtodd]

My chief remaining concern is the verbosity of loadFromEnvironment as a name. While I could live with it if no one else shares the concern, I'd like to propose the following shorter alternatives (which may have other downsides):

• fromEnvironment: matches the more typical factory method pattern of starting with "from"
• detect/automatic/autoDetect: implies hands-off configuration based on available local context
• default (😛): pragmatically acknowledges that this will be the predominant use case (esp. given EC2, Lambda, and other compute environments where config is present)

Sure, now is the time to get it right 😉

Ok so I don't love default(), it doesn't imply the right semantics of what is going to happen. I'd also like to choose something that has symmetry with AwsClientConfig.loadFromEnvironment(). Whatever we choose here, I would choose the same function name for that.

I think my preference lands on one of:

1. fromEnvironment()
2. simply load()

While detect(), autoDetect(), or automatic() do imply the right thing, I think the other options would be more idiomatic.

@kggilmer @ianbotsf @kiiadi Opinions?

[kggilmer]

My chief remaining concern is the verbosity of loadFromEnvironment as a name. While I could live with it if no one else shares the concern, I'd like to propose the following shorter alternatives (which may have other downsides):

IMO there are (at least) two properties of this call that customers would care about knowing of:

1. that what is returned depends on the local environment in which the program is running
2. that this call is doing "substantial" work in order to resolve, as apposed to say a couple of local property lookups

So a concise name that coveys these two properties in my mind is the original, loadFromEnvironment(). To me load implies "substantial" work and the rest is obvious.

[ianbotsf]

this call is doing "substantial" work in order to resolve, as apposed to say a couple of local property lookups

The "substantial work" possibility is not the majority use case is it? It seems to me the majority use case is "a couple of local property lookups". To me, the possibility of greater work is already conveyed by suspend in the method signature, which implies "this thing you're about to call may do stuff so big that we need to yield the thread until it's done".

My approval's on the PR so I can live with whatever we decide, including loadFromEnvironment. Just want to argue for brevity when I can.

[kggilmer]

The "substantial work" possibility is not the majority use case is it?

IMO it doesn't matter as long as it's possible. The idea is to eliminate unwanted surprises. That this call may read from disk or network predicated on factors outside of the program itself I think warrants the extra word. I am not sure load is it but I can't think of anything better atm.

Regarding suspension, we have it everywhere and IMO that in itself is not a clear indicator.

[aajtodd]

The "substantial work" possibility is not the majority use case is it?

IMO it doesn't matter as long as it's possible. The idea is to eliminate unwanted surprises. That this call may read from disk or network predicated on factors outside of the program itself I think warrants the extra word. I am not sure load is it but I can't think of anything better atm.
Regarding suspension, we have it everywhere and IMO that in itself is not a clear indicator.

For posterity: we discussed offline and decided on fromEnvironment(). With additional documentation expected in the developer guide on various ways clients are constructed to help fill in any knowledge gaps.

To quote Kyle:

it strikes the right balance of brevity and function