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

Asset snapshot cleanup task #4901

Merged
merged 13 commits into from
Apr 23, 2024
Merged

Asset snapshot cleanup task #4901

merged 13 commits into from
Apr 23, 2024

Conversation

jamesrkiger
Copy link
Contributor

@jamesrkiger jamesrkiger commented Apr 10, 2024

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

Adds regular asset snapshot cleanup maintenance task. Reworks existing test to make sure latest versioned snapshots are preserved.

Notes

I would appreciate feedback on organization here. KPI already has a tasks.py file for celery tasks, and the new remove_old_snapshots() is not a celery task, so I didn't want to put it there. Currently it is in a new maintenance_tasks.py file. I was thinking of putting them both in a tasks folder. One could be celery_tasks.py and the other could stay as maintenance_tasks.py.

Down the line, I'm also wondering if I should be removing the existing management command code (here for snapshots and here for the DeleteBaseCommand class) once I replace the other task (removing old import tasks) that relied on it.

Related issues

Partially replaces work in #2434

@jamesrkiger jamesrkiger marked this pull request as draft April 10, 2024 14:30
@jamesrkiger jamesrkiger marked this pull request as ready for review April 11, 2024 13:29
@jamesrkiger jamesrkiger changed the title draft: Asset snapshot cleanup Asset snapshot cleanup task Apr 11, 2024
@jamesrkiger jamesrkiger requested a review from bufke April 11, 2024 13:40
Copy link
Contributor

@bufke bufke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also wondering if I should be removing the existing management command code (here for snapshots and here for the DeleteBaseCommand class) once I replace the other task (removing old import tasks) that relied on it.

Yeah for sure. Either now or later. We may want to consider our alternative abstractions like RemoveBaseCommand but not until we have more code to actually abstract.

kpi/maintenance_tasks.py Show resolved Hide resolved
kobo/tasks.py Outdated Show resolved Hide resolved
kpi/maintenance_tasks.py Outdated Show resolved Hide resolved
kpi/maintenance_tasks.py Outdated Show resolved Hide resolved
@jamesrkiger jamesrkiger requested a review from noliveleger April 19, 2024 18:51
Copy link
Contributor

@bufke bufke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending consideration of my comments.

kpi/tasks.py Outdated Show resolved Hide resolved
kpi/tasks.py Outdated Show resolved Hide resolved
kpi/tasks.py Outdated Show resolved Hide resolved
kpi/tasks.py Outdated Show resolved Hide resolved
kpi/maintenance_tasks.py Outdated Show resolved Hide resolved
kpi/tasks.py Outdated Show resolved Hide resolved
kpi/tests/test_asset_snapshots.py Outdated Show resolved Hide resolved
kpi/tests/test_asset_snapshots.py Outdated Show resolved Hide resolved
kpi/tests/test_asset_snapshots.py Show resolved Hide resolved
kpi/deployment_backends/mixin.py Show resolved Hide resolved
kpi/tests/test_asset_snapshots.py Outdated Show resolved Hide resolved
kpi/maintenance_tasks.py Outdated Show resolved Hide resolved
@noliveleger noliveleger self-assigned this Apr 23, 2024
@jamesrkiger jamesrkiger merged commit 409d7d5 into beta Apr 23, 2024
4 checks passed
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 this pull request may close these issues.

3 participants