meta: introduce custom OWNERS file #34150
Conversation
Introduces a custom OWNERS file, which is intended to be handled by `github-bot` as a way to work around GitHub CODEOWNERS limitation which prevents teams without explicit write access from being added as reviewers. Ref: nodejs#33984 Ref: nodejs/github-bot#265
Using a custom OWNERS file, `github-bot` will ping the appropriate teams based on which files were changed in a Pull Request. This feature is inteded to work around GitHub's limitation which prevents teams without explicit write access from being added as reviewers (thus preventing the vast majority of teams in the org from being used on GitHub's CODEOWNERS feature). The OWNERS file is a yaml file (to simplify parsing) composed of key-value paris. The key is always a string and represents a glob of the changed files. The value is always an array of strings, and each string is a team to be pinged. Ref: nodejs/node#33984 Ref: nodejs/node#34150
|
Couldn't we still use the CODEOWNERS file instead of a custom name and format? |
|
Sure, I just went with YAML to avoid writing a parser, although the parser shouldn't be that complex |
|
There's probably an open source parser on npm already :) |
|
This one might work: https://www.npmjs.com/package/codeowners-utils, I'll try it once I got some time to work on this again (probably later this week or over the weekend). Other packages I checked when I was implementing it required the repository to be cloned, which seemed like too much for that use case. |
There was a problem hiding this comment.
Couldn't we still use the CODEOWNERS file instead of a custom name and format?
@targos I'm not @mmarchini but my reaction would be 🤷 - who cares? As long as it works. I'm fine with the current approach.
| # net | ||
|
|
||
| ./deps/cares: ["@nodejs/net"] | ||
| ./doc/api/dns.md: ["@nodejs/net"] |
There was a problem hiding this comment.
Everything with dns in the filename should probably use @nodejs/dns.
There was a problem hiding this comment.
For context, I just got the CODEOWNERS we have on the repo today, uncommented the files/teams pairs that were commented due to the GitHub reviewers limitations, and converted the file to YAML. I didn't vet the teams assigned to each file/glob.
I like the suggestion of pinging @nodejs/dns here though.
| ./src/connection_wrap.*: ["@nodejs/net"] | ||
| ./src/node_sockaddr*: ["@nodejs/net"] | ||
| ./src/tcp_wrap.*: ["@nodejs/net"] | ||
| ./src/udp_wrap.*: ["@nodejs/net"] |
There was a problem hiding this comment.
@nodejs/net is only three people, one of them inactive... Not a change suggestion, just an observation.
| ./lib/http2.js: ["@nodejs/http2", "@nodejs/net"] | ||
| ./lib/internal/http2/*: ["@nodejs/http2", "@nodejs/net"] | ||
| ./src/node_http2*: ["@nodejs/http2", "@nodejs/net"] | ||
| ./src/node_mem*: ["@nodejs/http2"] |
There was a problem hiding this comment.
If we are aiming for compatibility with CODEOWNERS here, the CODEOWNERS format does not like the leading . at the start of the path.
There was a problem hiding this comment.
As mentioned above, the file was converted from our own CODEOWNERS. I don't mind removing the leading ./ from it though.
|
I'd prefer that we simply reuse the CODEOWNERS file directly, rather than inventing a second mechanism. Specifically, we would rely on CODEOWNERS where it works and use the bot to supplement when it doesn't (e.g. when a owning team does not have the necessary permissions, the bot would handle the review notification. |
|
I think using both would be confusing. To be clear, we can't even add a team as reviewer if they don't have explicit write permission, so the only way to notify them is via comments. I tried to bypass it via API calls too and no dice.
We would end up with a mix of comment and reviewers (confusing IMO). We can't even add TSC because it doesn't have explicit permission (only indirect via admin privileges), and neither can CODEOWNERS (see #34039, TSC should be reviewer but GH didn't add it). Unfortunately, GitHub's CODEOWNSERS just doesn't fit our organization. We can reuse the format (although I don't see much value in doing so, but I'm also not opposed to it), but mixing the features is not a good idea. |
|
OTOH since I'll give https://www.npmjs.com/package/codeowners-utils a try to see if we can reuse the CODEOWNERS file, if it doesn't work I'll reopen this. |
Introduces a custom OWNERS file, which is intended to be handled by
github-botas a way to work around GitHub CODEOWNERS limitation whichprevents teams without explicit write access from being added as
reviewers.
Ref: #33984
Ref: nodejs/github-bot#265
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes