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 tokenprovider package #1

Merged
merged 5 commits into from
Oct 14, 2021
Merged

Add tokenprovider package #1

merged 5 commits into from
Oct 14, 2021

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Oct 7, 2021

Fixes grafana/grafana#39950

Notes:

  • I had to remove the logging from the original code.
  • The old code creates TokenSource when calling GetAccessToken(). I can do that instead, this is nicer to read.

@iwysiu iwysiu requested review from a team, joshhunt and vickyyyyyyy and removed request for a team October 7, 2021 21:18
Copy link
Contributor

@sunker sunker left a comment

Choose a reason for hiding this comment

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

What a great improvement this refactoring is!

I guess writing tests for GCE would not make much sense? Possibly you could test that it creates a new cache entry different from jwt auth. Up to you!

Copy link

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

Awesome stuff!

}

// GetAccessToken implements TokenProvider
func (tpi *tokenProviderImpl) GetAccessToken() (string, error) {
Copy link

Choose a reason for hiding this comment

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

I'm not a golang person, but I find this pattern of short and obtuse variable names in grafana's go code to be really hard to read.

Perhaps this could be like tokenProvider or something else that makes it more readable at first glance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@iwysiu iwysiu merged commit fecad2a into main Oct 14, 2021
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.

Google: Move google plugin token provider logic to shared SDK
3 participants