1593700 - Set up global Herald rule for remote agent reviews

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Administration, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

Please see this first for context:
https://groups.google.com/forum/#!msg/mozilla.dev.platform/5GPY-zqEcag/pz0QqHK1DwAJ

This is more a question than a bug:

Would it be possible to set up a global Herald rule in Phabricator that adds the remote-protocol-reviewers group as a blocking reviewer to all changes under remote/?

I can set a personal Herald rule that adds me personally to all reviews touching files under remote/, but having a global group rule would—if I understand this correctly!—effectively mean contributors wouldn’t have to remember to set r=#remote when submitting a changeset.

This would have the very welcome effect that review requests without reviewers assigned would not go unnoticed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would like to add that this should be a blocking reviewer, means r=#remote!.

Comment 2

[David Lawrence [:dkl]]

Yes this is possible. An example would be: https://phabricator.services.mozilla.com/H158

Should I go ahead and do something like that for the remote-protocol-reviewers group?

dkl

Comment 3

[Andreas Tolfsen ❲:ato❳]

That is exactly what I’m talking about!

Could you do that, except instead of Affected files contains .ftl
it needs to be “any file under the top-level directory remote/”.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is there a way to only apply this rule when someone explicitly set a reviewer to a patch? I'm not particular happy in always forcing a review if eg authors are pushing WIP patches and don't wish a review yet. Is there a way to exclude that?

Comment 5

[Andreas Tolfsen ❲:ato❳]

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

Is there a way to only apply this rule when someone explicitly set
a reviewer to a patch?

Applying the rule only when a reviewer is set defeats one of my
stated goals, which is about ensuring we don’t lose track of
patches from contributors who forget to set the reviewer.

I’m sure it would be possible to filter out commits containing the
word WIP or some other special flag though. If you look at the
Herald rule form in Phabricator, perhaps there’s some special
attribute we could use which would match your requirement
for having one-step published reviews without reviewers associated?

Edit: Missed out some words.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't think that I'm the right person to answer that. And I don't have the time to dig into Herald to figure that out. I think David can better help here.

Also note that I'm pretty good in following my mentored bugs and so far we haven't forgotten a patch yet. As such I wouldn't put too much concern into this second part. Part 1 is way more important.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, do you have an update for us? We would kinda appreciate. If you don't know about my question lets just create the rule and figure out the WIP question later.

Comment 8

[David Lawrence [:dkl]]

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

David, do you have an update for us? We would kinda appreciate. If you don't know about my question lets just create the rule and figure out the WIP question later.

Sorry for the delay on this. I have created the new rule: https://phabricator.services.mozilla.com/H196

Please close this when a revision has been created and the rule has worked as expected.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I pushed a patch set with two revisions but it doesn't work:

https://phabricator.services.mozilla.com/D52929
https://phabricator.services.mozilla.com/D52831

Comment 10

[David Lawrence [:dkl]]

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

I pushed a patch set with two revisions but it doesn't work:

https://phabricator.services.mozilla.com/D52929
https://phabricator.services.mozilla.com/D52831

Ok. So using the regex pattern that I selected was not the correct way to look for files. So I have edited the rule some and seems to work better now.

There is a way in Herald to test rules without making actual changes and the new change is working according to
https://phabricator.services.mozilla.com/herald/transcript/3054396/

dkl

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry that I have to reopen but we will have to clarify the following issue... Our group was inappropriately added as blocking reviewer to https://phabricator.services.mozilla.com/D51921. I will revert that for now but please check what's wrong here.

I assume it's Affected files contains remote/ which also matches against any sub folder.

Also the above link gives tons of entries. Not sure what that all is.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that I cannot fix the other review in Phabricator. Can you please do that?

Comment 13

[David Lawrence [:dkl]]

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

Sorry that I have to reopen but we will have to clarify the following issue... Our group was inappropriately added as blocking reviewer to https://phabricator.services.mozilla.com/D51921. I will revert that for now but please check what's wrong here.

I assume it's Affected files contains remote/ which also matches against any sub folder.

Also the above link gives tons of entries. Not sure what that all is.

Ugh. Ok let's try another way. I have made another change to the rule that works properly for D52929 and does not match D51921.

https://phabricator.services.mozilla.com/H196

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I updated bug 1544417 right now and Herald actually added the remote reviewer group as blocking reviewer. Looks like it's working now.

I will file other bugs for geckodriver and marionette in a moment.

Thanks David!

Comment 15

[Andreas Tolfsen ❲:ato❳]

Thank you, dkl!

This is going to prevent accidental changes from non-peers in the remote debugger.