Centralize check registration / running logic #85
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 97.84% 98.23% +0.39%
==========================================
Files 18 19 +1
Lines 602 624 +22
Branches 88 85 -3
==========================================
+ Hits 589 613 +24
+ Misses 9 7 -2
Partials 4 4 ☔ View full report in Codecov by Sentry. |
- remove version check - autouse so that checks are cleared after every test run
grahamalama
force-pushed
the
centralized-check-registry
branch
from
9cb8198
to
fdc0148
Compare
grahamalama
force-pushed
the
centralized-check-registry
branch
from
1d0e0f9
to
c5da374
Compare
grahamalama
force-pushed
the
centralized-check-registry
branch
from
c5da374
to
7776fcf
Compare
src/dockerflow/checks/registry.py
Outdated
| Adds a given check callback with the provided object to the list | ||
| of checks. Useful for built-ins but also advanced custom checks. | ||
| """ | ||
| logger.debug("Adding extension check %s" % check.__name__) |
There was a problem hiding this comment.
I see that this comes from the Django implementation, but I don't really understand the vocabulary here: why is it an extension check?
Is it specific to Django built-in checks or are we going to use this in other integrations? Maybe it should remain under dockerflow.django
There was a problem hiding this comment.
These are great questions. I had ones that were similar. The init_check / "extension check" vocabulary existed before this PR -- it was a pattern for Flask and Sanic. I don't actually see any traces of it in Django 🤔.
It does seem like it could be reworked. I'll give it a try in this PR, but if things get too crazy, maybe it deserves its own.
There was a problem hiding this comment.
Oops you're right, misread the diff at the moment of writing the comment :)
Don't hesitate to open a follow-up if you think that's too much for this PR
src/dockerflow/checks/registry.py
Outdated
| def init_check(check, obj): | ||
| """ | ||
| Adds a given check callback with the provided object to the list | ||
| of checks. Useful for built-ins but also advanced custom checks. |
There was a problem hiding this comment.
Why init check?
Could we add types and/or docstrings for the input parameters for better clarity? Or provide an example on how to use this init_check()?
src/dockerflow/checks/registry.py
Outdated
| Adds a given check callback with the provided object to the list | ||
| of checks. Useful for built-ins but also advanced custom checks. | ||
| """ | ||
| logger.debug("Adding extension check %s" % check.__name__) |
There was a problem hiding this comment.
These are great questions. I had ones that were similar. The init_check / "extension check" vocabulary existed before this PR -- it was a pattern for Flask and Sanic. I don't actually see any traces of it in Django 🤔.
It does seem like it could be reworked. I'll give it a try in this PR, but if things get too crazy, maybe it deserves its own.
|
@grahamalama shall we resume this work? |
leplatrem
force-pushed
the
centralized-check-registry
branch
from
3386818
to
80ef729
Compare
This reverts commit 80ef729.
|
@grahamalama r? |
|
Nice @leplatrem, thanks for taking this over and finishing it 🙌🏻 But oh no, I can't actually approve this PR because I was an author on some of the commits 😱. You could pull another reviewer in, or you can consider this my approval and bypass branch protections. |
* Add tox config for FastAPI 100 * Add dockerflow router to package root * Add request summary middleware * Add version endpoint and tests * Add initial heartbeat route, check mechanism, and tests * Refactor package structure, names of things - Define "router" in package init, register view functions defined in views module - Add test for check with custom name - use better name for check operations * Leverage centralized checks (#85) * Do not test on Python 3.7 * Add 'taskName' to excluded fields * Test with python 3.12 * Improve test coverage of FastAPI * Blackify * FastAPI integration docs (#95) * Add FastAPI documentation * Update docs/fastapi.rst Co-authored-by: grahamalama <graham.beckley@gmail.com> * Mention APP_DIR for the version file --------- Co-authored-by: grahamalama <graham.beckley@gmail.com> * Update tox.ini Co-authored-by: grahamalama <graham.beckley@gmail.com> --------- Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
This PR adds to the
dockerflow.checkspackage to provide the ability to register checks, run registered checks (both synchronously and asynchronously), and return the results of the checks in a standardized manner.Since Django already has the concept of a check registry, we just run the checks there (but with our check runner). For other frameworks, we use the new registry in
dockerflow.checks.registry.Still todo is
add tests for the check running functions(to make assertions about the shape of the data they return) and update documentation. But I'm open to review in this PR's current state.