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
Test and fix UTC issue with AppInstallationAuth #2561
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. |
b380f3c
to
ca84755
Compare
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 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! |
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.
Confused about the proposed change
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, thanks!
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.
LGTM!
ca84755
to
5a700eb
Compare
@JLLeitschuh thanks! |
Fixes #2560.