On Thu, May 2, 2024 at 8:59 AM Brian J. Johnson <brian.john...@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....@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
