-
Notifications
You must be signed in to change notification settings - Fork 58
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
Aws sdk rust #469
Aws sdk rust #469
Conversation
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.
nice work
None => Region::default(), | ||
}, | ||
) | ||
let config = aws_config::from_env(); |
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.
Does from_env
already use the profile if environment variables are not present? Although the behavior was previously not commented, I think it would help to have some comments here explaining why from_env
happens first, then things are overridden by the profile.
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.
https://docs.rs/aws-config/0.13.0/aws_config/fn.from_env.html it creates a ConfigLoader, which is a builder for the SdkConfig. Looking at the source, it's the same as ConfigLoader::default()
https://docs.rs/aws-config/0.13.0/aws_config/struct.ConfigLoader.html .from_env()
is how the docs always refer to creating the builder for the SdkConfig, by default it looks in your environment and default profile, and the overrides are if you're doing anything else.
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.
I think Matt meant adding code comments for the explanation.
edit: Since ConfigLoader
does document that the default chain implementation goes through from_env
first, I think it's ok to document that here.
None => Region::default(), | ||
}, | ||
) | ||
let config = aws_config::from_env(); |
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.
Request same documentation here about why we do from_env
first then override values with the profile.
ee084ed
to
56bacc9
Compare
tough-kms/tests/all_test.rs
Outdated
@@ -15,7 +10,7 @@ use tough::schema::key::Key; | |||
use tough_kms::KmsKeySource; | |||
use tough_kms::KmsSigningAlgorithm::RsassaPssSha256; | |||
|
|||
/// Deserialize base64 to `bytes::Bytes` | |||
/// Deserialize base64 to `byt/es::Bytes` |
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.
/// Deserialize base64 to `byt/es::Bytes` | |
/// Deserialize base64 to `bytes::Bytes` |
Is this a typo?
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.
Is there anything we can do to test these changes? I believe at a minimum you should try this out in a Bottlerocket host (after bottlerocket-os/bottlerocket#2300 is merged) and see the update flow still works.
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.
Richard pointed out to me offline that github actions runs the integration tests. After looking through the integration test, I'm satisfied with the coverage.
Update tough-kms to use aws sdk rust Update tough-ssm to use aws-sdk-rust Update tuftool to AWS SDK Rust
Issue #, if available:
#428
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.