Skip to content
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

Metadata API: Remove dependency on dateutil #1722

Closed
jku opened this issue Dec 13, 2021 · 5 comments · Fixed by #1726
Closed

Metadata API: Remove dependency on dateutil #1722

jku opened this issue Dec 13, 2021 · 5 comments · Fixed by #1726
Labels

Comments

@jku
Copy link
Member

jku commented Dec 13, 2021

PR #1710 added a new package dependency (dateutil) to Metadata API: This was likely not intended, and was not caught by the tests because dateutil is a test dependency.

I think we should fix this before releasing 0.20 (#1694).

@jku
Copy link
Member Author

jku commented Dec 13, 2021

I think bump_expiration() as a method is not super useful.

Current test code looks like this:

        delta = relativedelta(years=1)
        timestamp.signed.bump_expiration(delta)

Same thing without using the helper function:

        timestamp.signed.expires += relativedelta(years=1)

this is in my opinion at least as readable and works fine without the API depending on dateutil.

@jku
Copy link
Member Author

jku commented Dec 13, 2021

This breaks RTD documentation:

WARNING: autodoc: failed to import module 'metadata' from module 'tuf.api'; the following exception was raised:
No module named 'dateutil'

@jku jku added the bug label Dec 13, 2021
@MVrachev
Copy link
Collaborator

MVrachev commented Dec 13, 2021

I agree we can remove Signed.bump_expiration().
If we have an agreement I can propose a pr for this one today, so we can move forward with #1723.
What do you think @lukpueh?

@jku
Copy link
Member Author

jku commented Dec 13, 2021

we don't need to remove the function to fix the immediate issue.

Let's fix the dependency issue first, then spend the time we need to get the API (and/or docs) right before 1.0 release: as an example I only now realized the test (and the function) are also bad examples: I don't think anyone ever wants to bump to old_expiry_date+delta, real users want to bump to current_date+delta... which leads to a potential issue with microseconds.

I suggest we

  • modify the tests to use something like my suggestion (even if it's a bit bad example as mentioned)
  • remove mentions of dateutil from Metadata API
  • file an issue to improve Metadata expiry API

@MVrachev
Copy link
Collaborator

An issue where we can discuss if we want Signed.bump_expiry() #1727

@jku jku closed this as completed in #1726 Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants