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

["Dionna Glaze via groups.io" <dionnaglaze=google.com@groups.io>]

On Thu, May 2, 2024 at 8:59 AM Brian J. Johnson <brian.johnson@hpe.com> wrote:
>
> 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?
>
> >>> * 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."
>

Github has branch security features that let you _require_ that all
messages are resolved before merging, so that could be turned on.

> >>>
> >>>
> >>> 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
>


-- 
-Dionna Glaze, PhD, CISSP (she/her)


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