feat: support environment based region detection #137
Conversation
| override fun toString(): String = buildString { | ||
| append("AwsRegionProviderChain") | ||
| providers.fold(this) { sb, provider -> | ||
| sb.append(" -> ") | ||
| sb.append(provider::class.simpleName) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: Simplified:
override fun toString(): String =
(listOf(this) + providers).map { it::class.simpleName }.joinToString(" -> ")| override suspend fun getRegion(): String? { | ||
| // FIXME - 1.5 had firstNotNullOfOrNull() | ||
|
|
||
| for (provider in providers) { | ||
| try { | ||
| val region = provider.getRegion() | ||
| if (region != null) { | ||
| return region | ||
| } | ||
| } catch (ex: Exception) { | ||
| logger.debug { "unable to load region from $provider: ${ex.message}" } | ||
| } | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Question: Is the right outcome from getting no provider to return null? Would throwing an exception be better? Is the answer different if the chain has no providers in it?
There was a problem hiding this comment.
It's a good question. I considered throwing an exception but I'm not sure it's really an error condition but I can definitely make an argument for it to be that way.
I guess the answer comes down to "must a provider ALWAYS return a value"?
Which do you prefer and why?
There was a problem hiding this comment.
After thinking more about how providers will be used and chained together, I prefer returning null. So never mind here.
| public open class AwsRegionProviderChain( | ||
| private vararg val providers: AwsRegionProvider | ||
| ) : AwsRegionProvider { |
There was a problem hiding this comment.
Question: Is a chain with no providers "valid"?
val chain = AwsRegionProviderChain() // Is this valid?There was a problem hiding this comment.
no this is a good call out. I think we ought to require at least one.
| /** | ||
| * [AwsRegionProvider] that checks `AWS_REGION` and `AWS_DEFAULT` region environment variables | ||
| * @param environ the environment mapping to lookup keys in (defaults to the system environment) | ||
| */ | ||
| public class EnvironmentRegionProvider( | ||
| private val environ: Environment | ||
| ) : AwsRegionProvider { | ||
| public constructor() : this(SystemEnvironment) | ||
|
|
||
| override suspend fun getRegion(): String? = | ||
| environ.get(AWS_ENVIRON_REGION) ?: environ.get(AWS_ENVIRON_DEFAULT_REGION) | ||
| } |
There was a problem hiding this comment.
Suggestion: Generally interfaces or types with single members are clearer as functional types. I might've written this as:
public class EnvironmentRegionProvider(
private val getenv: (String) -> String,
) : AwsRegionProvider {
public constructor() : this(Platform::getenv)
override suspend fun getRegion(): String? =
getenv(AWS_ENVIRON_REGION) ?: getenv(AWS_ENVIRON_DEFAULT_REGION)
}There was a problem hiding this comment.
The interface is defined as a functional interface.
fun interface Environment { ... }
Which means you should be able to do:
EnvironmentRegionProvider { key -> ... }There was a problem hiding this comment.
but you're right, and we can already do what you suggested I just missed it:
public constructor(): this(Platform::getenv)
2 participants
Issue #, if available:
closes smithy-kotlin#356
Description of changes:
AWS_REGIONandAWS_DEFAULT_REGIONenvironment variablesexpect/actualor not, how we want test it, etc. It likely depends on whether some providers don't make sense on all platforms (which is basically guaranteed with JVM system property and profile based providers). We could also just have internalexpect/actualfunctions likeplatformProviderChain() -> List<AwsRegionProvider>. Rather than solve all this or implement them, I've left it for us to fill in as time allows since several of the providers are easy low hanging fruit.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.