Skip to content

Conversation

landonxjames
Copy link
Contributor

@landonxjames landonxjames commented Jul 28, 2025

Motivation and Context

Continuing the work in #4224, adding user-agent feature tracking for Credential Providers.

Description

Added AwsCredentialFeatures properties for each implementor of ProvideCredentials in aws-config.

Testing

Added integration tests for each of the supported Credential Providers. This required exposing some previously pub(crate) utility functions as pub under a test-util feature flag.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@landonxjames landonxjames marked this pull request as ready for review July 28, 2025 21:27
@landonxjames landonxjames requested a review from a team as a code owner July 28, 2025 21:27
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

Overall looks good. It seems like we made a lot of stuff public or behind a test-util feature to allow testing via sdk/integration-tests. Part of me wonders if we wouldn't be better off making these unit tests in aws-config OR updating our integration tests to assert a specific UA credential feature

e.g.

make_test!(prefer_environment, feature: AwsCredentialFeature::CredentialsEnvVars)

That way we don't have to expose a bunch of test utilities just for this. I'm slightly concerned about having to maintain a bunch of things in test-util because we made them public but open to discussion.

@ysaito1001
Copy link
Contributor

It seems like we made a lot of stuff public or behind a test-util feature to allow testing via sdk/integration-tests. Part of me wonders if we wouldn't be better off making these unit tests in aws-config

Good point. We should generally place tests in sdk/integration-tests only when tests are something specific about those target services. For instance, the test logic in imds_ua_feature can equally apply to any services, so probably better off having those tests in aws-config and making newly exposed test-utils back to pub(crate) 🤔

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

Will circle back for the test part, but looks great. Thank you for making this change 👍

…nfig test-utils

Move remaining test into S3 tests since that kind of serves as our catch
all for random features
@landonxjames
Copy link
Contributor Author

We discussed offline and decided that the best course of action, to avoid exposing new pub test-util functions, is to test the credentials features are added in aws-config and only retain some of the tests that don't need new test-util functionality in integration tests. This work was done in:

  • 63f2134 adding tests to aws-config
  • eda2912 removing the newly expose test-utils
  • 37276c5 removing the now broken integration tests and moving the remaining ones to the S3 test crate

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

There is a CI failure, but the changes look good.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@landonxjames landonxjames merged commit fa922a5 into feature/credential-features Jul 30, 2025
45 checks passed
@landonxjames landonxjames deleted the landonxjames/credential-providers branch July 30, 2025 18:39
landonxjames added a commit that referenced this pull request Jul 30, 2025
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
Merging the feature branch containing work from the below two PRs:
* #4238
* #4224


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: AWS SDK Rust Bot <[email protected]>
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.

3 participants