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: rethink the expiry related API #1727

Closed
MVrachev opened this issue Dec 14, 2021 · 5 comments · Fixed by #1743
Closed

Metadata API: rethink the expiry related API #1727

MVrachev opened this issue Dec 14, 2021 · 5 comments · Fixed by #1743
Assignees

Comments

@MVrachev
Copy link
Collaborator

Description of issue or feature request:

In a recent discussion (see discussion in #1722) we reevaluated the need for Singed.bump_expiry() as instead of using the API:

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

one can just do:

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

As a side benefit this will allow the user to use dateutil.relativedelta which is not supported by Singed.bump_expiry as of #1726.

@jku
Copy link
Member

jku commented Dec 14, 2021

Some additional detail here

  • I think an actual repository would never bump expiry to previous expiry plus delta. They'd want to bump expiry to today plus delta (if you want reasons why, imagine an original expiry period of 365days: a responsible repository would of course resign much earlier than that, let's say at 121 day mark. Now the next expiry is set 609 days forward. In 121 more days the repository resigns again: now the expiry is 853 days into the future...).
  • so a potential reason to include a method is to handle today plus delta and then make sure microseconds are not an issue
  • since relativedelta may only be needed for annotations, there is an option of "stringified" annotations (and then importing only if we're statically type checking) but let's only do this if it's really necessary

@jku jku changed the title Metadata API: do we need Signed.bump_expiry() ? Metadata API: rethink the expiry related API Dec 15, 2021
@jku
Copy link
Member

jku commented Dec 15, 2021

#1712 is about handling microseconds with a set_expiry(): I think these two are related...

@jku
Copy link
Member

jku commented Dec 15, 2021

(Edited to change example to a property instead of a method)

One possibility:

  • make expiry a property with a setter function that floors microseconds
  • remove bump_expiry()
  • document a reasonable way to bump expiry:
  # set expiry to seven days from now
  signed.expiry = utcnow() + relativedelta(day=7)

I think I'm fine with this.

@lukpueh
Copy link
Member

lukpueh commented Dec 21, 2021

(Edited to change example to a property instead of a method)

One possibility:

  • make expiry a property with a setter function that floors microseconds

Done as per #1712.

  • remove bump_expiry()

Agreed!

  • document a reasonable way to bump expiry:
  # set expiry to seven days from now
  signed.expiry = utcnow() + relativedelta(day=7)

I think that's fair.

@lukpueh lukpueh self-assigned this Dec 21, 2021
@lukpueh
Copy link
Member

lukpueh commented Dec 21, 2021

I have a PR in the queue. Will publish tomorrow.

lukpueh added a commit to lukpueh/tuf that referenced this issue Dec 21, 2021
TODO:
- Update tests
- Make property docs render on RTD?

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/tuf that referenced this issue Dec 22, 2021
Remove `bump_expiration()` method, which is unlikely to be used as
is, i.e.  bump to "current expiration date plus delta". A more
realistic use case is to bump to "now plus delta" (see theupdateframework#1727 for
details).

Moreover, bump_expiration can either way easily be replaced by a
one-liner expression using the 'datetime' module. A corresponding
code snippet is added to the `expires` property's docstring.  Note:
`expires` became a property with a millisec-removing setter (for
spec conformance) in  theupdateframework#1712, which further reduces the need for a
convenience bump_expiration method.

This patch also removes a related unit test and updates another
one.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
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 a pull request may close this issue.

3 participants