-
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
Remove datetime.datetime.utcnow #3145
base: develop
Are you sure you want to change the base?
Conversation
Is this issue resolved? We are still getting when we upgrade python to 3.12 |
It would be wonderful if this could get reviewed and merged. We're getting a mountain of deprecation warnings from our tests because of this. Thanks for the work @azliu0 |
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 am not a botocore contributor but I want to see this get merged!
This PR appears to resolve the DeprecationWarning
s that are giving me heartburn, but it fundamentally alters the datetime
instances, causing ripples throughout the code and test suite.
The correct, minimal swap is:
- datetime.datetime.utcnow()
+ datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None)
I've left comments, but not at every location to be updated.
@@ -416,7 +416,7 @@ def signature(self, string_to_sign, request): | |||
def add_auth(self, request): | |||
if self.credentials is None: | |||
raise NoCredentialsError() | |||
datetime_now = datetime.datetime.utcnow() | |||
datetime_now = datetime.datetime.now(datetime.timezone.utc) |
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.
The minimal change here is:
datetime_now = datetime.datetime.now(datetime.timezone.utc) | |
datetime_now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) |
The code as it currently stands makes datetime_now
timezone-aware. The change I'm suggesting above resolves the DeprecationWarning
without fundamentally altering datetime_now
in the process.
You can see how datetime_now
was affected by looking in the test suite, where all of the existing tests had to be modified to suddenly be timezone-aware.
datetime_now = datetime.datetime.now(datetime.timezone.utc).replace( | ||
tzinfo=datetime.timezone.utc | ||
) |
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.
No need to call .replace()
here anymore; it's already set.
datetime_now = datetime.datetime.now(datetime.timezone.utc).replace( | |
tzinfo=datetime.timezone.utc | |
) | |
datetime_now = datetime.datetime.now(datetime.timezone.utc) |
datetime_now = datetime.datetime.now(datetime.timezone.utc).replace( | ||
tzinfo=datetime.timezone.utc | ||
) |
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.
Same as above.
datetime_now = datetime.datetime.now(datetime.timezone.utc).replace( | |
tzinfo=datetime.timezone.utc | |
) | |
datetime_now = datetime.datetime.now(datetime.timezone.utc) |
@@ -152,10 +152,10 @@ def prepare_request(self, request): | |||
def _calculate_ttl( | |||
self, response_received_timestamp, date_header, read_timeout | |||
): | |||
local_timestamp = datetime.datetime.utcnow() | |||
local_timestamp = datetime.datetime.now(datetime.timezone.utc) |
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.
As noted above, this can be simplified to avoid fundamentally changing local_timestamp
:
local_timestamp = datetime.datetime.now(datetime.timezone.utc) | |
local_timestamp = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) |
I've submitted an alternative, #3239, which makes significantly fewer changes to accomplish the same thing, and passes CI. |
Resolves #3088, which is currently being tracked in boto/boto3#3889.
It looks like this change was attempted in #3003, but was reverted after #3077 in #3004. These changes differ from #3003 by also addressing the conversion issues brought up in #3077, ensuring that all datetime objects are timezone aware.
These changes further improve on the changes in #3003 by mocking out
.now
with a factory, rather than hard-coding the return to be theutc
value only. This is closer to the actual functionality of.now
, which is a function that takes timezone as input (but defaults toutc
).All unit and functional tests pass locally, with the exception of
functional/test_resource_leaks.py
, which does not pass for me locally before the change.