fix(ci): handle missing server log files in cleanup steps#769
fix(ci): handle missing server log files in cleanup steps#769TimeToBuildBob wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
When aw-server-rust doesn't produce log files (e.g. logs go to stdout/stderr instead of files), the glob pattern in the cleanup steps fails because bash treats unmatched globs as literal strings. Fix: use `shopt -s nullglob` so unmatched globs expand to nothing, and switch `mv` to a for-loop that handles the empty case. This was causing false CI failures on the "aw-server-rust master" test matrix entry for every PR.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to ccc77c4 in 5 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_IdHOhFLWNdIo4mzL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryThis PR fixes false CI failures in the The fix applies two changes:
Both the "Print server logs to console" and "Move logs to subdir" steps are now properly protected against missing log files. The changes are minimal, targeted, and preserve the existing behavior when log files do exist. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: ccc77c4 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #769 +/- ##
=======================================
Coverage 25.60% 25.60%
=======================================
Files 30 30
Lines 1746 1746
Branches 306 306
=======================================
Hits 447 447
Misses 1277 1277
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm pretty sure the root cause was ActivityWatch/aw-server-rust#565 which was merged? We just need another commit to master/rerun CI to address/verify this, or? |
|
Actually, master seems to be passing? |
|
Right, aw-server-rust#565 fixed the root cause (server now writes log files to the expected location). If master CI is passing now, this PR is just a defensive fix — handles the edge case where log files are missing for any reason so the cleanup step doesn't fail. Happy to close this if you think it's unnecessary now, or merge it as a robustness improvement. Your call! |
|
Closing, we should fail on the empty case as it suggests logs weren't being written or picked up. |
Summary
aw-server-rust mastertest matrix entry~/.cache/activitywatch/log/*/*.loghas no matches, causing bash to treat it as a literal string →cat/mvfails → step fails → job shows as redFix
shopt -s nullglobso unmatched globs expand to nothingmvglob to a for-loop that handles the empty case gracefullyContext
Re-submission of #768 which was mistakenly closed — #767 (date range picker fix) was merged but is unrelated to this CI issue. The log cleanup failure still occurs on every PR's
aw-server-rust mastermatrix entry.Important
Fixes CI false failures by handling missing log files with
shopt -s nullgloband a for-loop innodejs.yml.shopt -s nullglobinnodejs.ymlto handle unmatched globs, preventing false CI failures.mvcommand with a for-loop to handle empty log file cases gracefully.aw-server-rust mastertest matrix entry.This description was created by
for ccc77c4. You can customize this summary. It will automatically update as commits are pushed.