-
Notifications
You must be signed in to change notification settings - Fork 429
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
feat: Switch existing task processor health checks to new liveness probe #5161
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5161 +/- ##
==========================================
+ Coverage 97.47% 97.49% +0.02%
==========================================
Files 1224 1223 -1
Lines 42590 42581 -9
==========================================
+ Hits 41515 41516 +1
+ Misses 1075 1065 -10 ☔ View full report in Codecov by Sentry. |
|
||
sys.exit(0 if 200 >= status < 300 else 1) | ||
def main() -> None: |
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.
Can we add a deprecation warning when this is being used? In case we need to troubleshoot other customer deployments that have similar issues, this could tell us immediately if they are using the old style healthchecks or not.
I understand this code is only ever called by Docker Compose healthchecks, which should be removed.
Otherwise LGTM 👍
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.
Done.
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.
I am slightly concerned with the deprecation warning that's been added here since, if I understand correctly, anyone invoking python manage.py checktaskprocessorthreadhealth
will also see this confusing deprecation warning.
I think that could be anyone who explicitly uses the management command, but more likely anyone that updates the flagsmith version without updating their chart version?
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.
Valid point. I've reworked the warning to be more descriptive.
|
||
sys.exit(0 if 200 >= status < 300 else 1) | ||
def main() -> None: |
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.
I am slightly concerned with the deprecation warning that's been added here since, if I understand correctly, anyone invoking python manage.py checktaskprocessorthreadhealth
will also see this confusing deprecation warning.
I think that could be anyone who explicitly uses the management command, but more likely anyone that updates the flagsmith version without updating their chart version?
a7cf5ed
to
ac086b4
Compare
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
This accommodates for changes introduced in Flagsmith/flagsmith-task-processor#25 by intercepting
python manage.py checktaskprocessorthreadhealth
command and redirecting it to new liveness probe introduced in #5151.How did you test this code?
Ran
python manage.py checktaskprocessorthreadhealth
and made sure the correct HTTP call is made.