Node: CODEOWNERS next steps

[mmarchini]

Following discussions from: https://github.com/nodejs/node/pull/33895

The CODEOWNERS feature on GitHub can be useful to ensure the appropriate folks are made aware of relevant PRs for them. Unfortunately, the feature only works for teams with write access to the repository, which means teams line @nodejs/diagnostics, @nodejs/modules, @nodejs/build, @nodejs/security, etc. which are knowledgeable about or responsible for some files/subsystems cannot be pinged by this feature.

The PR above introduces CODEOWNERS for quic as an experimentation. While the quic team is testing this feature, we should also discuss how to proceed to include other teams without generating unnecessary organizational burden to the project. A few ideas came up in the PR:

1. Have a GitHub Actions set the reviewers.
   a. Due to how Actions work, it would either need to be a scheduled action or a request_dispatch action which is triggered from a remove caller (like github-bot)
2. Restructure our teams so that relevant teams are under @nodejs/collaborators
   a. This might lead to increase organizational burden, as it would be easy to add someone to one of the subteams even thought that person is not a collaborator yet
3. Introduce -reviewers team for teams which are not under @nodejs/collaborators but should be pinged
   a. -reviewers teams would include only folks who are already collaborators
   b. Ideally these groups should be created and updated via some automation tool
4. Implement similar behavior to CODEOWNERS on github-bot
   a. This means we'll need to implement our own thing, which could lead to increased maintenance burden
5. Wait for GitHub to implement this feature for non-committers teams
   a. This might never happen though

The list above includes all options raised on the PR (with technical details and any downsides of each option). There are probably other options we can explore, but this should be a good starting point for conversation.

[mmarchini]

I like the idea of implementing this with github-bot. It already reacts to new PRs and creates labels for it, so part of the implementation is already there. It's also worth noting that AMP is using this approach too.

If we implement this as a comment instead of "request review" from teams, I'd say 85% of what we need is already implemented on github-bot, since it also knows how to comment on issues. The implementation would consist of fetching the CODEOWNERS file, checking which files were changed, match with CODEOWNERS content, and send a comment to the PR. Alternatively, we could decide which teams to ping based on which labels the bot will add.

[Trott]

The PR above introduces CODEOWNERS for quic as an experimentation.

It will not work for the quic team because the quic team does not have write access to the repository. It does not matter that every member of the quic team has write access to the repository. The team itself does not have write access assigned in the repository so notifications/assignments/whatever will not happen for that team on the basis of CODEOWNERS.

[mmarchini]

It will not work for the quic team because the quic team does not have write access to the repository.

@jasnell moved the team to be under @nodejs/collaborators, so it should work since the permission is inherited (or is it?). As a starting point I think it's fine, I'm not convinced it's scalable to the entire org though, which is why I'm in favor of a github-bot approach.

Another advantage of the github-bot approach is that folks with code or domain knowledge but without commit access (I know it's rare but we have a few cases) can also get pinged.

[Trott]

Alternatively, we could decide which teams to ping based on which labels the bot will add.

Using the bot to do this would be an awesome first step, and might even be all we need. No need to consult CODEOWNERS. Just associate the labels it's already adding with the right team and we've got most (or maybe even all) of what we need, I think.

[Trott]

@jasnell moved the team to be under @nodejs/collaborators, so it should work since the permission is inherited (or is it?).

Much to my surprise, it should indeed work, at least based on https://github.community/t/codeowners-doesnt-work-with-subteams/1751. 🤯

[Trott]

@jasnell moved the team to be under @nodejs/collaborators, so it should work since the permission is inherited (or is it?).

Much to my surprise, it should indeed work, at least based on https://github.community/t/codeowners-doesnt-work-with-subteams/1751. 🤯

While OK for testing, I assume this is off the table as a sustainable way to do this, unless we're going to rename every group along the lines of collaborators-quic or quic-reviewers. If we have 50 subteams, sooner or later, someone will mistakenly add a non-Collaborator to one or more of them and no one will notice. At least if they're all named the same way, it's easier to know which team is for collaborators only.

[mmarchini]

No need to consult CODEOWNERS. Just associate the labels it's already adding with the right team and we've got most (or maybe even all) of what we need, I think.

That should work, but maybe we should move the labels and teams definitions to here instead of having those hardcoded in the github-bot repository. IMO it makes more sense for any collaborator or at least the TSC to approve and merge changes to labels and team notifications instead of leaving that responsibility to the already small team responsible for the bot.

[mmarchini]

https://github.com/nodejs/github-bot/pull/265 implements an alternative to GitHub built-in code owners. It uses the same format and looks for the same file, so no changes on this repository will be needed.