Skip to content
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

Add credshelper package to support the credentials helper interface #562

Merged
merged 1 commit into from
May 16, 2024

Conversation

banikharbanda
Copy link
Collaborator

@banikharbanda banikharbanda commented May 6, 2024

This adds all the credentials helper relevant code from
bazelbuild/reclient/internal/pkg/auth to the new credshelper package in
remote-apis-sdks with some changes:

  • CredentialsHelper isn't an auth "Mechanism" (since we're only
    supporting one mechanism, we don't need to define a type for that)
  • If no cache file is provided then it is assumed that no caching is
    required. If a valid cache file path is provided then it always caches
    credentials.
  • Updated oauth2 package version to support ReuseTokenWithExpiry - this
    had to be moved earlier in the Workspace file to override older
    versions imported from elsewhere.

Copy link
Collaborator

@gkousik gkousik left a comment

Choose a reason for hiding this comment

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

Can you add a description to the change about "what the current CL adds" and what subsequent CLs will add?

Also, is the code copied over from re-client or has there been some modifications to it?

I guess without the subsequent change that actually uses the creds helper, I'm inclined to rubber-stamp this and review this code in the CL that actually uses it :) Would that be ok?

This adds all the credentials helper relevant code from
bazelbuild/reclient/internal/pkg/auth to the new credshelper package in
remote-apis-sdks with some changes:
- CredentialsHelper isn't an auth "Mechanism" (since we're only
  supporting one mechanism, we don't need to define a type for that)
- If no cache file is provided then it is assumed that no caching is
  required. If a valid cache file path is provided then it always caches
credentials.
- Updated oauth2 package version to support ReuseTokenWithExpiry - this
  had to be moved earlier in the Workspace file to override older
versions imported from elsewhere.
@banikharbanda
Copy link
Collaborator Author

Can you add a description to the change about "what the current CL adds" and what subsequent CLs will add?

Also, is the code copied over from re-client or has there been some modifications to it?

I guess without the subsequent change that actually uses the creds helper, I'm inclined to rubber-stamp this and review this code in the CL that actually uses it :) Would that be ok?

Added more detail to the PR description and commit message. And yes makes sense to review the final version of this which is actually used (and will look a bit different from what it is currently as well)

@banikharbanda banikharbanda merged commit e74bc3d into bazelbuild:master May 16, 2024
6 checks passed
@banikharbanda banikharbanda deleted the credshelper branch May 16, 2024 14:49
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.

2 participants