Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

["Michael D Kinney" <michael.d.kinney@intel.com>]

> -----Original Message-----
> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Brian J.
> Johnson
> Sent: Thursday, May 2, 2024 8:59 AM
> To: devel@edk2.groups.io; dionnaglaze@google.com;
> quic_llindhol@quicinc.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; rfc@edk2.groups.io;
> Andrew Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> On 5/1/24 18:19, Dionna Glaze via groups.io wrote:
> > On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via groups.io
> > <quic_llindhol=quicinc.com@groups.io> wrote:
> >>
> >> On 2024-05-01 18:43, Michael D Kinney wrote:
> >>> Hello,
> >>>
> >>> I would like to propose that TianoCore move all code review from
> email
> >>> based code reviews to GitHub Pull Requests based code reviews.
> >>>
> >>> The proposed date to switch would be immediately after the next
> stable
> >>> tag which is currently scheduled for May 24, 2024.
> >>
> >> Thanks Mike.
> >>
> >> I'm in favour of this change, and the date.
> >>
> >> I still want us to try to figure out how to retain review history
> beyond
> >> what github decides we need, but I don't think it justifies
> indefinitely
> >> delaying the switchover. And frankly, it will be easier to experiment
> >> with what works and not after the switch.
> >
> > +1. UI-based interactions don't export well for archival-permalinking
> > reasons, and Github archive behavior is for repositories only, not the
> > reviews.
> > But yes, wouldn't want to delay for a bot to echo conversations to
> > devel@edk2.groups.io or some other solution.
> >
> 
> +1 from me as well.  We need to maintain review history in some fairly
> permanent manner, both the reviewed code and review comments.
> 
> >>
> >> /
> >>       Leif
> >>
> >>> Updates to the following Wiki page would be required to describe the
> >>> required process when using GitHub Pull Requests for all code review
> >>> related activity.
> >>>
> >>>       https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process
> >>>
> >>> A couple examples of the changes that would need to be documented
> are:
> >>>
> >>> * All contributors, maintainers, and reviewers must have GitHub IDs.
> >
> > It looks like this is already resolved for the existing
> > Maintainers.txt with the `[username]` syntax, but I don't see any
> > explanation of that expectation. Seems fine to me.
> >
> >>> * The commit message would no longer require Cc:, Reviewed-by:,
> Acked-by:
> >>>     or Tested-by: tags.  The only required tag would be Signed-off-
> by.
> 
> Would those tags be optional?  Test and ack info can be helpful when
> researching a change, to find people who may be knowledgeable about it.
> 
> Similarly, the Reviewed-by info is nice to have in the history, without
> having to dig it out of archives.  But it's a bit awkward to add on
> github:  you have to push new commits with the Reviewed-by tags, but
> that changes the SHAs, so it's not obvious that the commits you're
> merging have the same code as the ones which were reviewed.  For the
> email flow, we trust maintainers to get this right.  For the github
> flow, are we deciding to rely exclusively on the PR archives?
> 
> What if a maintainer decides to tweak a commit before merging it, eg. to
> fix a trivial typo?  With the email flow they just go ahead and do it.
> With the github flow, would they need to post another PR, so it could
> make it through the process and be merged?
> 

Editing commit messages after reviews is something we want to avoid so
the PR does not need to be re-reviewed to be merged.  GitHub has history
of who did the reviews and the conversations that can include information
on testing and review activity.

Yes.  Even trivial changes to the code would require the PR to be updated
and re-reviewed.  It is a different process than email where we allowed
maintainers to make minor changes.  However, even a maintainer making a 
minor change could break things in unexpected ways.  It seems better to
have stricter control over changes and all changes are reviewed and go 
through CI.

> >>> * The Pull Request submitter is required to invite the required
> >>>     maintainers and reviewers to the pull request. This is the same
> >>>     set of maintainers and reviewers that are required to be listed
> in
> >>>     Cc: tags in today's process.
> >
> > This is not configured on tianocore/edk2 at the moment. I have no way
> > to invite a reviewer. Is this a planned fix?
> >
> >>> * Maintainers are responsible for verifying that all conversations
> in
> >>>     the code review are resolved and that all review approvals from
> the
> >>>     required set of maintainers are present before setting the
> 'push' label.
> 
> Will there be documentation on how to use the conversation resolution
> feature?  It's unclear to me whether the PR owner or the reviewer is
> responsible for marking a conversation "resolved."

Please help with proposals for the updated documentation that covers 
this topic.

> 
> >>>
> >>>
> >>> Please provide feedback
> >>> 1) If you are not in favor of this change.
> >>> 2) If you are not in favor of the proposed date of this change.
> >>> 3) On the process changes you would like to see documented in the
> Wiki
> >>>      pages related to using GitHub Pull Request based code reviews.
> >>>
> >>> There is some prototype work to automate/simplify some of the PR
> based
> >>> code review process steps. Those could be added over time as
> resources
> >>> are available to finish and support them.
> >>>
> >>> Best regards,
> >>>
> >>> Mike
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >
> 
> --
> 
>                                                  Brian
> 
> --------------------------------------------------------------------
> 
>    "The answers we have found only serve to raise a whole set of new
>     questions.  In some ways we feel we are as confused as ever, but we
>     are confused on a higher level and about more important things."
>                                             -- Anonymous
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118527): https://edk2.groups.io/g/devel/message/118527
Mute This Topic: https://groups.io/mt/105871305/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-