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....@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
