Skip to content

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

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

Mansur908
Copy link
Contributor

Fixes #2572

Copy link
Member

@lukpueh lukpueh left a 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.

@lukpueh
Copy link
Member

lukpueh commented Feb 27, 2024

FYI: You can run ruff format . to reformat the long lines

@jku
Copy link
Member

jku commented Feb 27, 2024

@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]>
@Mansur908 Mansur908 requested a review from lukpueh February 27, 2024 19:21
Copy link
Member

@lukpueh lukpueh left a 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.

@jku
Copy link
Member

jku commented Feb 28, 2024

This might help you avoid the long lines in tests and examples: Instead of from datetime import timezone you could do from datetime import UTC -- I think this alias looks fine for our case and makes usage short.

(but the suggestion to run tox after any changes is still good)

Signed-off-by: Nurgaleev_Mansur_908 <[email protected]>
@jku
Copy link
Member

jku commented Feb 28, 2024

Interesting, this leads to more code issues (serialization and deserialization), I believe:

  • datetime.strptime() returns naive datetimes (at least for the input we give it): these will then not be compatible with non-naive datetime.now() results. We probably should set the timezone to UTZ immediately after reading the naive datetime with strptime
  • serializing utc datetimes fails as our code expires.isoformat() + "Z" produces gibberish now: we should use an explicit strftime call here

I can have a look at this later but if you want to continue investigating:

  • strptime() is used in tuf/api/_payload.py and tests/generated_data/generate_md.py
  • isoformat() is used in tuf/api/_payload.py

jku added 2 commits February 29, 2024 15:34
* 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]>
@jku
Copy link
Member

jku commented Feb 29, 2024

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

@coveralls
Copy link

coveralls commented Feb 29, 2024

Pull Request Test Coverage Report for Build 8097056321

Details

  • 13 of 15 (86.67%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 97.546%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tuf/api/_payload.py 6 8 75.0%
Totals Coverage Status
Change from base Build 8066093275: -0.2%
Covered Lines: 1466
Relevant Lines: 1493

💛 - Coveralls

Older Pythons do not have this alias, use timezone.utc

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku requested a review from lukpueh February 29, 2024 13:49
@jku
Copy link
Member

jku commented Mar 7, 2024

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

@jku jku merged commit ff497c7 into theupdateframework:develop Mar 7, 2024
MVrachev added a commit to MVrachev/repository-service-tuf-worker that referenced this pull request Apr 4, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datetime.utcnow() is deprecated: stop using it
4 participants