-
-
Notifications
You must be signed in to change notification settings - Fork 43
Deprioritize GCloudAuthorizedUser #74
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
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.
LGTM, I'm just thinking since the order is already being changed if it doesn't make sense to bring the ADC as last step to have the same order as other SDKs
I found it hard to intuitively tell the difference between DefaultAuthorizedUser and DefaultServiceAccount.
|
Okay, I've aligned the order with the upstream SDKs. Also renamed the |
valkum
left a comment
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.
Looks good now. Just a single thought. But feel free to merge without following my remarks.
|
@hrvolapeter thanks for the feedback, please have a look to see if you agree with the renaming! |
hrvolapeter
left a comment
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.
Looks good and thanks for the changes! 👍
|
Published 0.9.0 on crates.io. I was going to add a GitHub release to write some release notes but I don't think I have permissions to do so. |
While we're not sure this is a regression since #67 we've recently been seeing elevated error rates in production for some of our code running gcp_auth in a Docker container that has the
gcloudCLI installed. The error looks like this (this is from a gRPC client call):Code that doesn't have
gcloudinstalled in the container seems to be running without errors.The documentation for the
print-access-tokencommand also lists some limitations:We also noted that the order of token acquisition methods tried by the
gcp_auth::AuthenticationManagerdoesn't match what official Google SDKs do -- this PR would bring gcp_auth closer in line with official SDKs.Because this seems like a substantial change to the documented mechanics, I've included a commit that bumps the version number to the next semver-incompatible release (despite there being no actual changes to the public API). When releasing this change we should publish release notes that clearly point out the change.
(I worked with @valkum on this, hope I'm correctly representing his research.)