account id routing and credentials interface #998
Conversation
| { | ||
| "id": "6467a679-d9c3-4741-8219-28d7d32abf5f", | ||
| "type": "misc", | ||
| "description": "refactor: make `Credentials` an interface" | ||
| } |
gradle.properties
Outdated
| # SDK | ||
| sdkVersion=0.28.3-SNAPSHOT | ||
| sdkVersion=0.29.0-SNAPSHOT | ||
|
|
||
| # codegen | ||
| codegenVersion=0.28.2 | ||
| codegenVersion=0.29.0-SNAPSHOT |
There was a problem hiding this comment.
Question: Does our tooling not handle this automatically? Or is that just for aws-sdk-kotlin?
There was a problem hiding this comment.
Do you mean this should have been 0.28.2-SNAPSHOT before the change?
The tooling will automatically bump the patch version (to 0.28.3, in this case) if there's a SNAPSHOT suffix or not. But since this is a breaking change in codegen, we need to bump the minor version which can't be done automatically.
There was a problem hiding this comment.
I think he's referring to the --minor-version-bump flag to kat handling this automatically based on the changelog. I don't actually know and kind of forgot we added that. I'll be reverting the change to sdkVersion in aws-sdk-kotlin as it will cause conflicts between main and release if we don't run the merge workflow before next release.
For this one though will it auto bump both sdk and codegen version? I'm confused why these weren't in sync since I thought thats what we landed on.
There was a problem hiding this comment.
Yes the --minor-version-bump should bump both versions, since the logic for it is located in bump-version command which both sdkVersion and codegenVersion use
There was a problem hiding this comment.
They weren't in sync because there is a second bump command which runs for sdkVersion only. Probably could use a fix
| val expiration = minOf(providerCreds.expiration, (clock.now() + expireCredentialsAfter)) | ||
| val expiration = minOf(providerCreds.expiration!!, (clock.now() + expireCredentialsAfter)) | ||
| ExpiringValue(providerCreds, expiration) | ||
| } else { | ||
| val expiration = clock.now() + expireCredentialsAfter |
There was a problem hiding this comment.
Style:
val expiration = listOfNotNull(providerCreds.expiration, clock.now() + expireCredentialsAfter).min()
ExpiringValue(providerCreds, expiration)There was a problem hiding this comment.
I changed this but I'm not sure it's what we want looking closer. If provider creds aren't null we set the cache expiration to the min of the two but keep the original expiration. If they are null we set the expiration of the credentials to match when we'll expire them in the cache.
If we make this change we narrow the expiration of the credentials which seems undesirable.
There was a problem hiding this comment.
Maybe something like this:
val cacheExpiration = listOfNotNull(providerCreds.expiration, clock.now() + expireCredentialsAfter).min()
val credsExpiration = providerCreds.expiration ?: cacheExpiration
val creds = providerCreds.copy(expiration = credsExpiration)
ExpiringValue(creds, cacheExpiration)| /** | ||
| * Create a new [Credentials] instance | ||
| */ | ||
| public fun Credentials( | ||
| accessKeyId: String, | ||
| secretAccessKey: String, | ||
| sessionToken: String? = null, | ||
| expiration: Instant? = null, | ||
| providerName: String? = null, | ||
| attributes: Attributes? = null, | ||
| ): Credentials { | ||
| val resolvedAttributes = if (providerName != null && attributes?.getOrNull(IdentityAttributes.ProviderName) != providerName) { | ||
| val merged = attributes?.toMutableAttributes() ?: mutableAttributes() | ||
| merged.setIfValueNotNull(IdentityAttributes.ProviderName, providerName) | ||
| merged | ||
| } else { | ||
| attributes ?: emptyAttributes() | ||
| } | ||
|
|
||
| return CredentialsImpl(accessKeyId, secretAccessKey, sessionToken, expiration, resolvedAttributes) | ||
| } |
There was a problem hiding this comment.
Question: Any reason not to make this an invoke function on Credentials.Companion?
| init { | ||
| require(name.isNotBlank()) { "AttributeKey name (`$name`) cannot be blank" } | ||
| } |
There was a problem hiding this comment.
Nit: If name is blank there might not be much value in including it in the error message (e.g., "AttributeKey name (``) cannot be blank").
gradle.properties
Outdated
| # SDK | ||
| sdkVersion=0.28.3-SNAPSHOT | ||
| sdkVersion=0.29.0-SNAPSHOT | ||
|
|
||
| # codegen | ||
| codegenVersion=0.28.2 | ||
| codegenVersion=0.29.0-SNAPSHOT |
There was a problem hiding this comment.
Do you mean this should have been 0.28.2-SNAPSHOT before the change?
The tooling will automatically bump the patch version (to 0.28.3, in this case) if there's a SNAPSHOT suffix or not. But since this is a breaking change in codegen, we need to bump the minor version which can't be done automatically.
aajtodd
force-pushed
the
accountid-builtin
branch
from
b3b3f94 to
5574862
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
n/a
downstream: awslabs/aws-sdk-kotlin#1111
Description of changes
Refactors
Credentialsto be an interface.Miscellaneous support for upstream account ID based routing PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.