Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Aug 25, 2021

Issue #

closes #202

Description of changes

  • Moves SDK settings to a common location and defines both the corresponding environment variable and JVM system property for each setting. I've opted to only add the ones we currently use but future additions MUST use the same property names as Java v2. There is no SEP yet for this.
  • Adds a JVM system property based region provider and added it to the JVM default chain provider

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

@aajtodd aajtodd requested review from ianbotsf and kggilmer August 25, 2021 15:22
Comment on lines -16 to +25
public companion object {
internal const val ACCESS_KEY_ID: String = "AWS_ACCESS_KEY_ID"
internal const val SECRET_ACCESS_KEY: String = "AWS_SECRET_ACCESS_KEY"
internal const val SESSION_TOKEN: String = "AWS_SESSION_TOKEN"
}

public constructor() : this(Platform::getenv)

private fun requireEnv(variable: String): String =
getEnv(variable) ?: throw ConfigurationException("Unable to get value from environment variable $variable")

override suspend fun getCredentials(): Credentials = Credentials(
accessKeyId = requireEnv(ACCESS_KEY_ID),
secretAccessKey = requireEnv(SECRET_ACCESS_KEY),
sessionToken = getEnv(SESSION_TOKEN),
accessKeyId = requireEnv(AwsSdkSetting.AwsAccessKeyId.environmentVariable),
secretAccessKey = requireEnv(AwsSdkSetting.AwsSecretAccessKey.environmentVariable),
sessionToken = getEnv(AwsSdkSetting.AwsSessionToken.environmentVariable),
Copy link
Contributor

Choose a reason for hiding this comment

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

Much cleaner!

/**
* The name of the corresponding JVM system property that configures the setting
*/
public val jvmProperty: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Would we ever have a case for a setting available via environment but not the jvm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of that and wasn't sure. I slept on it and think the answer is probably no. If there is an environment variable there ought to be a corresponding JVM property. I don't see any cases in the Java SDK where this isn't true FWIW.

@aajtodd aajtodd merged commit 7f4e0ff into main Aug 26, 2021
@aajtodd aajtodd deleted the jvm-region-provider branch August 26, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

detect aws region from system properties

3 participants