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

move context to GetAccessToken function #2

Merged
merged 2 commits into from
Oct 19, 2021
Merged

move context to GetAccessToken function #2

merged 2 commits into from
Oct 19, 2021

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Oct 18, 2021

Have context passed to GetAccessToken instead of the initialize functions

@iwysiu iwysiu requested review from a team, joshhunt and sunker and removed request for a team October 18, 2021 16:06
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.

Great work on this @iwysiu!

Do you think we should move the tokenprovider package to the pkg folder? If not, we should remove the pkg directory.

Copy link

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 25 to 27
func (source *gceSource) getCacheKey() string {
return source.cacheKey
}

Choose a reason for hiding this comment

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

I guess it's a matter of style but if both the function and the property are not exported, what's the point of defining the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was so tokenProviderImpl could call it. Alternatively, cacheKey could just be a string stored on tokenProviderImpl, but this felt neater.

Copy link
Contributor Author

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

moved to pkg!

Comment on lines 25 to 27
func (source *gceSource) getCacheKey() string {
return source.cacheKey
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was so tokenProviderImpl could call it. Alternatively, cacheKey could just be a string stored on tokenProviderImpl, but this felt neater.

@iwysiu iwysiu merged commit 3ff525a into main Oct 19, 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.

None yet

3 participants