meta: introduce codeowners again #33895
Conversation
nodejs-github-bot
added
the
meta
jasnell
force-pushed
the
codeowners-take-2
branch
from
86c8fa8 to
65013eb
Compare
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:
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 |
|
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 |
|
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. Relevant: https://twitter.com/cramforce/status/1182349710121734145 |
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 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! |
Team like |
|
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. |
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team
jasnell
force-pushed
the
codeowners-take-2
branch
from
c2137e0 to
a2a2d55
Compare
|
@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 |
There was a problem hiding this comment.
Will it work? I thought the team had to be a subteam of @nodejs/collaborators to get pinged (but maybe I misunderstood). This also adds @nodejs/tsc as codeowners of codeowners, which I agree, but I wonder if it will face the same issue.
Regardless, LGTM. If it doesn't work, we can tweak codeowners and teams until we figure out something that works for us.
|
I don’t think the
I like that suggestion. 👍 |
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. |
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 |
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 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 Has anyone tried to contact GitHub to ask about the possibility of codeowners working for teams who don't have write permission? |
Automating these teams would be great.
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://gh.neting.ccmunity/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 |
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
|
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 |
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The [Contributor's Survey results](nodejs/TSC#882) 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. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: #33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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.
Rather than start with a codeowners for the entire project, I propose
that we start with a small experiment focused on specific subsystems.
Specifically, codeowners for modules, streams, net/tls, http/http2, and
quic (once that lands). We can expand out from there as we see how
things go with the minimal starter set.
A couple of points:
provide coverage for signing off on semver-major changes)
one person on the codeowner team
/cc @nodejs/tsc
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes