-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Alternate more flexible code owners mechanism, soon to avoid mass pings #336261
Conversation
cc @tie for bash wizardry and excellent reviews as well. |
Bash code is high quality, although I’d avoid $ 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 #!nix-shell -i python3 -p python3Packages.pygithub |
I would definitely use something else than Bash too. |
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! |
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. |
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:
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. |
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 |
f8cbd6c
to
c058d14
Compare
812be74
to
c47a50d
Compare
85284f0
to
2a9bfbb
Compare
Worked on this a bunch, and a bit more is needed, but it's looking pretty good! I tested:
|
0cc2893
to
efeb298
Compare
So is there still going to be still |
@Eveeifyeve Good question. |
Yeah but I guess it makes sense. |
aaf1d6b
to
3964fb5
Compare
Since NixOS#336261 we have CI that checks that the codeowners file is valid: https://github.com/NixOS/nixpkgs/actions/runs/11243668280/job/31260095472#step:7:34 Which files are correct (or whether they were removed) was determined using the Git history and some grepping
Alright merged and enabled again. Here's the place to watch how it's doing: https://github.com/NixOS/nixpkgs/actions/workflows/codeowners.yml |
After #340162 and #336261 it started failing: https://github.com/NixOS/nixpkgs/actions/runs/11246996195/job/31269748379
Since NixOS#336261 we have CI that checks that the codeowners file is valid: https://github.com/NixOS/nixpkgs/actions/runs/11243668280/job/31260095472#step:7:34 Which files are correct (or whether they were removed) was determined using the Git history and some grepping
Since NixOS#336261 we have CI that checks that the codeowners file is valid: https://github.com/NixOS/nixpkgs/actions/runs/11243668280/job/31260095472#step:7:34 Which files are correct (or whether they were removed) was determined using the Git history and some grepping
Another follow-up improvement: #347592 |
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 |
This effectively disables the native GitHub codeowners feature and enables the new alternate codeowners mechanism introduced in NixOS#336261 This means that: - We can now declare users without write access as code owners! - Targeting the wrong branch won't trigger mass pings anymore!
#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 |
This effectively disables the native GitHub codeowners feature and enables the new alternate codeowners mechanism introduced in NixOS#336261 This means that: - We can now declare users without write access as code owners! - Targeting the wrong branch won't trigger mass pings anymore!
Since NixOS#336261 we have CI that checks that the codeowners file is valid: https://github.com/NixOS/nixpkgs/actions/runs/11243668280/job/31260095472#step:7:34 Which files are correct (or whether they were removed) was determined using the Git history and some grepping
# https://github.com/mszostok/codeowners-validator/pull/222 | ||
(fetchpatch { | ||
name = "user-write-access-check"; | ||
url = "https://github.com/mszostok/codeowners-validator/compare/f3651e3810802a37bd965e6a9a7210728179d076...840eeb88b4da92bda3e13c838f67f6540b9e8529.patch"; |
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.
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.
We don't have any tests for this other than running it in dry-run mode? |
This effectively disables the native GitHub codeowners feature and enables the new alternate codeowners mechanism introduced in NixOS#336261 This means that: - We can now declare users without write access as code owners! - Targeting the wrong branch won't trigger mass pings anymore!
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:
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
This work is sponsored by Antithesis ✨
Add a 👍 reaction to pull requests you find important.