-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Update CRT to 0.5.0+ #777
Conversation
6942e2a
to
546165c
Compare
|
||
init(awsCredentialsProvider: CRTAWSCredentialsProvider) { | ||
init(awsCredentialsProvider: AwsCommonRuntimeKit.CredentialsProvider) { | ||
self.crtCredentialsProvider = awsCredentialsProvider | ||
} | ||
|
||
public static func fromEnv() throws -> AWSCredentialsProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these static methods look scary from testing point of view. Not related to this PR. I would much prefer a singleton pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only ever supposed to have a single credentials provider instance (for each type?), if not then im not sure we want singleton pattern here. (ideally i'd like to avoid singletons and global state whenever possible).
But I agree, the current interface for creating credential providers has much room for improvement
} | ||
} | ||
|
||
struct CredentialsProviderProfileOptions: CRTCredentialsProviderProfileOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. This struct no longer has any purpose.
Checkout the related factory method in [AwsCredentialsProvider]](https://github.com/awslabs/aws-sdk-swift/pull/777/files#diff-c858c06a42a458b91bb72c1ce5190f515a40cf4463775e74d8f8f91d4e53c506) to see why its no needed.
} | ||
} | ||
|
||
struct CredentialsProviderSTSConfig: CRTCredentialsProviderSTSConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. This struct no longer has any purpose.
Checkout the related factory method in [AwsCredentialsProvider]](https://github.com/awslabs/aws-sdk-swift/pull/777/files#diff-c858c06a42a458b91bb72c1ce5190f515a40cf4463775e74d8f8f91d4e53c506) to see why its no needed.
} | ||
} | ||
|
||
struct CredentialsProviderStaticConfig: CRTCredentialsProviderStaticConfigOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. This struct no longer has any purpose.
Checkout the related factory method in [AwsCredentialsProvider]](https://github.com/awslabs/aws-sdk-swift/pull/777/files#diff-c858c06a42a458b91bb72c1ce5190f515a40cf4463775e74d8f8f91d4e53c506) to see why its no needed.
} | ||
} | ||
|
||
struct CredentialsProviderWebIdentityConfig: CRTCredentialsProviderWebIdentityConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. This struct no longer has any purpose.
Checkout the related factory method in [AwsCredentialsProvider]](https://github.com/awslabs/aws-sdk-swift/pull/777/files#diff-c858c06a42a458b91bb72c1ce5190f515a40cf4463775e74d8f8f91d4e53c506) to see why its no needed.
} | ||
|
||
public func getError() throws -> String? { | ||
try crtResolvedEndpoint.getError() | ||
public func getError() -> String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would require changes in endpoint resolver. you would have to include those changes in this PR to pass checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Will update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question and a few superficial changes
AWSClientRuntime/Sources/Middlewares/Sha256TreeHashMiddleware.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, pending resolution of @ganeshnj's comments and a green build.
Issue #
#756
Description of changes
Updates code to make it compatible with CRT 0.5.0+
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.