Implement Github action #52
Conversation
|
Very verbose output. |
|
Awesome. How about output formats? Current the report is in a format that would works as a comment on the PR. In packagecontrol/st-package-reviewer-action#5 I've made it output in the message format that GitHub can use to present annotations on the PR. Example: 
Since we're here we might be able to change the output format at the root of the reviewer script instead of transforming it later? |
|
That was not in the plan. But if you already, just recently, patched the reviewer you likely remember the code path. (?) For the action.sh here ... should that be in Python to make something like that easier? Let's just look at the function signature, what we have is If the reviewer output is stable, we could read and transform it to What would be the title? How does the typical output look like? "Reporting 3 errors..." Maybe that's easier if we just do it here. Formatters are applied as late as possible so that we get a good look on CLI and on CI. |
|
I have this (possibly bad) idea wherein you use a basic logger for CLI running and an alternative GitHub logger for the Action output. I don't really want to push back release, though. Happy to file a discussion or issue later. 🙂 |
|
It depends on what we have to support. It is enterprisy if diff --git a/gh_action/action.sh b/gh_action/action.sh
index 8f235c8..9d70159 100644
--- a/gh_action/action.sh
+++ b/gh_action/action.sh
@@ -259,7 +259,17 @@ PY
fi
echo " Reviewing with st_package_reviewer: $topdir" >&2
- if ! uv run st_package_reviewer "$topdir"; then
+ if ! uv run st_package_reviewer "$topdir" | awk '
+ /^Reporting [0-9]+ errors:/ { mode = "error"; next }
+ /^Reporting [0-9]+ warnings:/ { mode = "warning"; next }
+ /^- / && mode {
+ sub(/^- /, "");
+ print "::" mode "::" $0;
+ next;
+ }
+ { mode = ""; print }
+ ';
+ then
echo " ! Review failed for $pkg@$disp_ver" >&2
failures=$((failures+1))
continuewould suffice. That prints Maybe we're lucky and that's enough. |
Sort of... 😅 But I changed the action script mostly, not the reviewer script, most of it in packagecontrol/st-package-reviewer-action@90d487d
The title isn't super relevant. In various contexts it's not even used and overwritten with something else by GitHub. So the message is the important bit. I went for very short PASS/FAIL/WARN like titles.
On the action tab in a PR you'll see callouts for the number of messages: 
If you look at an individual action you'll see: 
In other words, you don't need to dive into the job output to find information in all the logs, it picks out the lines that have the right format and presents them separately. On the PR conversation tab you don't see anything (assuming the jobs pass) sadly. So warnings are mostly just there for those who dig a bit deeper, and they're there for the reviewer (ie. me). But it does also make errors easier to find. |
|
Ok. These annotations look nice. I added log groups and annotations logging. |
|
I'm having some issues running this locally: ~/c/st_package_reviewer ((2e538bc8)|✚1) [127]$ ./gh_action/action.sh --pr https://github.com/wbond/package_control_channel/pull/9239
::group::Fetching PR metadata
Resolving PR metadata via gh: https://github.com/wbond/package_control_channel/pull/9239
Base URL: https://raw.githubusercontent.com/wbond/package_control_channel/2e840eafa99c823f533a32d228693f1570bdde96/repository.json
Target URL: https://raw.githubusercontent.com/wbond/package_control_channel/8c3a9430c0254ef62221dc173d8fb70de3fbb229/repository.json
::endgroup::
::group::Getting thecrawl
Using thecrawl at: /Users/koenlageveen/code/st_package_reviewer/.thecrawl
::endgroup::
::group::Generating base registry…
Fetching registered packages...
Found 4671 packages and 91 dependencies in 1 repositories.
Prepared packages in 0.31 seconds.
Saved registry as /var/folders/_g/g4s658wd54v2g9grfrfwcydw0000gn/T/tmp.LvmXKGAuab/base_registry.json
::endgroup::
::group::Generating target registry…
Fetching registered packages...
Found 4672 packages and 91 dependencies in 1 repositories.
Prepared packages in 0.46 seconds.
Saved registry as /var/folders/_g/g4s658wd54v2g9grfrfwcydw0000gn/T/tmp.LvmXKGAuab/head_registry.json
::endgroup::
./gh_action/action.sh: line 181: mapfile: command not found
Traceback (most recent call last):
File "/Users/koenlageveen/code/st_package_reviewer/gh_action/diff_repository.py", line 145, in <module>
sys.exit(main())
File "/Users/koenlageveen/code/st_package_reviewer/gh_action/diff_repository.py", line 29, in main
with open(args.base_file, 'r', encoding='utf-8') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/var/folders/_g/g4s658wd54v2g9grfrfwcydw0000gn/T/tmp.LvmXKGAuab/base_registry.json' |
Easiest would be if you update your Bash which you maybe never did. (T.i. you would run stock bash.) Otherwise I need to implement a loop here. |
|
I think I'm just at stock MacOS bash (GNU bash, version 3.2.57(1)-release (arm64-apple-darwin25)). I can upgrade via homebrew. |
|
I refactored to not use mapfile which I think is the only things requiring Bash4. How can you a justify a Python rewrite? |
|
Looks like that worked indeed. I was worried for a second... bash can at times be annoyingly less portable than one hopes for. I'm really out of bash scripting, haven't done anything serious in it for like 7 years+ and am not going to pretend I understand halve of what goes on in action.sh 😅 For the same reason, I can test this in practice, but I'm not in a position (knowledge or experience wise) to review the PR. But this is great 👍🏻 It's a clean interface for the action and output on the CLI looks good. I did have to set the executable bit on action.sh. Maybe you want to do that and check that in? So, what do you want to do? I can start using this for PRs on the main package_control_channel and get some real world usage feedback. I can do that right now based on HEAD of your fork. Or should I wait for this PR to get reviewed? |
|
I thought you just use it. (This is actually re-usable I think. So it could be used in SL or LSP channel repositories.) Likely we need output tuning for the annotations etc. No-one reviews such plumbing scripts tbh. It is just 300 lines anyway (but I made a patch in thecrawl for example to make it work. One needs that knowledge for the inter-op.) |
|
If we also require curl and unzip as deps we could remove some lines. |
|
Using curl and unzip are fine by me. I imagine they're available on ubuntu-latest as well. |
|
Alright, finally got a test setup. This PR adds the new reviewer action next to the existing one. I've then replicated the change from wbond/package_control_channel#9235 on top of that so we can compare the output. I'm running into an error I don't understand though, see https://github.com/wbond/package_control_channel/actions/runs/19407178504/job/55523567264?pr=9246. error: Failed to spawn: `st_package_reviewer`
Caused by: No such file or directory (os error 2)A quick Google says "failed to spawn" is an error from |
|
Try again with baec27a. |
|
Got a step further. Looks like the virtual environment isn't what it should be? https://github.com/wbond/package_control_channel/actions/runs/19411270967/job/55532991089?pr=9246 
... |
|
The Github action environment isn't what I thought it to be. But also impossible to have that in your head tbh. Just try again. 780217d |
|
Got it, works 🎉 https://github.com/wbond/package_control_channel/actions/runs/19437333329/job/55611547454 We're however missing annotations for the actual failures. It reports "Completed with 1 failure(s)" whereas the old one reports " |
|
Hm. It is reporting "Reporting 1 failures:", I was expecting "Reporting 1 errors:". I think you can reference the branch actually instead of the exact commit hash. |
Sure, but bumping the hash gives a bit more control. Do you think it's possible to report the individual errors? Since I did that on the old implementation I can also give it a go if you want? |
|
You say the awk script collates all failures into one message? Maybe you can patch the script to your liking. I neither know what the reviewer actually emits nor what you had before tbh. Ideally we collate the reviewers output and post it as PR comment. Likely wbond doesn't have the secrets stored here in the repo but at home on his laptop. 🤔 You can also post example output from the reviewer and how that should be formatted/transformed. 🤷♀️ |
He doesn't even have a laptop though. I'm going to dig into the script, see what's what. Ideally it can do both a collated comment, and streaming annotations. Maybe the thing to do is let the awk script do it's thing, but add a "verbose" option to the reviewer itself that let's it print anything it finds immediately. |
LOL, a modern form of, we all stand naked before God. |
|
Just a heads-up, since I've been looking at some GitHub notifications today: I won't merge this. Not because I don't like your changes per se but because I believe this to belong into https://github.com/packagecontrol/st-package-reviewer-action. The package reviewer itself (this repo) is intended to be a standalone Python application, much like a linter that can lint ST packages. It should produce readable output for both humans and machines (the latter is not exactly considered currently because of the way output is structured) and can then be used by developers locally, in CI for an individual package, or for the more complex operation that is reviewing Package Control Channel additions. However, the logic of the latter should not bleed into this repo. Edit: Forgot to mention that I didn't look at the changes in detail and also probably won't for the next couple weeks. |
kaste
force-pushed
the
gh-action
branch
3 times, most recently
from
caf22ef to
ee0516e
Compare
Implements a Github action. Refer gh_action/README.md for its usage. Ref packagecontrol/thecrawl#66 Ref packagecontrol/thecrawl#166
`mapfile` doesn't give us much here as we still need to strip the trailing '\r' and filter empty lines. Invent a (kind of unreadable) helper to make it on the user side easier.
|
FYI, I'm marking this as a draft to indicate that it is not ready for merge (and will never be), but feel free to continue pushing commits here and to mention me for questions. |
|
To the readers here: The PR here is self-contained and adds a new directory to this The actual maintainers can pull the changes. I don't think splitting this into two repositories can be justified as it just adds a cross- |
4 participants
Implements a Github action. Refer gh_action/README.md for its usage. But in a nutshell you just do
Ref packagecontrol/thecrawl#66
Ref packagecontrol/thecrawl#166