The Contributor's Survey results highlight the fact that
it is often not easy for contributors to know who the right people are
to talk to about a proposed change or who to ask for reviews of a given
subsystem. We briefly toyed around with codeowners before when GitHub
introduced it but just as quickly disabled it because the structure of
our repository made it exceedingly difficult to get right.

My recollection was we disabled it because it didn't work the way it was intended to with the way we'd set up teams/permissions for the repository: #21161

Yep, and it's possible that it still doesn't but we should at least try it. If it still doesn't work the way we need it to, then the bot needs to be updated to ensure that the right people are automatically pulled in to review.... it's also possible that a github action can be set up to achieve the behavior we need here. This is all why this is still just a draft PR.

I checked GitHub docs, and CODEOWNERS will not work for the same reason it didn't work last time: The owners require write access in the repo. From https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners:

When the code owner is a team, that team must have write permissions, even if all the individual members of the team already have write permissions directly, through organization membership, or through another team membership.

So every entity listed in CODEOWNERS will need to have write access to the repo granted directly to it. So adding teams will not work. Adding individuals will not work. The only team that will work is @nodejs/collaborators. We either need to change the way we have the permissions in the repository set up (please, no) or do something different.

I suppose we could add teams with write access to the repo and then restrict the teams to collaborators. But that will be a lot of work when offboarding people, generally error-prone, and I think basically not a good idea, as we often benefit from non-collaborators being members of teams like @nodejs/http.

Dang... ok, then let's explore a github actions based approach here... essentially, an action that applies the basic codeowner rules to teams without the write requirement.

The approach taken by @amphtml may be of some interest here. It's an OWNERS file rather than CODEOWNERS. The @amp-owners-bot pings the appropriate people when you open a PR.

Check it out. :)

Relevant: https://twitter.com/cramforce/status/1182349710121734145

it's also possible that a github action can be set up to achieve the behavior we need here.

GitHub Actions can't comment or add reviewers on PRs from forks as Actions running on those PRs lack write-permission, so it wouldn't work either. Our bot should be able to do it though.

If we can specify specific files we can probably also add the n-api in the initial set of areas.

I think this is a good motivation to rethink our team structure and analyze who is part of said teams and why they are not collaborators yet.

I think we should make those teams subteams of @nodejs/collaborators and onboard (or remove) people that do not need that level of permission.

I think we should give a shot to codeowners. This will also highlight areas where we are lacking contributors (and we can also point people to work on those part of core).

Good point @mcollina. In fact, we can start rolling this out incrementally as I originally suggested with teams that we know do have write access. The teams can definitely be cleaned up a bit!

analyze who is part of said teams and why they are not collaborators yet

Team like @nodejs/modules and @nodejs/diagnostics are Working Group teams. On both teams there are folks who are actively engaged but don't have write access to nodejs/node. Maybe for those cases we could create @nodejs/[teamname]-reviewers, and use the reviewers team for codeowners. This might even be automatable with GitHub Actions (edit: not as well as I was hoping, there's no trigger when a team is changed). But I agree with @jasnell, we can start with the teams which work and incrementally add teams later.

I would really need to have something in place for streams.

To move forward on this, let's initially limit the subsystem scope and see where we get and what issues may come up. The quic subsystem is new, the team is currently limited to folks who have write access, and it's well contained. Let's limit the CODEOWNERS to it and give it a test run. If things work out, we can expand out incrementally from there.

@nodejs/tsc ... please take a look. This re-introduces a limited CODEOWNERS as an experiment to see if we can use CODEOWNERS more extensively and what the issues may be. It is currently limited in scope to cover the new experimental quic subsystem, so it shouldn't interfere with anything else.

I don’t think the quic team currently has write access here? Is the current idea to verify whether write access is no longer required?

Maybe for those cases we could create @nodejs/[teamname]-reviewers, and use the reviewers team for codeowners.

I like that suggestion. 👍

I don’t think the quic team currently has write access here? Is the current idea to verify whether write access is no longer required?

The team, no, but everyone currently on it should, and enabling it for the team should work.

the challenge with using separate *-reviewer teams is that I think we're already just about at our allowed limit on the number of teams we're allowed to create in this github org (I could be wrong, but I thought the upper limit was about 200)

Oh, I didn't know there was a limit. Maybe we should try to contact GitHub and ask if they can bump it?

As for giving write permission to quic: I'd be wary of giving write permission to teams without those teams being a subteam of collaborators. I believe we don't want to fragment permissions too much. But adding the permission temporarily to test should be fine.

Oh, I didn't know there was a limit. Maybe we should try to contact GitHub and ask if they can bump it?

I could be wrong. We need to find out. It would also be good to just audit our existing teams and see if there are any that can be removed

As for giving write permission to quic: I'd be wary of giving write permission to teams without those teams being a subteam of collaborators. I believe we don't want to fragment permissions too much. But adding the permission temporarily to test should be fine.

All the current members of the quic team are part of collaborators already, so we should be fine here.

I've made the @nodejs/quic team a sub-team of nodejs/Collaborators and have confirmed that the team has write access and everyone in it currently in a collaborator.

@nodejs/tsc ... I'm proposing that we land this as an experiment with the relaxed time requirements proposed by @addaleax in #33879 scoped specifically to the QUIC work for now. Let's run the experiment for a couple of weeks to see how things go, and assuming all goes well we can expand it to other subsystems incrementally before rolling the process changes out to the full repository.

The idea here is to create new -reviewer teams and give them write access so that CODEOWNERS can ping them? Not blocking, but I'm not excited that it increases the overhead of maintaining teams. We're already not-great at it. This is going to require updates to our onboarding and offboarding docs, and probably has other consequences. It may render the current teams obsolete, thus excluding people who aren't already collaborators.

Again, not blocking, but want to get a full accounting of probable downsides out here, in case there's another way....

@Trott ideally if we decide to go the -reviewer way, I'd like to see it automated to keep these teams up to date with everyone from respective teams who are also collaborators. I don't think going with a manual approach would work well for us.

Has anyone tried to contact GitHub to ask about the possibility of codeowners working for teams who don't have write permission?

@Trott ideally if we decide to go the -reviewer way, I'd like to see it automated to keep these teams up to date with everyone from respective teams who are also collaborators. I don't think going with a manual approach would work well for us.

Automating these teams would be great.

Has anyone tried to contact GitHub to ask about the possibility of codeowners working for teams who don't have write permission?

Yes, they have definitely heard that feedback. I know I gave it to them some time ago and I know I'm not the first one.

Speaking of supplementing CODEOWNERS with bots: https://bionic.fullstory.com/taming-github-codeowners-with-bots/

Can't seem to locate the repo I used to put GitHub feature requests in, but I did find this: https://github.community/t/feature-request-codeowners-without-write-permissions/2323

Oh yes, that's definitely a good alternative: we can have github-bot apply the reviewers instead of GitHub.

I hope one day Actions will be able to do these cool things (without relying on scheduler shenanigans).

Landed in fdf10ad

Let's continue the discussion on how to proceed with other teams in an issue, since that's not a blocker for this PR

Continue discussion on: #33984