Test and fix UTC issue with AppInstallationAuth #2561
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2561 +/- ##
=======================================
Coverage ? 98.35%
=======================================
Files ? 131
Lines ? 13140
Branches ? 0
=======================================
Hits ? 12924
Misses ? 216
Partials ? 0 ☔ View full report in Codecov by Sentry. |
EnricoMi
force-pushed
the
app-installation-auth-utc-fix
branch
from
b380f3c
to
ca84755
Compare
There was a problem hiding this comment.
I manually reproduce the code I show in the issue #2560, and it works fine by now. I'm able to do multiple query with the same github rest client as it's shown in the unit test.
Thanks for the quick fix.
|
@antoineKorbit thanks for testing! |
| @@ -307,7 +307,8 @@ def _is_expired(self) -> bool: | |||
| self.__installation_authorization.expires_at | |||
| - TOKEN_REFRESH_THRESHOLD_TIMEDELTA | |||
| ) | |||
| return token_expires_at < datetime.now(timezone.utc) | |||
| # to be fixed by https://github.com/PyGithub/PyGithub/pull/1831 | |||
| return token_expires_at < datetime.now(timezone.utc).replace(tzinfo=None) | |||
There was a problem hiding this comment.
Sorry, I'm confused. Why are you creating a date with the UTC timezone, then removing the timezone information?
There was a problem hiding this comment.
We need the UTC timestamp here (without timezone information), not the local time (without timezone information). Otherwise under- or overshoot the expiry by the local timezone offset hours.
The token_expires_at is a UTC timestamp without timezone information, so we cannot compare it with a timestamp that has timezone information.
There was a problem hiding this comment.
I understand now, thanks!
EnricoMi
force-pushed
the
app-installation-auth-utc-fix
branch
from
ca84755
to
5a700eb
Compare
|
@JLLeitschuh thanks! |
Fixes #2560.