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

fix(option): Having both a TokenSource and Scopes should fail validation #1666

Closed
wants to merge 1 commit into from

Conversation

eisandbar
Copy link
Contributor

If both ds.TokenSource and ds.Scopes are set, then an error will be returned as Scopes will be ignored

Fixes #1644

@eisandbar eisandbar requested review from a team and yoshi-approver as code owners August 16, 2022 23:12
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Although I think is a good change to make, it can't be made just yet. We will first need to audit our libraries to ensure we are not passing scopes via the WithScopes client option. I know where are in clients in this repo and may still have places where this is done in our sister repo as well. Adding this check would cause people to never be able to pass the TokenSource option right now in these instances.

I also due worry thinking more about this that there may be libraries built on libraries where these options are passed today. Once we have a better logging solution in place it may be better to log a warning in cases like this. I will need to think about this a little more.

Clarify that when both WithScopes and WithTokenSource are used, the
scope settings will be taken from the token source and the WithScopes
option will be ignored.

Fixes googleapis#1644
@eisandbar
Copy link
Contributor Author

It was decided that returning an error is unnecessary and that simply clarifying the behavior in the docs will suffice.

@eisandbar eisandbar closed this Aug 17, 2022
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.

Specifying option.WithScopes is ignored when using option.WithTokenSource
2 participants