Auto-refresh zoom tokens via celery task + implement task locks - #1652
Conversation
9715027 to
91dbaec
Compare
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Looks good, I have a few suggestions though. I can't test this locally right now, but I'm sure someone else can 😄
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Thanks for the change, just wanted to suggest a better flow for it though.
| if lock_token is None: | ||
| log.info('Zoom token check is already running, skipping.') | ||
| return | ||
| else: |
There was a problem hiding this comment.
This logic should probably be wrapped in a context manager so we can just use
try:
with task_lock('refresh_zoom_tokens'):
run()
except TaskLockFailed:
print('ah shoot it failed to acquire the lock')
... etc ...With task_lock pulling redis instance, locking the task, and has a finally that unlocks the task. Default values can include the default lock ttl seconds.
There was a problem hiding this comment.
Ah nice, indeed much better to do that! Changed, thanks!
| return 0 | ||
| end | ||
| """ | ||
| redis_instance.eval(release_script, 1, lock_key, lock_token) |
There was a problem hiding this comment.
I hope this can be done without an eval 🤔
There was a problem hiding this comment.
Hmm I've replaced this with Redis' Transactions, what do you think?
There was a problem hiding this comment.
Ohhh I should have seen this before commenting. That works for me!
davinotdavid
changed the title
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Just have some questions, no real changes. Looks good! Once they're answered I can approve.
|
I'm out today due to cat stuff, but will test tonight or tmw morning. Thanks! |
|
@MelissaAutumn / @davinotdavid Is there any update on this? The current state seems to be good to go, if I didn't miss anything 😇 |
|
@MelissaAutumn @davinotdavid Just a little ping on this thread: Should we take a look at the changes once more? Should I test again? Let me know if there's any status update on this. I can happily review again if needed. |
|
@devmount Whoops, misclicked here lol! Just waiting on @MelissaAutumn 's review as it is blocked by a requested changes that happened before. |
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Looks good! I realize I have a pending comment from 200 years ago but should be good tbh.
| ZOOM_AUTH_CLIENT_ID= | ||
| ZOOM_AUTH_SECRET= | ||
| ZOOM_AUTH_CALLBACK=http://localhost:8090/zoom/callback | ||
| ZOOM_TOKEN_TTL_IN_SECONDS=604800 # 7 days |
There was a problem hiding this comment.
Do we want this in the pulumi configs?
There was a problem hiding this comment.
Ah yes, thanks! Also realized that I forgot to add the GOOGLE_CHANNEL_TTL_IN_SECONDS there so I've added that too heh
a45ce9e to
03fa3a8
Compare
3 participants
What changed?
This change implements a celery task that triggers a command to check all Zoom token expiry dates. If a token is expired or under a given time threshold, a token refresh is triggered by initializing the OAuth2 Session and making a lightweight
get_me()call.A new env var
ZOOM_TOKEN_TTL_IN_SECONDSgot introduced to specify, how long we assume an auth token to be valid before checking and renewing it.If there is an error during the process, the external connection is marked with
status = errorso that at least it is shown up in the UI. Otherwise, it is marked asstatus = ok.Also, we've added some repository functions for external connection token and status handling.
Why?
This improvement should help users reducing manual effort to keep a Zoom connection active.
Limitations and Notes
Added a comment with a TODO that we should proactively send an email to the Subscriber in case there's a problem with renewing the token: #1662
How to test
Run
docker compose exec backend run-command main refresh-zoom-tokensand thendocker compose logs celery-worker --tail 30to check the celery worker logs.Applicable Issues
Closes #1580