Re: [dpdk-ci] [PATCH v2 05/10] tools: add functionality for setting pw delegates

[Ali Alnubani <alialnu@nvidia.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, November 4, 2021 8:16 PM
> To: Ali Alnubani <alialnu@nvidia.com>
> Cc: ci@dpdk.org; jerinj@marvell.com; ferruh.yigit@intel.com;
> david.marchand@redhat.com; juraj.linkes@pantheon.tech
> Subject: Re: [PATCH v2 05/10] tools: add functionality for setting pw
> delegates
> 
> 04/11/2021 17:48, Ali Alnubani:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 18/10/2021 09:48, Ali Alnubani:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 21/09/2021 16:35, alialnu@nvidia.com:
> > > > > > From: Ali Alnubani <alialnu@nvidia.com>
> > > > > >
> > > > > > A new command was added to set patch delegates in Patchwork
> > > > > > based on the emails found in DPDK's MAINTAINERS file.
> > > > > >
> > > > > > Example usage:
> > > > > >   $ export MAINTAINERS_FILE_PATH=/path/to/dpdk/MAINTAINERS
> > > > > >   $ ./pw_maintainers_cli.py --type series set_pw_delegate
> SERIES_ID
> > > > > >
> > > > > > Signed-off-by: Ali Alnubani <alialnu@nvidia.com>
> > > > > > ---
> > > > > > +    def set_delegate(self, patch_list, delegate):
> > > > > > +        """Set the delegate for a patch.
> > > > > > +        Only tries to set a delegate for patches that don't have
> > > > > > +        one set already.
> > > > >
> > > > > I'm not sure we should skip patches which are already delegated.
> > > > > If we use the command to explicitly delegate the patch,
> > > > > we should do it, right?
> > > > >
> > > > > The skip logic may be implemented at a higher level in the CI.
> > > >
> > > > I added an arg (--force_set_delegate) to force overriding delegates in
> v3.
> > >
> > > Given the command is to set delegate, the force looks strange to me.
> > > Wouldn't it be more logical to add an option to skip already delegated
> > > patches?
> >
> > Isn't it safer not to force overriding the delegate by default?
> > Users can send patches with the header "X-Patchwork-Delegate" to set a
> specific delegate. Maintainers might want to set another user as the
> delegate when there are multiple delegates in the MAINTAINERS file for a
> patch. Should we override these by default?
> 
> These considerations don't contradict with the option name.
> If we have the option --skip-delegated (or --only-non-delegated),
> we can always use this option in the automatic run.
> 
> But for a manual run, I expect the script to delegate a patch
> the user explicitly wants to set, without requiring a "force" option.

Thanks for the feedback, updated in v4 and added a --skip-delegated arg instead:
https://inbox.dpdk.org/ci/20211108062815.7614-6-alialnu@nvidia.com/T/#u