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

["Brian J. Johnson" <brian.johnson@hpe.com>]

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

>>>
>>>
>>> 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 (#118524): https://edk2.groups.io/g/devel/message/118524
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]
-=-=-=-=-=-=-=-=-=-=-=-