Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for collecting an SOS report as an optional attachment in bugit-v2 bug reports, and ensures the snap includes the required sosreport tooling.
Changes:
- Extend the
LogNamechoices to includesosreport. - Add a new
sos_reportlog collector and register it inreal_collectors(not collected by default). - Stage the
sosreportpackage into the snap so thesoscommand is available at runtime.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/bugit_v2/models/bug_report.py |
Adds sosreport to the log selection type. |
src/bugit_v2/dut_utils/log_collectors.py |
Implements and registers a new SOS report collector. |
snap/snapcraft.yaml |
Stages sosreport into the snap image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "snap-debug", | ||
| "long-job-outputs", | ||
| "oem-getlogs", | ||
| 'sosreport' |
There was a problem hiding this comment.
In this Literal[...] list, the rest of the entries use double quotes and a trailing comma on the last element. The newly added 'sosreport' uses single quotes and is missing a trailing comma, which will likely fail the repo’s formatter/linter expectations and makes the list inconsistent. Switch to "sosreport", and add the trailing comma.
| 'sosreport' | |
| "sosreport", |
| assert target_dir.is_dir() | ||
| out = await asp_check_output( | ||
| ["sos", "report", "--batch", f"--tmp-dir={target_dir}"], | ||
| timeout=600, # just in case |
There was a problem hiding this comment.
The collector hard-codes timeout=600 while the rest of this module consistently uses COMMAND_TIMEOUT (10 minutes) for long-running collectors and also advertises COMMAND_TIMEOUT in the UI. Use the shared COMMAND_TIMEOUT constant here (and avoid divergence if it changes later).
| timeout=600, # just in case | |
| timeout=COMMAND_TIMEOUT, # just in case |
| ) | ||
| # remove the sha file |
There was a problem hiding this comment.
This collector currently relies on sos report to place its generated archive into target_dir, but unlike other collectors it doesn’t explicitly write/copy any artifact into target_dir (it only sets --tmp-dir). If sos writes the final report to its default output directory, nothing will be uploaded from the attachment directory. Make the output location explicit (e.g. pass an output-dir option if supported, run with cwd=target_dir, and/or move the generated sosreport-*.tar.* into target_dir) and consider validating that an archive was created.
| ) | |
| # remove the sha file | |
| cwd=target_dir, | |
| ) | |
| # ensure an archive was actually created in the target directory | |
| archives = list(target_dir.glob("sosreport-*.tar.*")) | |
| if not archives: | |
| raise RuntimeError(f"sos_report did not create an archive in {target_dir}") | |
| # remove the sha file(s) |
| # remove the sha file | ||
| for file in target_dir.iterdir(): | ||
| if file.name.startswith("sosreport") and file.name.endswith(".sha256"): | ||
| os.remove(file) |
There was a problem hiding this comment.
When removing the .sha256 file, prefer Path.unlink() (consistent with other code using Path APIs) and consider using file.unlink(missing_ok=True) to avoid races if the file disappears between iterdir() and removal.
| os.remove(file) | |
| file.unlink(missing_ok=True) |
No description provided.