RFC 37: Split META.yml suggested_reviewers into suggested_reviewers and notify

[zcorpan]

See the rendered version of this RFC.

[foolip]

Is implementing the round robin review part of this RFC?

[zcorpan]

Yes, clarified below.

[foolip]

I think it'd be better to stop using the assignee field, even though that means I'll have to update mail filters again.

[zcorpan]

Done.

[jgraham]

Yeah, I'm worried about this. In general I think if we were going to have a comment from bots, I'd expect much more value in providing a concise summary of test failures, surfacing warnings, or linking to results, than in reverting to a system where we @-notify people in a comment in the hope that tweaking notifications makes people spend more time reviewing PRs.

[foolip]

The purpose of those comments wouldn't mainly be to get review, but rather:

• move people from suggested_reviewers to notify if they have no intention of reviewing
• put people in both suggested_reviewers and notify if they do want to review but also don't want to miss anything. I think @annevk is likely in this category.

[jgraham]

Sure, but the main concern still applies. If we have some acceptable non-zero number of bot comments we can add without annoying people too much, why is this the most useful thing to spend that budget on? We have a lot of information that's buried several links deep in the checks API that we could instead surface in a comment, for example. To me that seems likely to be higher impact than tweaking the notification system to allow people to be notified without being flagged for review.

[zcorpan]

I don't think having mentions instead of review requests is more important than surfacing other information hidden in CI checks, but I think the problem this RFC is trying to address is worth doing something about.

Personally, I'd love to have a bot comment that surfaces all useful information we want people to see (such as wptpr.live links).

[annevk]

I don't want bot comments. A bot modifying OP as the PR bot for standards does is fine, but adding comments is noisy and annoying.

Does adding usernames in an edit result in notifications for those added?

[zcorpan]

It does, yes. (Tested in web-platform-tests/wpt#20660 )

Editing OP would be ok for me.

[zcorpan]

I've changed the RFC to say to edit OP.

[foolip]

@zcorpan by happenstance I noticed that the GitHub API has a requested_teams field on PRs, and that indeed it's possible to request review from a team. Can you check what this does in terms of notifications and email filtering? I don't think that it's a solution to any of our problems, but if it does something different to requesting from everyone separately it might be worth considering.

[zcorpan]

(I haven't yet looked into requested_teams. I've made a todo to do so in early January.)

Having notify for "I'm interested in everything" could also allow for different delegation of
suggested_reviewers.

For example, myself and @foolip are listed in html/META.yml, but for html/obsolete/META.yml only @foolip is. Right now the latter makes no difference; we're both notified for PRs in obsolete. An alternative would be for @wpt-pr-bot to only consider the nearest META.yml for suggested_reviewers, and all ancestor directories' META.yml for notify.

I don't know if this would be an improvement or a regression, though. Thoughts?

[gsnedders]

Pretty sure that would be a regression. I'm not opposed to some way to non-recursively add yourself as a reviewer/notify or a way to exclude certain children.

[zcorpan]

I've updated this to address comments. I still haven't looked into requested_teams.

[stephenmcgruer]

@zcorpan At this point, are you still interested in pursuing this RFC?

If not, we should determine whether there is appetite from another community member to champion this. It seems like there is enough interest that we shouldn't just drop it.

[zcorpan]

Unfortunately, I don't have bandwidth to work on this at the moment (or the next few months probably). I would be happy if someone else can take this. :)

[stephenmcgruer]

Thanks @zcorpan ; it can be hard to take the step back and acknowledge when one is overloaded, but it's also important so I appreciate you doing so :)

I have reviewed this thread and the RFC itself. Outside of the actual implementation work, it appears there is not that much currently to do here. I came up with the following list:

• Determine what requested_teams does and if would be useful.
• (Optionally) Consider other extensions that would be useful.

For the latter, I took a look through Chromium's OWNER files for inspiration. We have:

• noparent (tells an OWNERS file not to recurse upwards),
• per_file (accepts a glob-pattern for files a particular person cares about),
• file (which appends another OWNERS file onto this one),
• and a WATCHLIST concept which sort of separates our 'notify' and 'reviewer' sets into different files.

[jgraham]

It seems like this is not making progress currently and there's no one who is able to champion it. I'm going to close the issue for now, but we can reopen it if we identify a way forward.