Add a method of Signed metadata class returning information about metadata expiration #1347
Conversation
tests/test_api.py
Outdated
| is_exired = md.signed.is_expired(md.signed.expires) | ||
| self.assertEqual(is_exired, True) |
tuf/api/metadata.py
Outdated
| @@ -351,6 +351,10 @@ def _common_fields_to_dict(self) -> Dict[str, Any]: | |||
| "expires": self.expires.isoformat() + "Z", | |||
| } | |||
|
|
|||
| def is_expired(self, reference_time=datetime.now()) -> bool: | |||
There was a problem hiding this comment.
reference_time should be type annotated, no? Also more importantly this looks like a mutable default argument and may be a bug
There was a problem hiding this comment.
In addition, datetime.now() returns local time and date. The recommended way to get UTC time seems to be:
datetime.now(timezone.utc)
There was a problem hiding this comment.
This is a good point ... but I think our expiry datetimes are timezone-naive so datetime.utcnow() might actually be what we want (or alternatively we should be creating expiry datetime as timezone-aware)
There was a problem hiding this comment.
well the tests seem happy so maybe I was wrong and our expiry datetime is not naive?
There was a problem hiding this comment.
Looks good, except for the docstring format (TUF is full of the format you used but we're trying to move to standard docstrings). I think I have a suggestion that's correct below...
Feel free to squash all of this into a single commit (btw, commit message titles preferred under 50 characters).
tuf/api/metadata.py
Outdated
| """ | ||
| <Purpose> | ||
| Checks metadata expiration against reference time. | ||
|
|
||
| <Arguments> | ||
| reference_time: | ||
| The time to check the metadata expiration date against. | ||
| A naive datetime in UTC expected. | ||
| If not provided, checks against the current UTC date and time. | ||
|
|
||
| <Returns> | ||
| True if the reference time is equal or | ||
| greater than the metadata expiration date. | ||
| """ |
There was a problem hiding this comment.
Thanks this helps... but it's the old style doc format which we're hoping to not use in api/
| """ | |
| <Purpose> | |
| Checks metadata expiration against reference time. | |
| <Arguments> | |
| reference_time: | |
| The time to check the metadata expiration date against. | |
| A naive datetime in UTC expected. | |
| If not provided, checks against the current UTC date and time. | |
| <Returns> | |
| True if the reference time is equal or | |
| greater than the metadata expiration date. | |
| """ | |
| """Checks metadata expiration against a reference time. | |
| Args: | |
| reference_time: Optional; The time to check expiration date against. | |
| A naive datetime in UTC expected. | |
| If not provided, checks against the current UTC date and time. | |
| Returns: | |
| True if expiration time is less than the reference time. | |
| """ |
I think this matches the current style (https://google.github.io/styleguide/pyguide.html#383-functions-and-methods) ... but I've never used this style yet either so maybe @MVrachev or someone more familiar can confirm if this is what we want for new code?
In any case let's try to keep it to as few lines as possible.
There was a problem hiding this comment.
Looks good, should be ok with the new style.
There was a problem hiding this comment.
Thank you all for the comments! I believe I addressed all issues and pushed a squashed commit. I promise to carefully go through the guidelines again and to do better with observing best practices and recommendations next time :)
Checks metadata expiration against a reference time (a naive datetime in UTC). If not provided, checks against the current UTC date and time. Returns True if expiration time is less than the reference time. Signed-off-by: Velichka Atanasova <avelichka@vmware.com>
Adds the is_expired method to the Signed metadata class that checks the expires attribute against a reference time and returns information about metadata expiration
Signed-off-by: Velichka Atanasova avelichka@vmware.com
Please fill in the fields below to submit a pull request. The more information
that is provided, the better.
Fixes #1305
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following
requirements: