feat: ping reviewers based on which files were changed #265

mmarchini · 2020-07-01T04:13:37Z

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

mmarchini · 2020-07-01T04:15:36Z

Still need to add tests, but I'm looking for feedback on the implementation (using a YAML file, fetching from core, etc.). This has been discussed already in nodejs/node#33895 and nodejs/node#33984 (and a couple old pull requests where CODEOWNERS were first introduced but then removed because of the GitHub limitations mentioned above).

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

phillipj · 2020-07-01T07:17:24Z

Conceptually looks good to me 👍

The fact that the definition lives inside the node repo is surely a step in a more sustainable direction going forward.

I'm not opinionated about the format and naming of OWNERS | CODEOWNERS, whatever makes most sense for the core maintainers.

lundibundi

👍

lib/node-owners.js

lundibundi · 2020-07-01T09:11:03Z

lib/node-repo.js

+    debug(`Fetching OWNERS on ${url}`)
+    request(url, (err, res, body) => {
+      if (err || !res || res.statusCode >= 400) {
+        return options.logger.error(err, 'Error retrieving OWNERS')


Perhaps, also log res (or at least res.statusCode) in this case?

Agreed, but can be done in a follow up PR (so we don't block the PR)

mmarchini · 2020-08-02T04:09:26Z

Previous version was a merge-conflict show, it was easier for me to go revert the changes and copy them manually. I added all new functions after current function to avoid any future conflicts if this doesn't land soon.

Main changes:

Rewrote everything in async/await fashion since we started to use it
Broke down all steps into separate function to make testing easier
The format is not YAML anymore, we're using the same format GitHub does
- No need to create a new file on nodejs/node amymore, as soon as we land this the bot will start ping folks :D
Added tests
- I used nock for all tests because using sinon to stub github API calls will supress deprecation warnings that are helpful when bumping major versions of the github rest API package

lib/node-repo.js

mcollina

Rubberstamp lgtm, let's try this.

mmarchini · 2020-08-04T22:59:43Z

@phillipj do you want to take another look before we land this? Was hoping to land it this week (but if you want to take another look and don't have time this week we can postpone it)

Using .github/CODEOWNERS, `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). Ref: nodejs/node#33984

mmarchini · 2020-08-07T19:08:49Z

@phillipj feel free to ping me if you have any comments, suggestions or concerns on this PR. I can send a patch addressing those or we can revert if necessary.

phillipj · 2020-08-07T19:15:42Z

Gotcha! Sorry for the late reply, your ping a few days ago drowned in my mailbox somehow.

…

On Fri, 7 Aug 2020 at 21:09, mary marchini ***@***.***> wrote: @phillipj <https://github.com/phillipj> feel free to ping me if you have any comments, suggestions or concerns on this PR. I can send a patch addressing those or we can revert if necessary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#265 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMWE7ASGJNCN3APUE3IPLR7RGM7ANCNFSM4ONAGOOQ> .

A recent feature was added to github-bot to ping codeowners defined on the CODEOWNERS file even if the team doesn't have write permission to the repository. That means we can enable codeowners everywhere in the repository. Ref: nodejs/github-bot#265 Fix: nodejs#33984

phillipj

Belated LGTM, nice one 💯

A recent feature was added to github-bot to ping codeowners defined on the CODEOWNERS file even if the team doesn't have write permission to the repository. That means we can enable codeowners everywhere in the repository. Ref: nodejs/github-bot#265 Fix: #33984 PR-URL: #34670 Fixes: #33984 Refs: nodejs/github-bot#265 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mmarchini mentioned this pull request Jul 1, 2020

meta: introduce custom OWNERS file nodejs/node#34150

Closed

4 tasks

mmarchini force-pushed the owners branch from af8b06a to 6ccbba9 Compare July 1, 2020 04:17

lundibundi reviewed Jul 1, 2020

View reviewed changes

mmarchini mentioned this pull request Jul 28, 2020

meta: enable http2 team for CODEOWNERS nodejs/node#34534

Merged

1 task

mmarchini force-pushed the owners branch 2 times, most recently from 2befe4b to 8d158a9 Compare August 2, 2020 04:07

mmarchini mentioned this pull request Aug 2, 2020

CODEOWNERS next steps nodejs/node#33984

Closed

targos approved these changes Aug 2, 2020

View reviewed changes

lib/node-repo.js Outdated Show resolved Hide resolved

lib/node-repo.js Outdated Show resolved Hide resolved

lib/node-repo.js Outdated Show resolved Hide resolved

lib/node-repo.js Outdated Show resolved Hide resolved

lib/node-repo.js Outdated Show resolved Hide resolved

mcollina approved these changes Aug 2, 2020

View reviewed changes

mmarchini mentioned this pull request Aug 4, 2020

more subteams under Collaborators nodejs/admin#537

Closed

mmarchini force-pushed the owners branch from 37dd9b2 to 8f5863a Compare August 5, 2020 21:09

mmarchini merged commit e64662f into nodejs:master Aug 7, 2020

mmarchini deleted the owners branch August 7, 2020 19:07

mmarchini mentioned this pull request Aug 7, 2020

meta: uncomment all codeowners nodejs/node#34670

Closed

4 tasks

phillipj reviewed Aug 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ping reviewers based on which files were changed #265

feat: ping reviewers based on which files were changed #265

mmarchini commented Jul 1, 2020 •

edited

Loading

mmarchini commented Jul 1, 2020

phillipj commented Jul 1, 2020

lundibundi left a comment

lundibundi Jul 1, 2020

mmarchini Aug 2, 2020

mmarchini commented Aug 2, 2020

mcollina left a comment

mmarchini commented Aug 4, 2020 •

edited

Loading

mmarchini commented Aug 7, 2020

phillipj commented Aug 7, 2020 via email

phillipj left a comment

feat: ping reviewers based on which files were changed #265

feat: ping reviewers based on which files were changed #265

Conversation

mmarchini commented Jul 1, 2020 • edited Loading

mmarchini commented Jul 1, 2020

phillipj commented Jul 1, 2020

lundibundi left a comment

Choose a reason for hiding this comment

lundibundi Jul 1, 2020

Choose a reason for hiding this comment

mmarchini Aug 2, 2020

Choose a reason for hiding this comment

mmarchini commented Aug 2, 2020

mcollina left a comment

Choose a reason for hiding this comment

mmarchini commented Aug 4, 2020 • edited Loading

mmarchini commented Aug 7, 2020

phillipj commented Aug 7, 2020 via email

phillipj left a comment

Choose a reason for hiding this comment

mmarchini commented Jul 1, 2020 •

edited

Loading

mmarchini commented Aug 4, 2020 •

edited

Loading