-
Notifications
You must be signed in to change notification settings - Fork 279
change utcnow() to now(timezone.utc) #2573
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
Conversation
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.
Thanks for the patch! Please fix the linter warnings (a few lines are too long now), and sign off your commits (see DCO check), otherwise this looks great.
FYI: You can run |
@Mansur908 if you have any suggestions on how our developer docs could be better to improve this "first PR flow", please comment in #2578 |
Signed-off-by: Nurgaleev_Mansur_908 <[email protected]>
Signed-off-by: Nurgaleev_Mansur_908 <[email protected]>
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.
Lint is still failing. Did you reformat with ruff
? Btw. you can also run tests locally with tox
, or tox -e lint
to run only the linter.
This might help you avoid the long lines in tests and examples: Instead of (but the suggestion to run |
Signed-off-by: Nurgaleev_Mansur_908 <[email protected]>
Interesting, this leads to more code issues (serialization and deserialization), I believe:
I can have a look at this later but if you want to continue investigating:
|
* Most importantly use strftime() to serialize the datetime * Force the timezone as UTC when deserializing Signed-off-by: Jussi Kukkonen <[email protected]>
Practically were changing API if we start requiring that expires is non-naive because this no longer works: metadata.signed.expires = datetime(3000,1,1) We can make this work without API breaks though: * it the input is naive, just use UTC * if the input is not naive or UTC, raise Signed-off-by: Jussi Kukkonen <[email protected]>
You seem to be allowing changes to your branch (thanks) so I'll push some code fixes on top: they should get this into a ready state |
Pull Request Test Coverage Report for Build 8097056321Details
💛 - Coveralls |
Older Pythons do not have this alias, use timezone.utc Signed-off-by: Jussi Kukkonen <[email protected]>
It's a bit of a scary change (with the serialization code tweaks) but I feel pretty good about our test suite... so let's go with this. Thanks @Mansur908 |
I updated everywhere to replace `datetime.utc()` to `datetime.now(timezone.utc)` as `datetime.utc()` is documented for deprecation and is officially deprecated from python 12: https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow Also, I added to all `datetime.now()` calls the argument `timezone.utc` to make sure that we are consistent with python-tuf as python-tuf did that change as well: theupdateframework/python-tuf#2573 Signed-off-by: Martin Vrachev <[email protected]>
Fixes #2572