-
Notifications
You must be signed in to change notification settings - Fork 95
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
Adding support for and id_token result using a service account #840
base: master
Are you sure you want to change the base?
Conversation
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.
Overall flow looks great, only some small/nit comments. Feel free to tackle whatever subset of the nits you'd like, I can also futz around with naming/etc after the fact.
auth/gcloud/aio/auth/token.py
Outdated
cache_preempt_after: float = 0.5, | ||
cache_refresh_after: float = 0.95, |
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.
nit: I think "cache" is the wrong word here. What do you think about background_refresh_after
and force_refresh_after
, maybe?
Also occurs to me that in the gcloud-rest
version of this library, there is no background/preempt version, so whichever is the lower of these values just becomes the forced/immediate refresh. So maybe it'd be even better to name accordingly: async_refresh_after
and sync_refresh_after
.
Thoughts? Trying to make usage as clear as possible in both cases, but I'm kind of blanking on wording which makes sense for both flows.
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 like the first suggestion, and specifically background_refresh_after
is spot on imo. However, in the gcloud-rest
case, it make zero sense.
Having said that, having two thresholds doesn't make any sense in that case altogether, since it's whichever is first forces a blocking refresh. No idea how to name intricate details of a mechanism that is pure obfuscation in gcloud-rest
.
id_token
return payload.