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.

Sign up for GitHub

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

Jump to bottom

fix(task-processor): implement grace period for deleting old recurring task #3169

Merged
merged 4 commits into from
Dec 21, 2023
Merged

fix(task-processor): implement grace period for deleting old recurring task #3169

merged 4 commits into from
Dec 21, 2023

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented Dec 15, 2023 •
edited
Loading

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Fixes #2551 by implementing a grace period to not remove new tasks during deployment

How did you test this code?

Updates/Adds unit test

@gagantrivedi gagantrivedi marked this pull request as ready for review December 15, 2023 08:44
@vercel Vercel
Copy link

vercel bot commented Dec 15, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 3:44am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 3:44am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 3:44am

@github-actions github-actions bot added the api Issue related to the REST API label Dec 15, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 15, 2023 •
edited
Loading

Uffizzi Preview deployment-42906 was deleted.

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (79e67ee) 96.11% compared to head (b9b1c14) 95.99%.
Report is 35 commits behind head on main.

Files Patch % Lines
...sk_processor/test_unit_task_processor_processor.py 94.73% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3169      +/-   ##
==========================================
- Coverage   96.11%   95.99%   -0.12%     
==========================================
  Files        1059     1062       +3     
  Lines       32036    32265     +229     
==========================================
+ Hits        30791    30973     +182     
- Misses       1245     1292      +47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zachaysan
zachaysan reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

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

Wouldn't the issue be solved if we could check the created_at on the task_processor_recurringtask and if the task is sufficiently old it could be deleted?

@gagantrivedi
Copy link
Member Author

Wouldn't the issue be solved if we could check the created_at on the task_processor_recurringtask and if the task is sufficiently old it could be deleted?

Yeah, good shout! I have added the code to use a grace period

@gagantrivedi gagantrivedi changed the title fix(task-processor): do not delete tasks if not found in registry fix(task-processor): implement grace period for deleting old recurring task Dec 20, 2023
zachaysan
zachaysan approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

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

One minor nit but the rest looks good

api/tests/unit/task_processor/test_unit_task_processor_processor.py Outdated
Comment on lines 198 to 201
# make the task old
RecurringTask.objects.update(
created_at=timezone.now() - UNREGISTERED_RECURRING_TASK_GRACE_PERIOD
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: If we can use freezegun that would be better.

@gagantrivedi gagantrivedi added this pull request to the merge queue Dec 21, 2023
Merged via the queue into main with commit 00f0552 Dec 21, 2023
@gagantrivedi gagantrivedi deleted the fix/2551 branch December 21, 2023 03:47
@dabeeeenster dabeeeenster mentioned this pull request Dec 20, 2023
Merged
@dabeeeenster dabeeeenster mentioned this pull request Apr 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachaysan zachaysan zachaysan approved these changes

Assignees
No one assigned
Labels
api Issue related to the REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Creating new Recurring Tasks only happens on second release
3 participants
@gagantrivedi @codecov-commenter @zachaysan