回复: [edk2-devel] Tianocore community page on who we are - please review

["Leif Lindholm" <leif@nuviainc.com>]

Agreed.

Reviever or Maintainer can approve a patch. Any Maintainer can push a
patch that has been approved. This can happen either:
- when the designated Maintainer for that patch is
  unavailable/unresponsive
- if the patch submitter is also a Maintainer of some other part of
  the repo.

No one can approve their own patches.

The act of adding a Reviewer means delegating the approval work to
them.

Best Regards,

Leif

On Wed, Sep 30, 2020 at 17:25:38 +0800, gaoliming wrote:
> Jiewen:
>   Frankly speaking, I don't know this rule that the patch needs to
>   get review or ack from the maintainer. When the reviewer name is
>   formally added into maintainers.txt, I think the maintainer has
>   delegated the approval work to reviewers. So, I think that the
>   reviewer takes the same role to the maintainer except for the
>   patch merge.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: bounce+27952+65748+4905953+8761045@groups.io
> > <bounce+27952+65748+4905953+8761045@groups.io> 代表 Yao, Jiewen
> > 发送时间: 2020年9月30日 10:12
> > 收件人: Leif Lindholm <leif@nuviainc.com>
> > 抄送: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
> > Guptha, Soumya K <soumya.k.guptha@intel.com>;
> > announce@edk2.groups.io; lersek@redhat.com; Kinney, Michael D
> > <michael.d.kinney@intel.com>; 'Andrew Fish' <afish@apple.com>
> > 主题: Re: [EXTERNAL] RE: [edk2-announce] 回复: [edk2-devel] Tianocore
> > community page on who we are - please review
> > 
> > Hi Leif and Liming
> > I have double checked with Mike Kinney on the role and responsibility of
> > reviewers.
> > Mike and I reach the consensus below (a short version, detail will be added to
> > the wiki page later):
> > 
> > 1) Maintainers are the ONLY ones who can approve a patch.
> > 2) Reviewers CANNOT approve the patch. (*)
> > 3) A maintainer CANNOT approve his/her own patch.
> > 4) Maintainers MAY delegate the approval work to reviewers.
> > 
> > So the final state of the commit message as a minimum must be either:
> >              Reviewed-by: <Package Maintainer>
> > Or:
> >              Acked-by: <Package Maintainer>
> >              Reviewed-by: <Package Reviewer>
> > 
> > All in all, I don’t think it is correct to say "Reviewers can approve the patch.
> > The only additional work from maintainers is to check in the patch".
> > 
> > Please let us know if you have different thought.
> > 
> > Thank you
> > Yao Jiewen
> > 
> > > -----Original Message-----
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Monday, September 28, 2020 8:02 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
> > Guptha,
> > > Soumya K <soumya.k.guptha@intel.com>; announce@edk2.groups.io;
> > > lersek@redhat.com; Kinney, Michael D <michael.d.kinney@intel.com>;
> > 'Andrew
> > > Fish' <afish@apple.com>
> > > Subject: Re: [EXTERNAL] RE: [edk2-announce] 回复: [edk2-devel]
> > Tianocore
> > > community page on who we are - please review
> > >
> > > Hi Jiewen,
> > >
> > > On Sun, Sep 27, 2020 at 03:25:24 +0000, Yao, Jiewen wrote:
> > > > Thanks Liming.
> > > >
> > > > It seems I have some misunderstanding here.
> > > >
> > > > According to current process, I feel that only maintainer has right to
> > > *approve* the patch.
> > > > The reviewer cannot approve the patch.
> > > > Do you mean the reviewer can also approve the patch?
> > >
> > > My view is that a reviewer has a right to "approve" a patch, but they
> > > do not have access to actually push the patch. A maintainer is needed
> > > for that. In instances where a designated maintainer is unavaliable to
> > > do so, another maintainer would be permitted to push the patch.
> > >
> > > In instances where the designated maintainer disagrees with the
> > > reviewer, the patch should not be pushed. However, the same should be
> > > true for a patch where two designated maintainers or two designated
> > > reviewers disagree.
> > >
> > > Best Regards,
> > >
> > > Leif
> > >
> > > > According to
> > https://github.com/tianocore/tianocore.github.io/wiki/Who-we-
> > > are#role-of-a-reviewer, I don’t think "Reviewer takes role 1~4.". (I am
> > confused
> > > here ... So please do correct me if I am wrong.)
> > > > =================
> > > > Role of a Reviewer
> > > > Reviewers help maintainers review code, but don't have push access.
> > > >
> > > > A designated Package Reviewer:
> > > >
> > > > shall be reasonably familiar with the Package (or some modules thereof)
> > > >
> > > > will be copied on the patch discussions,
> > > >
> > > > and/or provides testing or regression testing for the Package (or some
> > > modules thereof), in certain platforms and environments.
> > > > ================
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: announce@edk2.groups.io <announce@edk2.groups.io> On
> > Behalf Of
> > > > > gaoliming
> > > > > Sent: Sunday, September 27, 2020 10:33 AM
> > > > > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
> > Guptha,
> > > > > Soumya K <soumya.k.guptha@intel.com>; announce@edk2.groups.io
> > > > > Cc: lersek@redhat.com; 'Leif Lindholm (Nuvia address)'
> > <leif@nuviainc.com>;
> > > > > Kinney, Michael D <michael.d.kinney@intel.com>; 'Andrew Fish'
> > > > > <afish@apple.com>
> > > > > Subject: [edk2-announce] 回复: [edk2-devel] Tianocore community
> > page on
> > > > > who we are - please review
> > > > >
> > > > > Jiewen:
> > > > >
> > > > >  Now, we have reviewer and maintainer role. Reviewer takes role 1~4.
> > > > > Maintainer takes role 1~7. If the people know edk2 process well, they
> > mostly
> > > > > know edk2 one or more packages (modules). So, they can take
> > Maintainer
> > > role.
> > > > > If the people only focus on the technical review, they can take reviewer
> > > > > role. I would suggest there is at lease one Maintainer for each package.
> > > > > There are more reviewers for each package.
> > > > >
> > > > >
> > > > >
> > > > > Soumya:
> > > > >
> > > > >  Here are my comments.
> > > > >
> > > > >   Guidelines for a Maintainer. Never let a pending request get older
> > than a
> > > > > calendar week. This requirement is too strict to the maintainer or
> > reviewer.
> > > > > The maintainer or reviewer should try to give the response in one week.
> > But,
> > > > > they may not fully review one patch set in one week, es for the feature
> > or
> > > > > the complex change.
> > > > >
> > > > > Role of a Contributor/developer. We need to highlight the role &
> > > > > responsibility for the incompatible change. If the contributor proposes
> > the
> > > > > incompatible change, he needs to coordinate with the impacted platform
> > > > > maintainer and make the agreement who will follow up to update the
> > > impacted
> > > > > platforms before he requests to merge his patch set. The impacted
> > platforms
> > > > > include all ones in Edk2 and Edk2Platforms.
> > > > >
> > > > >
> > > > >
> > > > > Last, this page also needs to include release maintainer Definition and
> > > > > Role. The release maintainer is to create the quarterly stable tag. He
> > takes
> > > > > the role to collect the feature planning for each stable tag, schedule the
> > > > > release date, and create the stable tag with the release notes on tag
> > page.
> > > > > He will also send the announcement of soft feature freeze, hard feature
> > > > > freeze and the stable tag completement to edk2 community.
> > > > >
> > > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > Liming
> > > > >
> > > > > 发件人: bounce+27952+65655+4905953+8761045@groups.io
> > > > > <bounce+27952+65655+4905953+8761045@groups.io> 代表 Yao,
> > Jiewen
> > > > > 发送时间: 2020年9月26日 13:33
> > > > > 收件人: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
> > > Guptha,
> > > > > Soumya K <soumya.k.guptha@intel.com>; announce@edk2.groups.io
> > > > > 主题: Re: [edk2-devel] Tianocore community page on who we are -
> > please
> > > > > review
> > > > >
> > > > >
> > > > >
> > > > > Some other thought is about maintainer’s role definition:
> > > > >
> > > > >
> > > > >
> > > > > The role of a maintainer is to:
> > > > >
> > > > > 1.	Maintainer assignments to packages and source file name patterns
> > are
> > > > > provided in the "
> > > > > <https://github.com/tianocore/edk2/blob/master/Maintainers.txt>
> > > Maintainers.
> > > > > txt" file.
> > > > > 2.	Subscribe to the "edk2-bugs" mailing list
> > > > > <https://edk2.groups.io/g/bugs> https://edk2.groups.io/g/bugs, which
> > > > > propagates TianoCore Bugzilla  <https://bugzilla.tianocore.org/>
> > > > > https://bugzilla.tianocore.org/ actions via email. Keep a close eye on
> > new
> > > > > issues reported for their assigned packages. Participate in triaging and
> > > > > analyzing bugs filed for their assigned packages.
> > > > > 3.	Responsible for reviewing patches and answering questions from
> > > > > contributors, on the edk2-devel mailing list
> > > > > <https://edk2.groups.io/g/devel/> https://edk2.groups.io/g/devel/.
> > > > > 4.	Responsible for coordinating patch review with co-maintainers and
> > > > > reviewers of the same package.
> > > > > 5.	Has push / merge access to the merge branch.
> > > > > 6.	Responsible for merging approved patches into the master branch.
> > > > > 7.	Follow the EDK II development
> > > > > <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > > Development-Pr
> > > > > ocess> process.
> > > > >
> > > > >
> > > > >
> > > > > IMHO, the 1~4 need technical expertise, while 5~7 need process
> > expertise.
> > > > >
> > > > > Logically, the can be two separated roles and be done by two different
> > > > > persons.
> > > > >
> > > > > A people who has strong technical expertise might NOT be the best
> > person
> > > to
> > > > > do the integration, and vice versa. I hope we can let right person do right
> > > > > thing in right way.
> > > > >
> > > > > For example, to avoid mistake during check in, 5~7 can be done by a role
> > > > > named “integrator”.
> > > > >
> > > > >
> > > > >
> > > > > My dream is that check-in process is just one click button. But it seems
> > we
> > > > > are still far from it…
> > > > >
> > > > >
> > > > >
> > > > > My two cents.
> > > > >
> > > > >
> > > > >
> > > > > Thank you
> > > > >
> > > > > Yao Jiewen
> > > > >
> > > > >
> > > > >
> > > > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io> > On Behalf Of
> > Yao,
> > > > > Jiewen
> > > > > Sent: Saturday, September 26, 2020 1:09 PM
> > > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io> ; Guptha,
> > > Soumya K
> > > > > <soumya.k.guptha@intel.com <mailto:soumya.k.guptha@intel.com> >;
> > > > > announce@edk2.groups.io <mailto:announce@edk2.groups.io>
> > > > > Subject: Re: [edk2-devel] Tianocore community page on who we are -
> > please
> > > > > review
> > > > >
> > > > >
> > > > >
> > > > > Thanks Soumya. I think this is a good start.
> > > > >
> > > > >
> > > > >
> > > > > Recently we are discussing the maintainer’s work in EDKII mailing list,
> > > > > with title “more development process failure”.
> > > > >
> > > > >
> > > > >
> > > > > I feel the process mentioned in
> > > > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > > Development-Pro
> > > > > cess is not clear enough to follow, especially for the maintainer who is
> > not
> > > > > full time working on EDKII.
> > > > >
> > > > >
> > > > >
> > > > > I wish we can have this opportunity to revisit the “Follow the EDK II
> > > > > development
> > > > > <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > > Development-Pr
> > > > > ocess> process” and make “the process” simpler and clearer.
> > > > >
> > > > >
> > > > >
> > > > > Then all maintainers can sign to follow one rule. The rule we define and
> > the
> > > > > rule we agree with.
> > > > >
> > > > >
> > > > >
> > > > > Thank you
> > > > >
> > > > > Yao Jiewen
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io> > On Behalf Of
> > > Soumya
> > > > > Guptha
> > > > > Sent: Saturday, September 26, 2020 6:35 AM
> > > > > To: announce@edk2.groups.io <mailto:announce@edk2.groups.io> ;
> > > > > devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > > > Subject: [edk2-devel] Tianocore community page on who we are -
> > please
> > > review
> > > > >
> > > > >
> > > > >
> > > > > Dear Community members,
> > > > >
> > > > >
> > > > >
> > > > > I have drafted a document “who we are”, explaining Tianocore
> > community
> > > > > structure, members of the community, their role and the current
> > > development
> > > > > process. I have drafted this document with the help of the Tianocore
> > > > > Stewards.
> > > > >
> > > > > We view this as a living document, as our development processes evolve,
> > I
> > > > > will keep this document updated.
> > > > >
> > > > >
> > > > >
> > > > > Please review the draft version of the document (link below) and provide
> > > > > your feedback. Please send it to me, no need to reply all.
> > > > >
> > > > > I appreciate your input by Friday, Oct 2. After this, I plan on make it live
> > > > > on our TianoCore wiki site.
> > > > >
> > > > >
> > > > >
> > > > > Link:
> > https://github.com/tianocore/tianocore.github.io/wiki/Who-we-are
> > > > >
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Soumya
> > > > >
> > > > >
> > > > >
> > > > > Soumya Guptha
> > > > > TianoCore Community Manager
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > 
> > 
> > 
> > 
> 
> 
> 
> 
> 
> 
> 
>