Add integration for FastAPI #81
Conversation
- 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
src/dockerflow/fastapi/middleware.py
Outdated
There was a problem hiding this comment.
This middleware is based heavily on https://github.com/Kludex/asgi-logger/. What's interesting though is that while this is currently in the fastapi subpackage, as is implied in that project, we may be able to write one Asgi middleware for all ASGI servers / frameworks e.g..
src/dockerflow/fastapi/checks.py
Outdated
There was a problem hiding this comment.
It feels like we should define some of the stuff in this module more centrally. Each of the framework integrations define their own healthcheck registration mechanisms. Also, it seems like just convention that "a healthcheck function should return a list of CheckMessage objects". That seems like something we should enforce or standardize somehow.
There was a problem hiding this comment.
maybe... I'm getting more into typed Python, so I like CheckMessage. dataclass requires Python 3.7, we're close to that being a minimum. Re-writing the code to use common functions sounds like a different PR, and probably a different version. I'd want a release of the "boring" duplicate version before a major rewrite like that.
I think if we can find a Mozilla employee that will use this integration, than we are justified to add it. If it won't be used by anyone, it is not worth adding. I don't have any active FastAPI projects, so I'm not volunteering to sponsor. It has been a while since I've used FastAPI, but the code looks correct. I can probably dig in an give a full review later this week. |
src/dockerflow/fastapi/checks.py
Outdated
| errors = check() | ||
| level = max([0] + [e.level for e in errors]) | ||
| return CheckDetail( | ||
| status=level_to_text(level), level=level, messages={e.id: e.msg for e in errors} |
There was a problem hiding this comment.
This bit worried me - it looks like if two errors have the same level, just one will win:
messages={e.id: e.msg for e in errors}
However, the other platforms seem to work that way as well, so 🤷
* Add FastAPI documentation * Update docs/fastapi.rst Co-authored-by: grahamalama <[email protected]> * Mention APP_DIR for the version file --------- Co-authored-by: grahamalama <[email protected]>
| APIRoute("/__version__", endpoint=version, methods=["GET"]), | ||
| ], | ||
| ) | ||
| """This router adds the Dockerflow views.""" |
There was a problem hiding this comment.
Move this to the top of the file?
There was a problem hiding this comment.
This is the way Sphinx autodoc documents module attributes (docstring below)
Co-authored-by: grahamalama <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
- Coverage 98.23% 97.97% -0.27%
==========================================
Files 19 22 +3
Lines 624 692 +68
Branches 85 92 +7
==========================================
+ Hits 613 678 +65
- Misses 7 8 +1
- Partials 4 6 +2 ☔ View full report in Codecov by Sentry. |
Looking for some early feedback on this implementation of a Dockerflow integration with FastAPI.
Many more tests to write, but I wanted to get an early look at the general "API" of the integration (for now, documented only in how it's used in tests).
I'm also wondering if FastAPI is a library we want to actually support. It's a pretty popular library, but with all the talk at Mozilla about standardizing how we build products, it's worth considering if FastAPI is or should be one of those standard tools.