feat: ping reviewers based on which files were changed by mmarchini · Pull Request #265 · nodejs/github-bot · GitHub

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

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

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.

👍

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)

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

Rubberstamp lgtm, let's try this.

@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

@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.

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

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

Comment thread lib/node-repo.js Outdated

Comment thread lib/node-repo.js Outdated

Comment thread lib/node-repo.js Outdated

Comment thread lib/node-repo.js Outdated

Comment thread lib/node-repo.js Outdated

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

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

feat: ping reviewers based on which files were changed by mmarchini · Pull Request #265 · nodejs/github-bot · GitHub

Uh oh!

Uh oh!

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

feat: ping reviewers based on which files were changed#265mmarchini merged 1 commit intonodejs:masternodejs/github-bot:masterfrom mmarchini:ownersmmarchini/github-bot:ownersCopy head branch name to clipboard

Conversation

mmarchini commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmarchini commented Jul 1, 2020

Uh oh!

phillipj commented Jul 1, 2020

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lundibundi Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

mmarchini Aug 2, 2020

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Aug 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmarchini commented Aug 7, 2020

Uh oh!

phillipj commented Aug 7, 2020 via email

Uh oh!

phillipj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: ping reviewers based on which files were changed#265
mmarchini merged 1 commit into
nodejs:masterfrom
mmarchini:owners

mmarchini commented Jul 1, 2020 •
edited
Loading

mmarchini commented Aug 4, 2020 •
edited
Loading