feat(breadbox): Add optional expiration date to transient datasets #364
Conversation
Two changes: 1. when a dataset is uploaded, it can now be assigned an expiration date. 2. Added a new bb command which actually performs deletes of expired datasets (or those transient datasets over a given age)
| @@ -66,6 +67,11 @@ def add_dataset_uploads( | |||
| """ | |||
| utils.check_celery() | |||
|
|
|||
| if not dataset.is_transient and dataset.expiry_in_seconds is not None: | |||
There was a problem hiding this comment.
I personally prefer adding these types of validations within the pydantic model itself since it feels a bit cleaner and will catch malformed request body faster. In addition, this prevents us from needing to do asserts or similar checks later on which I notice you do later on in breadbox/breadbox/compute/dataset_uploads_tasks.py:87
There was a problem hiding this comment.
I'm less a fan of validations within the pydanic model ... but only for reasons that I'd say more "habit" then anything.
I agree, this is a validation which would make perfect sense to include in the pydantic model. I'll fix.
| @@ -262,6 +262,7 @@ def upload_dataset( | |||
| allowed_values=valid_fields.valid_allowed_values, | |||
| dataset_metadata=dataset_metadata, | |||
| dataset_md5=None, | |||
| expiry=None, | |||
There was a problem hiding this comment.
I see that we are manually setting this field multiple places even though it is already default to None. Is there a reason we need to explicitly set this?
There was a problem hiding this comment.
It's being set to None in the legacy code paths. I've only updated the dataset-v2 endpoint's code path to be aware of expiry.
There was a problem hiding this comment.
Ah, I see what you're saying. On the pydantic model expiry is specified as:
expiry: Annotated[Optional[datetime], Field(default=None,)]
I'd love to leave out the expiry=None here, but pyright doesn't seem to be aware of the the defaulting and complained that I wasn't providing expiry.
| @@ -677,6 +678,35 @@ def update_dataset( | |||
| return dataset | |||
|
|
|||
|
|
|||
| def get_current_datetime(): | |||
| # this method only exists to allow us to mock it in tests. Since `datetime` is a built-in we're not able to | |||
There was a problem hiding this comment.
Thanks adding this informational comment!
Two changes: