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

Make gcs.credentials.default ternary #393

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

ivanyu
Copy link
Contributor

@ivanyu ivanyu commented Sep 19, 2023

null should be the default value for gcs.credentials.default, because otherwise certain practical combinations are impossible (e.g. just setting gcs.credentials.json without anything else).

Also this commit refactors how the credentials settings are validated: the validation logic is completely delegated to CredentialsBuilder to have it in one place.

@ivanyu ivanyu force-pushed the ivanyu/gcs-credentials branch 6 times, most recently from 23b781b to a142d05 Compare September 19, 2023 16:42
@ivanyu ivanyu marked this pull request as ready for review September 19, 2023 16:49
@ivanyu ivanyu requested a review from a team as a code owner September 19, 2023 16:49
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

Do we actually need all this machinery for checking if multiple ways to specify credentials were used simultaneously? Looks like a common approach is to specify the priority of credentials lookup and just use the first specified according to that priority, should we follow that?

@ivanyu ivanyu force-pushed the ivanyu/gcs-credentials branch from a142d05 to 7ddb9ad Compare September 20, 2023 03:50
`null` should be the default value for `gcs.credentials.default`, because otherwise certain practical combinations are impossible (e.g. just setting `gcs.credentials.json` without anything else).

Also this commit refactors how the credentials settings are validated: the validation logic is completely delegated to `CredentialsBuilder` to have it in one place.
@ivanyu ivanyu force-pushed the ivanyu/gcs-credentials branch from 7ddb9ad to 1e828c0 Compare September 20, 2023 03:54
@ivanyu
Copy link
Contributor Author

ivanyu commented Sep 20, 2023

Do we actually need all this machinery for checking if multiple ways to specify credentials were used simultaneously? Looks like a common approach is to specify the priority of credentials lookup and just use the first specified according to that priority, should we follow that?

I like the current approach better, it's intuitive for the user.

@AnatolyPopov AnatolyPopov merged commit 38831b0 into main Sep 20, 2023
6 checks passed
@AnatolyPopov AnatolyPopov deleted the ivanyu/gcs-credentials branch September 20, 2023 08:44
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