Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meta: introduce custom OWNERS file #34150

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Jul 1, 2020

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: #33984
Ref: nodejs/github-bot#265

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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
mmarchini added a commit to mmarchini/github-bot that referenced this pull request Jul 1, 2020
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
@targos
Copy link
Member

targos commented Jul 1, 2020

Couldn't we still use the CODEOWNERS file instead of a custom name and format?

@mmarchini
Copy link
Contributor Author

Sure, I just went with YAML to avoid writing a parser, although the parser shouldn't be that complex

@targos
Copy link
Member

targos commented Jul 1, 2020

There's probably an open source parser on npm already :)
If not, I don't object to using another format

@mmarchini
Copy link
Contributor Author

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.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything with dns in the filename should probably use @nodejs/dns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are aiming for compatibility with CODEOWNERS here, the CODEOWNERS format does not like the leading . at the start of the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, the file was converted from our own CODEOWNERS. I don't mind removing the leading ./ from it though.

@jasnell
Copy link
Member

jasnell commented Jul 1, 2020

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.

@mmarchini
Copy link
Contributor Author

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.

image

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.

@mmarchini
Copy link
Contributor Author

OTOH since @nodejs/collaborators is the only team with explicit write permissions here, that's the only team that can be added as reviewer, therefore there's no risk of conflict between the bot and GitHub codeowners. We just need to remove any subteams from collaborators once the bot starts to ping teams.

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.

@mmarchini mmarchini closed this Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants