1636024 - moz-phab should suggest reviewers if they were not provided

Status: NEW

(Conduit :: moz-phab, enhancement, P2)

Description

[:glob ✱]

If a non-WIP change is missing reviewers moz-phab should suggest a reviewer.

I wrote a Mercurial only POC a month ago which appears to do a reasonable job:
https://gist.github.com/globau/45deecabe039753c3222aceb4118abf0

It looks reviewers of the touched files or paths over the last 60 days and lists the top 5 reviewers, prioritising reviewer groups over individuals.

It's far from perfect, but it's better than nothing when the patch author has no idea where to direct a review.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Hm, so what's different to the current behavior? When I push a non-WIP patch for changes in /remote and not adding a reviewer explicitely it works just fine:

(D74635) 592842:be9d5bd2d078 Bug 1593226 - [remote] Emit Runtime.executionContextCreated and Runtime.executionContextDestroyed events also for frames.
!! Missing reviewers
Automatically submitting (as per submit.auto_submit in ~/.moz-phab-config)

Updating revision D74635:
592842:be9d5bd2d078 Bug 1593226 - [remote] Emit Runtime.executionContextCreated and Runtime.executionContextDestroyed events also for frames.

Maybe this is because we have a herald rule setup for any changes under /remote? Byron, what does your patch change? Wont' we need such herald rules anymore?

Also we have a contributor for the remote protocol who doesn't have any privileges yet, and he is not able to push a change without specifying a reviewer. Is that somewhat related or a different issue?

Comment 2

[:glob ✱]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

Maybe this is because we have a herald rule setup for any changes under /remote? Byron glob, what does your patch change? Wont' we need such herald rules anymore?

Thanks for raising this.

This proposal is to suggest reviewers if one was not provided when submitting the patch to Phabricator. Depending on how the herald rule is built this would either replace or supplement the reviewer list. As automatic reviewer assignment operates is inconsistent across teams we might instead have to implement this as a separate command.

Also we have a contributor for the remote protocol who doesn't have any privileges yet, and he is not able to push a change without specifying a reviewer. Is that somewhat related or a different issue?

Unrelated - moz-phab only issues a warning for missing reviewers; it isn't an fatal error.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

(In reply to :glob (Byron Jones) 🎈 from comment #2)

Also we have a contributor for the remote protocol who doesn't have any privileges yet, and he is not able to push a change without specifying a reviewer. Is that somewhat related or a different issue?

Unrelated - moz-phab only issues a warning for missing reviewers; it isn't an fatal error.

Right, he clarified that with me now that it was just the output. Good to know it works.

Comment 4

[Marco Castelluccio [:marco]]

This could be done with mots in the future (bug 1773291).