-
Notifications
You must be signed in to change notification settings - Fork 19
feat(notifications): add hardware_summary action #1610
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
base: main
Are you sure you want to change the base?
feat(notifications): add hardware_summary action #1610
Conversation
629b5a1 to
269cd86
Compare
|
Hello and thanks for the contribution! We will check this PR as soon as we can. For now, please check the lint rules, you can use the linter locally to check for required changes. Commit and PR description looks good! I glanced over the changes and they look well written, I would just make a nitpick for leaving a |
|
Hello, Thanks for this. Can you also attach and example output from running this command. I want to review how the report output is coming out. |
269cd86 to
2f1cc1a
Compare
Add new hardware_summary action in the notifications management command. This action generates hardware reports based on the subscription file. Signed-off-by: Yushan Li <[email protected]>
2f1cc1a to
603424a
Compare
Thanks for your feedback! Here’s an example report output, showing only a limited number of items for clarity. |
|
It is looking great. Thanks for sharing. I am fine with the output being generated and with the yaml config file for hardware. I'll let the Dashboard team review it in detail and comment on the implementation. |
MarceloRobert
left a comment
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.
Could you update the notifications.md document with this new command too?
| checkouts.git_commit_name AS build__checkout__git_commit_name, | ||
| checkouts.git_commit_hash AS build__checkout__git_commit_hash, | ||
| checkouts.tree_name AS build__checkout__tree_name, | ||
| checkouts.origin AS build__checkout__origin |
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.
The reason we have some renamings in our queries is that django uses this format of foreignkey__field to get fields from other tables, and the renaming was keeping compatibility for some functions. In this new query, if you know where you are going to use this data, you don't really need to rename them with this double underscore format; it's not a big problem though, just not needed I think
| tests.misc ->> 'runtime' AS runtime, | ||
| tests.environment_misc ->> 'job_id' AS job_id, |
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.
thought you can select these values at query level, I would prefer if you got them from the already-fetched fields at the Python level, which might have less impact in the database
| return dict_fetchall(cursor) | ||
|
|
||
|
|
||
| # select the detailed data depends on hardware_id |
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: If you want to add a comment explaining the function, prefer using a docstring at the beginning of the function
| {{- "{:>5}".format(test_status_group["inconclusive"]) }} ⚠️ | ||
| ------------- | ||
| {% for data in hardware_data %} | ||
| #kernelci test {{ data["id"] }} |
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.
Depends on what you want, but would it make sense to show the test path here, instead of the id?
| hardwares_data_raw = get_hardware_summary_data( | ||
| keys=list(hardware_key_set), | ||
| start_date=start_date, | ||
| end_date=end_date, | ||
| ) |
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.
I know that usually there will be results, but just for precaution I would add a
if not hardwares_data_raw:
print("No data for hardware summary")
returnso that the command can return gracefully
| # extract recipient | ||
| for (hardware_id, origin), report_configs in hardware_prop_map.items(): | ||
| for hardware_report in report_configs: | ||
| recipients = process_submission_options( | ||
| default_recipients=hardware_report.get("default_recipients", []), | ||
| specific_recipients=hardware_report.get("recipients", []), | ||
| options=hardware_report.get("options", []), | ||
| ) |
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.
shouldn't this block be getting only the recipient for the specific hardware in this loop? it seems like it is getting the recipients for all reports and using only the last one.
Also, since you already have (hardware_id, origin), you could do a report_configs = hardware_prop_map[(hardware_id, origin)] and follow that
| start_date = now - timedelta(days=30) | ||
| end_date = now |
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.
should the user be able to change this value? 30 days seem to be long, maybe the user just wants a recent summary
Description
Add a new hardware_summary action to the notifications management command.
This action generates hardware reports based on the hardware subscription files.
Changes
hardware_summaryaction to the notifications command with CLI argument.How to Test
Run the following command to generate the hardware summary report:
Related Issue
Closes #1081