Re: [dpdk-dev] Suggestion to improve the code review

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, Jun 2, 2020 at 8:27 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 6/2/2020 1:27 PM, Jerin Jacob wrote:
> > On Wed, May 27, 2020 at 3:38 PM Gaëtan Rivet <grive@u256.net> wrote:
> >>
> >> On 27/05/20 09:28 +0000, Jerin Kollanukkaran wrote:
> >>> I think, original discussion[1] on this topic got lost in GitHub vs current workflow.
> >>>
> >>>
> >>> I would like to propose GitHub "CODEOWNERS"[2] _LIKE_ scheme for DPDK workflow.
> >>>
> >>> Current scheme:
> >>> - When we submit a patch to ml, someone(Tree maintainer[3]) needs to manually
> >>> delegate the patch to Tree maintainer in patchwork.
> >>> - Tree maintainer is not responsible for the review of the patch but only responsible
> >>> for merging _after_ the review. That brings the obvious question on review responsibility.
> >>>
> >>>
> >>> Proposed scheme:
> >>> - In order to improve review ownership, IMO, it is better the CI tools delegate
> >>> the patch to the actual maintainer(who is responsible for specific code in MAINTAINERS file)
> >>> - I believe, it provides a sense of ownership, avoids last-minute surprise on
> >>> review responsibility and improve review traceability.
> >>>
> >>> Implementation of the proposed scheme:
> >>> GitHub provides a bot for CODEOWNERS integration, Similar alternative is possible with
> >>> patchwork with "auto delegation scheme" using the flowing methods:
> >>>
> >>> a) https://patchwork.readthedocs.io/en/latest/usage/delegation/
> >>> b) https://patchwork.readthedocs.io/en/latest/usage/headers/
> >>>
> >>> I think, option (a) would be relatively easy to change without introducing the new tools.
> >>>
> >>> Thoughts?
> >>>
> >>> [1]
> >>> http://mails.dpdk.org/archives/dev/2020-May/168740.html
> >>> [2]
> >>> https://github.com/zephyrproject-rtos/zephyr/blob/master/CODEOWNERS
> >>> [3]
> >>> https://patchwork.dpdk.org/project/dpdk/
> >>>
> >>
> >> Hi,
> >>
> >> +1 from me. People would be able to list current assigned tasks through
> >> pwclient. It would help reviews IMO.
> >
> > So far no objection to this proposal. Any other thoughts from anyone?
> > especially from the code maintainers.
> >
> > Thomas, Any input as patchwork maintainer. This would boil down to the
> > following change in patchwork.
> >
> > 1) Add code maintainers are maintainers in patchwork.
> > 2) Enable existing auto delegation[1] feature of Patchwork
> > [1]
> > a) https://patchwork.readthedocs.io/en/latest/usage/delegation/
> > b) https://patchwork.readthedocs.io/en/latest/usage/headers/
> >
> > The suggested process is:
> > # When a patch gets submitted to ml, patchwork finds the code owner
> > based on the MAINTAINER file using the auto delegation feature.
> > # The code maintainer will be responsible for the "review" of that
> > patch and patch will be delegate will code owner using auto delegation
> > feature.
> > # If multiple code maintainers operate on the same patch, "each code
> > maintainer" can assign to "other code maintainer" once he is done with
> > the review.
> > # The existing review process will be followed as is, just that we are
> > adding code maintainer have primary review responsibility for the
> > patch and expressing in the patchwork.
> > # Based on the Ack's received and/or when code owner is happy with
> > changes, he/she can change the state  to "Awaiting upstream" and
> > assign to respective
> > tree maintainer.
> > # Finally, Tree maintainer will merge the patch to respective tree and
> > make the state as  "Accepted"
> >
>
> +1 from me, this can help maintainers to figure out patches waiting for their
> review.
>
> Did you have a chance to test auto delegation, will it work for us?

I think, it can be done in two ways

a) https://patchwork.readthedocs.io/en/latest/usage/delegation/
b) https://patchwork.readthedocs.io/en/latest/usage/headers/

Option (a) need patchwork admin access and no dependency on email
client nor separate step[1]. I think, only Thomas only has access to
that.
I tested the option (b). It is not working, it is not straight forward
as we need to specific header to email[1]
Based on my debugging, Even though when I did "add-header", it is not
showing up on received email. Somewhere it is getting removed[2]

[1]
git send-email --to dev@dpdk.org --add-header="X-Patchwork-Delegate:
ferruh.yigit@intel.com"
0001-test-test-patch-for-checking-patchwork-auto-delegati.patch
[2]
http://patches.dpdk.org/patch/70749/