[dpdk-dev] Suggestion to improve the code review
Burakov, Anatoly
anatoly.burakov at intel.com
Wed May 27 11:59:42 CEST 2020
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
More information about the dev
mailing list