1547803 - reviewer group is listed in list of reviewers beside real reviewer

Status: RESOLVED FIXED

(Conduit :: Lando, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

See: https://phabricator.services.mozilla.com/D28047

For the review I selected the perftest reviewer group. Then rwood reviewed the patch. Landing the revision via lando still had the perfest reviewer group included.

To exactly know who reviewed the patch, the group should not be listed in the list of reviewers:

https://hg.mozilla.org/integration/autoland/rev/50b7f764f4af

Comment 1

[Israel Madueme [:imadueme]]

Lando normally handles removing the group but it looks like the space in the Looks Like field for the perftest_reviewer group is confusing things. https://phabricator.services.mozilla.com/tag/perftest_reviewers/

Comment 2

[Tom Prince [:tomprince]]

Based on Bug 1469380, having the group in the reviewers is expected. I think the space in the name is a bug that should be addressed, though.

Comment 3

[:glob ✱]

groups can't have spaces in their names for this reason; i'll reopen bug 1547248

Comment 4

[David Lawrence [:dkl]]

I have fixed the project name issue in bug 1547248 to remove the space. I will update our Mana docs to be specific that spaces are not allowed for reviewer project groups. Feel free to close this if it fixes the issue in Lando.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Attachment: Screenshot 2019-04-30 at 10.02.30.png

No, it still doesn't work. See the attached screenshot.

Comment 6

[Tom Prince [:tomprince]]

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

No, it still doesn't work. See the attached screenshot.

The expected behavior (as discussed in bug 1547248) is to include the group reviewer in the commit message. This is new with phabriactor/lando, as there was not really the concept of a group reviewer before.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Tom Prince [:tomprince] from comment #6)

The expected behavior (as discussed in bug 1547248) is to include the group reviewer in the commit message. This is new with phabriactor/lando, as there was not really the concept of a group reviewer before.

Sorry but I don't see a discussion about that on the referenced bug. Only a vague comment from :dkl, which I can't fully parse to be as what you say. I feel that including the group is useless given that it doesn't tell who reviewed the code.

I will reopen for now.

Comment 8

[Tom Prince [:tomprince]]

Sorry, I copied the wrong bug number from an earlier comment. Bug 1469380 is where this is discussed.