From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1D5D8A04A4; Wed, 27 May 2020 11:59:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 32BCE1D666; Wed, 27 May 2020 11:59:50 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 20C3F1D660; Wed, 27 May 2020 11:59:47 +0200 (CEST) IronPort-SDR: TAVwbRHzqIXDrtWnQzwEUZXaLNRGtp2nBi1tO84dxHOIVahg9GY1ZiGo9cSwk955czz8GUSL22 LiJMORYa4BzQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 May 2020 02:59:46 -0700 IronPort-SDR: XnGdoGXG34+EiKK++ep+sSPthMCSB4uP/Pon+zhKBlYgiq6GDXuGUuC9xC9t4MIdfUN3FYNPbm OVPOpKjy4/wQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,440,1583222400"; d="scan'208";a="255730639" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.254.185.66]) ([10.254.185.66]) by orsmga007.jf.intel.com with ESMTP; 27 May 2020 02:59:43 -0700 To: Jerin Kollanukkaran , dpdk-dev , Thomas Monjalon , "david.marchand@redhat.com" , "Yigit, Ferruh" , Maxime Coquelin , "cristian.dumitrescu@intel.com" , "akhil.goyal@nxp.com" , "rasland@mellanox.com" , "xiaolong.ye@intel.com" , "ajit.khaparde@broadcom.com" , "arybchenko@solarflare.com" Cc: "techboard@dpdk.org" References: From: "Burakov, Anatoly" Message-ID: Date: Wed, 27 May 2020 10:59:42 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] Suggestion to improve the code review X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 27-May-20 10:28 AM, 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/ > The "which patches should i review first" button is a huge +1000 from me, as this has been a big issue i've had with current workflow for a long time. Thomas has mentioned "Cc:" as a "fine grained" system to assign patches, but the truth is, CC is not a good way of doing it because i get CC'd in all kinds of stuff, and the important patches get lost. That said, i don't think it's that easy, because more often than not, patches touch a lot of different areas, so a one line change in meson, a test and a line in EAL gets half of DPDK maintainers CC'd into the patch. I wonder if there is a mechanism for some kind of "threshold" for assigning people to the patch - i.e. if a one-liner is half of the changes in the patch, then maintainer gets CC'd, but if a one-liner is just one of a thousand other unrelated lines, then perhaps there's no need to CC the maintainer... or something along those lines :) there's a machine learning project in here somewhere :D -- Thanks, Anatoly