Bug 1584998 - Need automatic inclusion of few reviewers to a given patch
Summary: Need automatic inclusion of few reviewers to a given patch
Keywords:
Status: NEW
Alias: None
Product: GlusterFS
Classification: Community
Component: project-infrastructure
Version: mainline
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: ---
Assignee: bugs@gluster.org
QA Contact:
URL:
Whiteboard: Process-Automation
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-06-01 06:24 UTC by Amar Tumballi
Modified: 2019-05-10 13:20 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:


Attachments (Terms of Use)

Description Amar Tumballi 2018-06-01 06:24:54 UTC
# Description of problem:

There is no easy way for a new comer to the project to add a reviewer based on component to a patch. And as I noticed, only when someone is added as a reviewer, they review the patch.

So, I propose, like we close the bug due to 90 days of inactivity, we should automatically add all our general architects, and few maintainers (list can be taken from MAINTAINERS file) to the given patch if there is no activity on a patch.


# Additional info:

A comment saying "No activity seen on patch since last 2 weeks! Plan to take some action, otherwise it would get on to inactivity queue and would be marked for abandon after 90 days" doesn't do a harm after adding reviewers.

Comment 1 Amar Tumballi 2018-06-01 09:24:10 UTC
Notice that this has a problem that we can get into infinite loop of warning people every 2weeks of inactivity, but never reaching 90days. Hence we need a mechanism to identify the patches only once, ie, for the first time. After that, it should wait for 90days, and get abandon'd.

Comment 2 Shyamsundar 2018-06-01 14:00:19 UTC
My thoughts:

Let's not do it after 2 weeks, let it happen as the review is posted.

Also, let's add MAINTAINERS as per their areas of responsibility, that way a patch does not get 20 reviewers, but rather 2-4 reviewers as the case maybe.

Comment 3 Niels de Vos 2018-06-03 15:34:35 UTC
If reviewers are added automatically, but still no action is taken after ~2 weeks, it may make sense to send an email with some details about the proposed change to the maintainers mailing list.

Comment 4 Shyamsundar 2018-06-04 13:28:11 UTC
(In reply to Niels de Vos from comment #3)
> If reviewers are added automatically, but still no action is taken after ~2
> weeks, it may make sense to send an email with some details about the
> proposed change to the maintainers mailing list.

Yes agree, the 2 week (or time based) reminder still applies.

Comment 5 Nigel Babu 2018-10-05 08:41:01 UTC
I'm going to be a bit skeptical here. If someone doesn't notice their first email notification, would an additional email actually help?

Our recommendation is that reviewers subscribe to their area of interest via Gerrit directly. The system is built to be an opt-in rather than one that does the whining for you.

Comment 6 Niels de Vos 2018-10-05 09:31:09 UTC
(In reply to Nigel Babu from comment #5)
> Our recommendation is that reviewers subscribe to their area of interest via
> Gerrit directly. The system is built to be an opt-in rather than one that
> does the whining for you.

This is something that we really need to be following more closely. I suspect that some of the maintainers and peers are not very aware of the guidelines we decided on long ago. When we onboard new maintainers/peers, they should have a (ideally face-to-face) discussion with an active maintainer about it. This includes setting up notification for patches as described in the doc.

https://docs.gluster.org/en/latest/Contributors-Guide/Guidelines-For-Maintainers/

And probably also https://github.com/gluster/glusterdocs/blob/master/docs/Developer-guide/Bugzilla%20Notifications.md (not sure why I can not find it on docs.gluster.org). Is there a similar doc for GitHub issues?

Comment 7 Shyamsundar 2018-10-05 12:50:52 UTC
(In reply to Nigel Babu from comment #5)
> I'm going to be a bit skeptical here. If someone doesn't notice their first
> email notification, would an additional email actually help?
> 
> Our recommendation is that reviewers subscribe to their area of interest via
> Gerrit directly. The system is built to be an opt-in rather than one that
> does the whining for you.

This is more for fly-by and/or folks who want to add reviewers clearly to call to attention the patch submitted.

Often I read the MAINTAINERS file and add reviewers based on the area of ownership, if we had this auto addition it makes life easier for patch submitter.

So in short, my thought is to have this not for the maintainers/owners, but for submitter as they now know in gerrit who should look at the patch or at least that it is passed onto someone's queue.

Comment 8 Nigel Babu 2018-10-05 13:08:19 UTC
Alright, so let me put down Shyam's suggestion into a good infra action:

Run a job in smoke which checks if the maintainers for the component are already added. If yes, do nothing. If not, add them. If a patch spans more than one component, we will have to do some magic. I'm going to try to do something that works for when it's a single component and then we can optimize further as we go along.

Does this sound like a good plan of action? It's not a complete solution but helps drive by contributors.

Comment 9 Niels de Vos 2018-10-05 14:06:12 UTC
(In reply to Shyamsundar from comment #7)
> This is more for fly-by and/or folks who want to add reviewers clearly to
> call to attention the patch submitted.

Ideally this should not be needed. However I agree that many maintainers/peers are not pro-active in reviewing changes that get posted. It helps a lot when adding people to review changes, and automating it might be useful.

Is this only applicable to the glusterfs project in Gerrit? I wonder how/if the maintainers of the other projects watch out for new changes.


Note You need to log in before you can comment on or make changes to this bug.