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

Adding support for and id_token result using a service account #840

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alexdialpad
Copy link

  • ensure_token will now not block everything after 50% of TTL.
  • Token class now able to deal with an id_token return payload.

@alexdialpad alexdialpad requested review from TheKevJames, cphoward and a team as code owners December 18, 2024 20:51
@alexdialpad alexdialpad requested review from juanamari94 and removed request for a team December 18, 2024 20:51
Copy link
Member

@TheKevJames TheKevJames left a 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 Show resolved Hide resolved
auth/gcloud/aio/auth/token.py Outdated Show resolved Hide resolved
Comment on lines 163 to 164
cache_preempt_after: float = 0.5,
cache_refresh_after: float = 0.95,
Copy link
Member

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants