Get rid of GitHub's CODEOWNERS feature (and replace it)

[piegamesde]

Because it is totally, utterly broken.

• People are not getting pinged although they should Some CODEOWNDERS entries are broken #124085
• It's easy to accidentally ping a lot of people with a bad PR Tor 21.05 #143425
  • Things like this easily happen when switching the target branch of a PR from master to staging
  • According to @maralorn, wrong pings like these make up a significant proportion of their email inbox

Taken together, the feature very probably causes more harm than good. We should make it a priority to disable it, and maybe find an alternative that is less broken. My suggestion would be to add it as an OfBorg feature: NixOS/ofborg#567

[AndersonTorres]

Maybe this should be pinged at Discourse?

[piegamesde]

Yeah, probably. I don't have an account there, but feel free.

[nixos-discourse]

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

https://discourse.nixos.org/t/get-rid-of-the-codeowners-feature/15741/1

[maralorn]

I have been summoned, so I shall speak.

• I think doing this would not solve anything. Yes, some codeowner entries are broken. We actually debugged this, when we created the new Haskell maintainers team and resolved to list our names manually. But this just means that some people are using it wrong, not that feature is useless itself.
• Yes, a lot people get a lot of pings. Yes, this is super annoying and slightly embarrassing for the person triggering it. Yes, these pings make a significant portion of my mailbox. However I also get very useful notifications over the code owners feature. This feature is central to my workflow. The trade-off is worth it. With a well configured e-mail client I can deal with the spammy notifications within seconds. And this is a trade off everyone can do personally. If you don‘t like the feature, just remove yourself from the CODEOWNERS file. That’s it problem solved. No need to take this personal choice away from anyone.

Of course a solution where I only get the relevant pings and not the irrelevant ones would be even better. But that sounds like a non-trivial problem.

I am against removing the feature without an alternative.

[jonringer]

I second @maralorn , even though I get spam because the python package set is massive. It still gives me a very steady backlog of work I could do.

[r-burns]

• Things like this easily happen when switching the target branch of a PR from master to staging

I think this is the most common cause, but it's possible to avoid if you are aware of it and know how to. If we document this properly in the contributors guide, and link to it when asking contributors to switch the target branch, we can probably avoid the vast majority of reviewer spam.

[piegamesde]

But this just means that some people are using it wrong, not that feature is useless itself.

I think "using it wrong" does not describe it well since for users without write access, there is no way to use it right. This applies to teams as well:

The other teams are actually no op as well. Only the security team has write access. [source]

So yes, the feature is useless for all teams except the security team.

---

Generally, I agree that a CODEOWNERS feature is worth-while having, and if those most affected by the spam would like to keep the current solution until a better alternative is set in place, then that's fine for me. By the way, prototyping an alternative implementation is done here.

[maralorn]

So yes, the feature is useless for all teams except the security team.

No. Writing teams into the codeowners file maybe useless. Any team can write their members into the file. That works.

But yeah a replacement feature which works better with teams would reduce redundancy.

[domenkozar]

If we have a better alternative, let's use it.

Until we do, dropping use of the codeowners with all its imperfections would be a step backwards.

[SuperSandro2000]

Wasn't the goal to not add more things to ofborg?

Also could we implement team code owners using GitHub actions?

[piegamesde]

Wasn't the goal to not add more things to ofborg?

I don't know of anything. OfBorg already handles review requests for package maintainers, so it makes sense to add this as a feature. But the actual matching code is pretty standalone, so it can be integrated either way. My prototypes will probably initially run on my account for testing.

[SuperSandro2000]

IIRC the code that handles labels for PRs was recently moved from ofborg to a github action to make updating it easier and remove the requirement to redeploy ofborg. If I remember it correct that was the general direction to not overload ofborg with new, standalone features if possible.

[piegamesde]

I'm increasingly in favor of using GitHub actions as well, simply because nobody wants to touch OfBorg and I'd prefer any solution over no solution.

I've stumbled over https://github.com/sourcegraph/codenotify which also has a good blog post that describes their thoughts on the issue (spoiler: they had many problems we have too). We might even use their code with modifications (for example, we prefer review requests rather than comment pings). But I'd be open to continuing work on our custom implementation as well.

I really liked their distributed approach across the file tree. It really makes sense to localize these, instead of keeping them in one huge file. I'm also increasingly in favor of moving the maintainership entirely out of the nix files, since it has caused much trouble w.r.t tooling support (pings for NixOS modules and tests are still broken if they ever worked).

One of our issues with current CODEOWNERS is the silent failure on wrong configuration. Independently of our other actions, this can be solved by having a dedicated CI action that does the checks that GitHub should be doing.

[SuperSandro2000]

Since recently GitHub shows you exactly which entry is wrong https://github.com/NixOS/nixpkgs/blob/master/.github/CODEOWNERS

[FliegendeWurst]

This was done in #347610 and related PRs. Problems with the new mechanism should go in new issues.