-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fallback for situations where Python's fromtimestamp() raises OSError or OverflowError #2972
Conversation
Codecov ReportPatch coverage:
❗ 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 @@
## develop #2972 +/- ##
===========================================
- Coverage 93.29% 93.28% -0.01%
===========================================
Files 64 64
Lines 13592 13607 +15
===========================================
+ Hits 12680 12693 +13
- Misses 912 914 +2
☔ View full report in Codecov by Sentry. |
{ | ||
"type": "bugfix", | ||
"category": "Parsers", | ||
"description": "fixes `#3642 <https://github.com/boto/botocore/issues/3642>`__, `#2564 <https://github.com/boto/botocore/issues/2564>`__, `#2355 <https://github.com/boto/botocore/issues/2355>`__, `#1783 <https://github.com/boto/botocore/issues/1783>`__, boto/boto3`#2069 <https://github.com/boto/botocore/issues/2069>`__" |
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 can link to an issue, but ideally the description should be mostly explanatory.
e.g.
Fixes datetime parse error handling for 32-bit systems and UTC positive timezone offsets
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.
Yeah, after expansion those issue references are a bit excessive! Fixed in 768b88a.
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.
Looks good, thanks @jonemo!
It looks like the docstring in
yet it accepts an arbitrary Secondly: (I know I'm very late with this review sort of thing, but I'm looking at making EDIT: See #3063. |
This addresses various issues where calling
datetime.datetime.fromtimestamp()
fails withOverflowError
orOSError
. These behaviors are platform-dependent and currently seem to affect mainly Windows. The Python documentation mentions the existence of these failure mode.This PR adds a fallback for when parsing a numeric timestamp fails that uses
timedelta
to create a datetime that is offset from the epoch (Jan 1, 1970) by the given number of seconds. This has the same effect as callingfromtimestamp()
without calling the Clocaltime()
function.Some related issues:
The general theme of these issues is that a service returns a timestamp that falls outside the timespan between 1970 and 2038. Examples I have seen include Quicksight dashboards with time axes starting in the early 1900s, ACM certificates that expire after 2038, and SSM Patches with incorrect
InstalledTime
fields claiming patch dates before 1970.#1970 and #1939 have been previous attempts to address a subset of these problems. The former has not been merged, the solution from the latter remains in place after this PR. (An aside: Note the coincidence that PR ID 1970 addresses an issue related to epoch times!)
Q: Why not also replace the use of
fromtimestamp()
incredentials.py
?A: In order to preserve existing code paths, this new fallback is not implemented as a drop-in replacement for Python's
fromtimestamp
. As a result, I could not use it to also replace the one remaining use offromtimestamp()
incredentials.py
. That's also why I made_epoch_seconds_to_datetime
private (with leading_
). This remaining instance offromtimestamp()
is unlikely to yield the exceptions addressed in this PR because timestamps in credentials do not fall outside the "safe" timespan between 1970 and 2038 (currently and in the foreseeable future).