Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Supporting service account key format OR user credential formats #76
base: main
Are you sure you want to change the base?
Supporting service account key format OR user credential formats #76
Changes from 1 commit
925dbba
d452a85
7cc714a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 it an actually expected use case that we find
ApplicationCredentials
in theUSER_CREDENTIALS_PATH
? That doesn't seem like something that makes sense.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 agree that this isn't a typical pattern, but Google's documentation refers to both the
GOOGLE_APPLICATION_CREDENTIALS
and the~/.config/gcloud/application_default_credentials.json
file as types of application default credentials, so it feels reasonable that we accept both behaviors.https://cloud.google.com/docs/authentication/application-default-credentials#order
Additionally in the go source I have been referencing, all these credential types are contained in one struct and there isn't any runtime checks to restrict usage between certain types.
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.
One more thing:
gcloud
accepts this behavior. I can runand the service account's token will get printed.
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.
Okay, in that case I suggest that we do this at the top level:
GOOGLE_APPLICATION_CREDENTIALS
environment variableUSER_CREDENTIALS_PATH
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.
Are you saying to return an error (don't try other options) if
USER_CREDENTIALS_PATH
exists but fails to parse as a valid ServiceAccount for some reason. This is the current behavior forGOOGLE_APPLICATION_CREDENTIALS
but notUSER_CREDENTIALS_PATH
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 hadn't really thought about it that way, but it seems kinda reasonable to me? Or do you think the
USER_CREDENTIALS_PATH
could contain other things? What does the Go code do in this case?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.
Hmm. The gcloud isn't open source, and the referenced go code doesn't cover that situation. I think keeping the current behavior is fine. I don't see a reason to change since we already have been using the current behavior -- I just wanted to clarify if you were expecting that.