-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
add "check cherry-picks" github action #172098
Conversation
I like the idea. This could even become it's own re-usable github action eventually. |
774ef5d
to
b33f23b
Compare
Updated with a version that's just a thin wrapper around a separate shell script now. Example run https://github.com/risicle/nixpkgs/runs/6377944756?check_suite_focus=true Not the most beautiful or friendly output when run as a shell script but people are welcome to refine it in the future. Output was mostly designed around the limitations of github actions. |
b33f23b
to
b2fe01a
Compare
Now that we're maintaining two simultaneous release branches I realize I probably need to add |
b2fe01a
to
ba52514
Compare
Updated with |
ba52514
to
7844062
Compare
7844062
to
b0ae355
Compare
Updated to fix an iteration bug (subshells 🙄) and use checkout v4 with |
the intention being to catch commits which declare themselves as cherry-picks, but either: - don't refer to a commit in the master or staging branches - are significantly altered from their original commit determining the latter is not an exact science, but the heuristic of looking for differences in only the added or removed lines seems to work quite well. still, this should be considered an assistant for reviewers rather than a hard failure. unfortunately github workflows don't have a way of raising a gentle warning instead of a failure. the formatting of the output also leaves something to be desired due to the limitations of github actions' "group" commands.
34cf200
to
fbad66d
Compare
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 code looks fine to me and I like the idea.
I checked the PRs recently merged into release-23.11
and outside of the security related work and the occasional revert it looks like this should not cause too much false positives.
Merged, hopefully I will not regret this sentence 😅 |
There's always a revert button. But I'm not sure it will have an effect until I backport it to the stable branch, so... |
Successfully created backport PR for |
Looks like it's broken (#303184):
|
See #303192 for that issue |
Description of changes
The intention being to catch commits which declare themselves as cherry-picks, but either:
Determining the latter is not an exact science, but the heuristic of looking for differences in only the added or removed lines seems to work quite well. Still, this should be considered an assistant for reviewers rather than a hard failure. Unfortunately github workflows don't have a way of raising a gentle warning instead of a failure. My real interest here is catching trojan horse commits, as I realized I'm a lot less thorough reviewing commits that are cherry-picks, and it would probably be a lot easier to slip something past a reviewer by claiming it to be a cherry-pick of something innocent.
An example run of this workflow is visible here https://github.com/risicle/nixpkgs/runs/6342603972?check_suite_focus=true where I ran it against a release-21.11 branch which I had made random cherry-picks (and phoney cherry-picks) to give it a bit of a torture test.
You'll see that the formatting of the output also leaves something to be desired due to the limitations of github actions' "group" commands. Ideally we'd be able to choose which groups (i.e. the ones containing problems) are initially expanded, but that's not possible. So the error output is printed after a commit's "group". Ugly, I know, but the alternative leaves the reader to go digging for the issues in all the collapsed groups.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes