-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
from datetime import date | ||
|
||
OS_DATA_MOCKED = [ | ||
{ | ||
"name": "RHEL", | ||
"major": 9, | ||
"minor": 2, | ||
"release_date": date(2023, 5, 1), | ||
"retirement_date": date(2023, 11, 1), | ||
"systems": 5, | ||
"lifecycle_type": "mainline", | ||
}, | ||
{ | ||
"name": "RHEL", | ||
"major": 8, | ||
"minor": 3, | ||
"release_date": date(2020, 11, 1), | ||
"retirement_date": date(2021, 5, 1), | ||
"systems": 50, | ||
"lifecycle_type": "eus", | ||
}, | ||
{ | ||
"name": "RHEL", | ||
"major": 8, | ||
"minor": 7, | ||
"release_date": date(2023, 5, 1), | ||
"retirement_date": date(2023, 5, 1), | ||
"systems": 12, | ||
"lifecycle_type": "e4s", | ||
}, | ||
{ | ||
"name": "RHEL", | ||
"major": 9, | ||
"minor": 0, | ||
"release_date": date(2022, 5, 18), | ||
"retirement_date": date(2032, 5, 1), | ||
"systems": 45, | ||
"lifecycle_type": "mainline", | ||
}, | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import pytest | ||
from fastapi.testclient import TestClient | ||
|
||
import app | ||
from app.main import app as application | ||
|
||
client = TestClient(application) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
("source_data", "path", "response"), | ||
( | ||
( | ||
[{"major": 8, "minor": 3, "data": "data"}, {"major": 8, "minor": 4, "data": "data"}], | ||
"/8/3", | ||
[{"major": 8, "minor": 3, "data": "data"}], | ||
), | ||
( | ||
[{"major": 8, "minor": 3, "data": "data"}, {"major": 9, "minor": 0, "data": "data"}], | ||
"/9/0", | ||
[{"major": 9, "minor": 0, "data": "data"}], | ||
), | ||
([{"major": 8, "minor": 3, "data": "data"}, {"major": 9, "minor": 0, "data": "data"}], "/9/20", []), | ||
([], "/9/20", []), | ||
( | ||
[ | ||
{"major": 8, "minor": 3, "data": "data"}, | ||
{"major": 9, "minor": 0, "data": "data"}, | ||
{"major": 8, "minor": 7, "data": "data"}, | ||
{"major": 9, "minor": 2, "data": "data"}, | ||
], | ||
"/9", | ||
[ | ||
{"major": 9, "minor": 0, "data": "data"}, | ||
{"major": 9, "minor": 2, "data": "data"}, | ||
], | ||
), | ||
( | ||
[ | ||
{"major": 8, "minor": 3, "data": "data"}, | ||
{"major": 9, "minor": 0, "data": "data"}, | ||
{"major": 8, "minor": 7, "data": "data"}, | ||
{"major": 9, "minor": 2, "data": "data"}, | ||
], | ||
"", | ||
[ | ||
{"major": 8, "minor": 3, "data": "data"}, | ||
{"major": 8, "minor": 7, "data": "data"}, | ||
{"major": 9, "minor": 0, "data": "data"}, | ||
{"major": 9, "minor": 2, "data": "data"}, | ||
], | ||
), | ||
), | ||
) | ||
def test_system_specified(source_data, path, response, monkeypatch): | ||
monkeypatch.setattr(app.v1.lifecycle.systems.endpoints, "OS_DATA_MOCKED", source_data) | ||
|
||
data = client.get(f"/api/digital-roadmap/v1/lifecycle/systems{path}") | ||
assert data.json() == response |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from fastapi import APIRouter | ||
|
||
from app.v1.lifecycle.systems.endpoints import v1_router as systems_v1_router | ||
|
||
v1_router = APIRouter() | ||
|
||
v1_router.include_router(systems_v1_router, tags=["lifecycle-systems"]) |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,40 @@ | ||||||||||||
from fastapi import APIRouter, Path | ||||||||||||
|
||||||||||||
from app.data.systems import OS_DATA_MOCKED | ||||||||||||
|
||||||||||||
v1_router = APIRouter() | ||||||||||||
|
||||||||||||
|
||||||||||||
@v1_router.get("/systems") | ||||||||||||
async def get_systems(): | ||||||||||||
systems = get_systems_data() | ||||||||||||
|
||||||||||||
return sorted(systems, key=lambda d: (d["major"], d["minor"])) | ||||||||||||
|
||||||||||||
|
||||||||||||
@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 commentThe 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.
Suggested change
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. |
||||||||||||
|
||||||||||||
return sorted(systems, key=lambda d: (d["major"], d["minor"])) | ||||||||||||
|
||||||||||||
|
||||||||||||
@v1_router.get("/systems/{major}/{minor}") | ||||||||||||
async def get_systems_major_minor( | ||||||||||||
major: int = Path(..., description="Major version number"), | ||||||||||||
minor: int = Path(..., description="Minor version number"), | ||||||||||||
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A better way to handle this is to use Annotated.
Suggested change
It's also unnecessary to pass the ellipsis to |
||||||||||||
): | ||||||||||||
systems = get_systems_data(major, minor) | ||||||||||||
|
||||||||||||
return sorted(systems, key=lambda d: (d["major"], d["minor"])) | ||||||||||||
|
||||||||||||
|
||||||||||||
def get_systems_data(major=None, minor=None): | ||||||||||||
data = OS_DATA_MOCKED | ||||||||||||
|
||||||||||||
if major is not None: | ||||||||||||
data = [d for d in data if d["major"] == major] | ||||||||||||
if minor is not None: | ||||||||||||
data = [d for d in data if d["minor"] == minor] | ||||||||||||
|
||||||||||||
return data |
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.