Skip to content

Conversation

@alvarowolfx
Copy link
Collaborator

@alvarowolfx alvarowolfx commented Oct 29, 2025

Fixes #2929 and #3362

@alvarowolfx
Copy link
Collaborator Author

This is a WIP and is missing all documentation for the new interface.

Pros:

  • AccessTokenCredentials is compatible with Credentials type (via automatic conversion to CredentialProvider -> Credentials)
  • Reuse all current Builders and can selective pick which Credentials types would support that (we don't want API Key creds to support this)
    Cons:
  • I don't like the build_access_token_credentials() method name. Too long and not sure if is a good pattern to add another final branch to a Builder.
  • Introduces new AccessTokenCredentials which is almost the same as Credentials. Perhaps it can be simplified ?
  • AccessTokenCredentialsProvider looks really similar to TokenProvider, can this be reused ?
    • Probably not, Token is private and we need a new AccessToken type to expose that.

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 68.51852% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.96%. Comparing base (6b5677c) to head (6f3dd99).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/auth/src/credentials.rs 76.19% 5 Missing ⚠️
src/auth/src/credentials/external_account.rs 62.50% 3 Missing ⚠️
src/auth/src/credentials/impersonated.rs 57.14% 3 Missing ⚠️
src/auth/src/credentials/service_account.rs 50.00% 3 Missing ⚠️
src/auth/src/credentials/user_account.rs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3642      +/-   ##
==========================================
- Coverage   96.14%   95.96%   -0.18%     
==========================================
  Files         140      144       +4     
  Lines        5442     5574     +132     
==========================================
+ Hits         5232     5349     +117     
- Misses        210      225      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coryan
Copy link
Collaborator

coryan commented Oct 29, 2025

I don't know what is the exact level of test coverage we want, but 46% is too low.

@alvarowolfx
Copy link
Collaborator Author

I don't know what is the exact level of test coverage we want, but 46% is too low.

this is also missing tests, is just a draft for the public API.

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

FWIW, this approach makes a lot of sense to me. It is lamentable that we are already making backwards compatibility sacrifices in version 1.1.0 of a library, but that is where we are.

I think I messed up by making Credentials an opaque object instead of a trait. I think at the time I was afraid of all the generics that entailed... but now I realize that is just what flexible Rust looks like.

)
.build()?;
//TODO: revert - this is just to show that AccessTokenCredentials is compatible with Credentials
.build_access_token_credentials()?;
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think build_foo() is not that crazy.

For example, for 99% of our builders, we have a send(), but in storage, our builder::WriteObject has send_buffered() and send_unbuffered().

Q: do we recommend applications use build() or build_access_token_credentials() ?

@alvarowolfx alvarowolfx marked this pull request as ready for review November 11, 2025 19:49
@alvarowolfx alvarowolfx requested review from a team as code owners November 11, 2025 19:49
@alvarowolfx alvarowolfx marked this pull request as draft November 11, 2025 20:10
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.

Add "access token" method to credentials

3 participants