-
Notifications
You must be signed in to change notification settings - Fork 5
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
[RSPEED-427] Add systems endpoint #14
[RSPEED-427] Add systems endpoint #14
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #14 +/- ##
===========================================
+ Coverage 80.82% 89.39% +8.57%
===========================================
Files 9 17 +8
Lines 73 132 +59
===========================================
+ Hits 59 118 +59
Misses 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looking good so far!
5dab81d
to
25c0548
Compare
25c0548
to
cbc7fdf
Compare
v1_router = APIRouter() | ||
|
||
|
||
@v1_router.get("/systems") |
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.
nit: REST API endpoints should have a trailing slash. Resources can omit the trailing slash (such as a request with path parameters).
This is a highly contentious topic and that's just my opinion.
Internally, FastAPI registers both routes (with and without a trailing slash) and issues a 307 redirect to get to real endpoint.
Add endpoint for returning RHEL systems start/end dates
cbc7fdf
to
2e8f957
Compare
|
||
@v1_router.get("/systems/{major}") | ||
async def get_systems_major(major: int = Path(..., description="Major version number")): | ||
systems = get_systems_data(major) |
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.
This is a call to a sync function from an async function. I need to look into this more, but I think this blocks the event loop.
I think the correct way to call sync code from within async code is to use the default event loop with a Future object.
systems = get_systems_data(major) | |
loop = asyncio.get_running_loop() | |
systems = await loop.run_in_executor(None, get_systems_data(major)) |
This isn't a problem currently because the blocking sync code is fast. But for a real IO or CPU bound operation, this could cause perceptible performance degradation.
major: int = Path(..., description="Major version number"), | ||
minor: int = Path(..., description="Minor version number"), |
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.
A better way to handle this is to use Annotated.
major: int = Path(..., description="Major version number"), | |
minor: int = Path(..., description="Minor version number"), | |
import typing as t | |
major: t.Annotated[int, Path(description="Major version number", ge=8, le=10)], | |
minor: t.Annotated[int, Path(description="Minor version number", ge=0, le=99)], |
It's also unnecessary to pass the ellipsis to Path
.
Add endpoint for returning RHEL systems start/end dates
TODO: