-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add cron to cleanup dangling single tests and batch periodic test reports #1579
base: main
Are you sure you want to change the base?
Conversation
a3fa3d0
to
c744619
Compare
Signed-off-by: Sasha Romijn <[email protected]>
migrations.AddField( | ||
model_name="webtestappsecpriv", | ||
name="timestamp", | ||
field=models.DateTimeField(auto_now_add=True, default=datetime.datetime(1, 1, 1, 0, 0)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will auto_now_add also apply to existing rows at the time of migration? Or will it only do that when adding a new object through ORM? Otherwise, we will be expiring currently existing reports early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should set the date to epoch(0) for existing records. I will test it out to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but that's bad, right? If the cleanup runs right after deployment, it'll consider every report old, and start cleaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup does not touch single test reports, only probe/test results. It will delete results that don't have a report associated and are not recent enough to be currently running. The timestamp is added to these 2 models because they don't have it yet while other tests in the report do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
No description provided.