Skip to content
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

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

risicle
Copy link
Contributor

@risicle risicle commented May 8, 2022

Description of changes

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. 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
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Mic92
Copy link
Member

Mic92 commented May 9, 2022

I like the idea. This could even become it's own re-usable github action eventually.

@risicle
Copy link
Contributor Author

risicle commented May 10, 2022

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.

@risicle
Copy link
Contributor Author

risicle commented Jun 19, 2022

Now that we're maintaining two simultaneous release branches I realize I probably need to add release-* (and staging-*?) to PICKABLE_BRANCHES. See #178230 for a case in point.

@risicle
Copy link
Contributor Author

risicle commented Jul 5, 2022

Updated with PICKABLE_BRANCHES globbing support, and accepting release-??.?? and staging-??.?? branches.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 7, 2023
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 1, 2024
@risicle
Copy link
Contributor Author

risicle commented Apr 1, 2024

Updated to fix an iteration bug (subshells 🙄) and use checkout v4 with filter argument allowing me to do blob:none and make it a lot faster.

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.
Copy link
Contributor

@LeSuisse LeSuisse left a 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.

@LeSuisse LeSuisse merged commit 413f064 into NixOS:master Apr 8, 2024
18 checks passed
@LeSuisse
Copy link
Contributor

LeSuisse commented Apr 8, 2024

[...] it looks like this should not cause too much false positives.

Merged, hopefully I will not regret this sentence 😅

@risicle
Copy link
Contributor Author

risicle commented Apr 8, 2024

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...

Copy link
Contributor

github-actions bot commented Apr 8, 2024

Successfully created backport PR for release-23.11:

@misuzu
Copy link
Contributor

misuzu commented Apr 10, 2024

Looks like it's broken (#303184):

Commit 
  fatal: ambiguous argument '': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'
  Error: Process completed with exit code 12[8](https://github.com/NixOS/nixpkgs/actions/runs/8636643882/job/23677013710#step:3:9).

@LeSuisse
Copy link
Contributor

See #303192 for that issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants