Alternate more flexible code owners mechanism, soon to avoid mass pings

[infinisil]

Description of changes

These damn mass pings are annoying, let's fix it!

This PR introduces an alternate mechanism of doing effectively the same as GitHub's native CODEOWNERS feature, but with some significant advantages:

• No reviews will be requested for PRs that target the wrong base branch.
• There is no need for user/team to have write access to be requested for reviews.
• Invalidity of the code owners file fails CI

This PR still runs the native CODEOWNERS together with the alternative mechanism, so that we can run it for some time to confirm that it works correctly. Once confirmed, we'll be able to easily turn off the native CODEOWNERS and just rely on the alternate mechanism.

Note that this PR depended on NixOS/org#31

Tested

• Codeowner pings can't be avoided by changing the code owners in the same PR: Ping avoid attempt Infinisil-s-Test-Organization/nixpkgs#13
• Can't set non-org members as code owners: Non-member code owner fails Infinisil-s-Test-Organization/nixpkgs#14
• Reviews can be requested from people without write access: Users without write access can be requested Infinisil-s-Test-Organization/nixpkgs#15
• Files without owners cause no problems: Non-owned files don't cause problems Infinisil-s-Test-Organization/nixpkgs#16
• Staging merges cause no problems: Staging merge Infinisil-s-Test-Organization/nixpkgs#18
• Pings get prevented and a comment is posted when the base branch is wrong: Wrong base Infinisil-s-Test-Organization/nixpkgs#21
• Can't add code owners for non-existent files: Owner of non-existent file Infinisil-s-Test-Organization/nixpkgs#20
• Drafts don't trigger pings: Draft doesn't trigger reviews Infinisil-s-Test-Organization/nixpkgs#22 (and the CI logs show who would be pinged)
• Undrafting triggers pings: Draft then undraft Infinisil-s-Test-Organization/nixpkgs#23

---

This work is sponsored by Antithesis ✨

Add a 👍 reaction to pull requests you find important.

[philiptaron]

cc @tie for bash wizardry and excellent reviews as well.

[tie]

Bash code is high quality, although I’d avoid cmd1 < <(cmd2) because it does not respect errexit (-e flag). E.g.

$ bash -e -u -o pipefail -O inherit_errexit -c 'true < <(false); echo oops'; echo exit code $?
oops
exit code 0
$ bash -e -u -o pipefail -O inherit_errexit -c 'cat <(bashgobrrr); echo oops'; echo exit code $?
bash: line 1: bashgobrrr: command not found
oops
exit code 0

I don’t think I’ll have time to pick this up, but I’d probably rewrite scripts in Python with GitHub client libraries to avoid Bash. It’s much nicer to work with if available, even if subprocess.run calls are a bit more bulky for scripting 😅

#!nix-shell -i python3 -p python3Packages.pygithub

[drupol]

I would definitely use something else than Bash too.

[infinisil]

If I have time to work on this again, I'd prefer to keep using bash, since the script part is pretty much done already. But if somebody else picks this up before that, I fully support a Python rewrite!

[SuperSandro2000]

This would only cover the use case to request reviews for changed files. We would loose the UI indicator that files are owned by a codeowners, the feature to block merges unless a codeowber approved (we're are not using that to much) and the validation in the UI.

We could split the codeowners file to those features for some entries back though.

[Mic92]

This would only cover the use case to request reviews for changed files. We would loose the UI indicator that files are owned by a codeowners, the feature to block merges unless a codeowber approved (we're are not using that to much) and the validation in the UI.

We could split the codeowners file to those features for some entries back though.

For me that sounds like a good tradeoff if it reduces the noise I am currently getting as a code owner.

[infinisil]

Good point @SuperSandro2000. I agree with @Mic92 that it's probably a good tradeoff still, but in the future we can also fully or partially implement those features:

• UI indicators: Because we're not limited by GitHub anymore, we can make owners declarations very local to files/dirs, e.g. an owner: @foo at the top of files, or a CODEOWNERS file in local directories
• Blocking merges: Can be done by a GitHub action
• UI validation: Can be done with a GitHub action too, I actually set this up for another repo before: https://github.com/NixOS/org/blob/da5ea7ed87d326568db245657ad8e00b8525a816/.github/workflows/ci.yml#L24-L56

Ideally all of this would be implemented by some GitHub action out there already, but I couldn't find anything that would work for our specific case.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-reaching-the-right-reviewers/51312/4

[infinisil]

Worked on this a bunch, and a bit more is needed, but it's looking pretty good! I tested:

• That it correctly requests reviews: Update default.nix Infinisil-s-Test-Organization/nixpkgs#3
• (Big!) Support for setting people without write access as a code owner! Update default.nix Infinisil-s-Test-Organization/nixpkgs#5
• Test that it doesn't request reviews when the base branch is wrong: Infinisil patch 1 Infinisil-s-Test-Organization/nixpkgs#6

[Eveeifyeve]

So is there still going to be still lib.maintainers or is nixpkgs switching to owners?

[infinisil]

@Eveeifyeve Good question. meta.maintainers does have advantages (such as being able to process it more easily, e.g. for detecting unmaintained packages at eval-time), but also disadvantages (it doesn't map cleanly to files). I can imagine a future in which we combine the best of both worlds, but for now we should definitely maintain the status quo.

[Eveeifyeve]

@Eveeifyeve Good question. meta.maintainers does have advantages (such as being able to process it more easily, e.g. for detecting unmaintained packages at eval-time), but also disadvantages (it doesn't map cleanly to files). I can imagine a future in which we combine the best of both worlds, but for now we should definitely maintain the status quo.

Yeah but I guess it makes sense.

[infinisil]

I briefly disabled it manually because apparently the current codeowners file references some non-existent files: https://github.com/NixOS/nixpkgs/actions/runs/11243668280/job/31260095472#step:7:34

Making a PR to fix this

[infinisil]

Kind of dangerous though, that we can disable the workflow to prevent pings (well not yet), we'll have to trust the committers with that..

[infinisil]

#347348

[infinisil]

Alright merged and enabled again. Here's the place to watch how it's doing: https://github.com/NixOS/nixpkgs/actions/workflows/codeowners.yml

[infinisil]

Another follow-up improvement: #347592

[infinisil]

We also did have a mass ping earlier with #346556, but because the base branch got adjusted within 10 seconds, CI didn't get to the nice error message: https://github.com/NixOS/nixpkgs/actions/runs/11261663743/job/31315595008. (Edit: I wrote a change to fix this, but I don't think we actually want that in practice)

The codeowner check job also failed because the /merge ref didn't exist anymore: https://github.com/NixOS/nixpkgs/actions/runs/11261666327/job/31315601456#step:6:59. I'm looking into whether it's possible to not fail like that. Edit: I don't think it's easily doable but also not really necessary.

[infinisil]

#347610 🚀

We could also wait longer, but honestly I don't think we need to. We can always revert if it causes problems, but I'm fairly convinced there won't be any, and I'll be available in case there are

[SuperSandro2000]

Isn't it kinda a bit up to luck if the commits get garbage collected in unmerged PRs? When you receive review feedback and force push things, the URL might break over time.

[SuperSandro2000]

We don't have any tests for this other than running it in dry-run mode?
That is kinda concerning for security related tool that talks to outside APIs and does many things in shellscript.