-
Notifications
You must be signed in to change notification settings - Fork 21
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
Dynamic Pagination in CPT Dashboard #135
Changes from 6 commits
9907ba5
e511db2
69e6bdc
a089841
23ece7e
2a086b0
946c52d
0d7d54f
11b5c07
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 |
---|---|---|
|
@@ -4,25 +4,30 @@ | |
from app.services.search import ElasticService | ||
|
||
|
||
async def getData(start_datetime: date, end_datetime: date, configpath: str): | ||
async def getData( | ||
start_datetime: date, end_datetime: date, size: int, offset: int, configpath: str | ||
): | ||
query = { | ||
"size": size, | ||
"from": offset, | ||
"query": { | ||
"bool": {"filter": {"range": {"timestamp": {"format": "yyyy-MM-dd"}}}} | ||
} | ||
}, | ||
} | ||
|
||
es = ElasticService(configpath=configpath) | ||
response = await es.post( | ||
query=query, | ||
size=size, | ||
start_date=start_datetime, | ||
end_date=end_datetime, | ||
timestamp_field="timestamp", | ||
) | ||
await es.close() | ||
tasks = [item["_source"] for item in response] | ||
tasks = [item["_source"] for item in response["data"]] | ||
jobs = pd.json_normalize(tasks) | ||
if len(jobs) == 0: | ||
return jobs | ||
return {"data": jobs, "total": response["total"]} | ||
|
||
jobs[ | ||
["masterNodesCount", "workerNodesCount", "infraNodesCount", "totalNodesCount"] | ||
|
@@ -52,7 +57,7 @@ async def getData(start_datetime: date, end_datetime: date, configpath: str): | |
jbs = cleanJobs | ||
jbs["shortVersion"] = jbs["ocpVersion"].str.slice(0, 4) | ||
|
||
return jbs | ||
return {"data": jbs, "total": response["total"]} | ||
Comment on lines
57
to
+60
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. What does cleanJobs come from? It may benefit from a comment. And I do not think jbs makes sense as a variable name. It's just confusing and looks like a typo. Dave also questioned this. 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.
What bugs me here is the 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. We will definitely need to keep a note of this for a future refactor PR. |
||
|
||
|
||
def fillEncryptionType(row): | ||
|
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.
It would reduce the code complexity if you just included those two lines in a
len(jobs) != 0
case instead of having an identical return early.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.
There are a lot of potential refactoring ideas we should pursue to make the somewhat messy codebase more maintainable.
Right now, we've got a long list of PRs we'd really like to land, and we need to focus on the most critical concerns ... starting with landing this PR, #138, and then a follow-on filtering PR which ought to complete the "revamp" branch. Once we land that onto "main" we can churn through my backlog of ilab PRs.
At that point, our focus should be cleaning up the code base (including unit testing, functional testing, lint and format checkers) to make it more maintainable with a viable CI.
As much as I hate being "pragmatic" in things like this, let's not let minor stuff like this bog us down at this point. Maybe leaving these comments in place and open as a reference for later work isn't a bad idea ...