-
Notifications
You must be signed in to change notification settings - Fork 424
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Docker builds report
|
Uffizzi Ephemeral Environment
|
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.
In 121128e, I added support for the new Task processor endpoint that now accepts command line arguments for Gunicorn as well. This enables HTTP healthchecks directly on Task processor containers. We'll need to merge Flagsmith/flagsmith-task-processor#25 and change the dependency reference before we merge this. I also refreshed documentation related to Task processor operation. |
e9ae682
to
8be11b8
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.