From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 6286CD801DE for ; Thu, 2 May 2024 16:10:04 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=02EuJMKWC/i5O3EKUEdAz93Nl/ujojDcnUGyJt6KREY=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20240206; t=1714666203; v=1; b=5Y/GW/gnph5aCe1YSsH1denYpepS4U0PxrqwPQcPmMvUNwbxhR3Lwdp9vu9DgyUCpKgJROAf t/82er0CuOfBeciZ3pJ+7vTmuIgJZ3KhdxRJ9kvmZ0Ie/tDhj2g2L1CPguWarZPRVZBBsvBATqm eo4vzZS9C0Rzo56L3mnpNsBDixx2AytDFU9BLlWb0naLVa6ThogttOty0Jeb34az/64ONFJvrRT w6JgDZyZG42tLEaiywA3DA2KAS1vLMfHlfbYiSbZaRI7jCIg+hafZePMjoCjOy/03KtExZDNTJY cdDFcY6io6IELAtVXZRVUp/2VwupuM4bMCGp2rdfRyg6Q== X-Received: by 127.0.0.2 with SMTP id Si4WYY7687511xH3nQ2KlgqU; Thu, 02 May 2024 09:10:02 -0700 X-Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by mx.groups.io with SMTP id smtpd.web10.9833.1714666201985511388 for ; Thu, 02 May 2024 09:10:02 -0700 X-Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-5724736770cso11013a12.1 for ; Thu, 02 May 2024 09:10:01 -0700 (PDT) X-Gm-Message-State: m7Ryy6Fk08rPZrogvd2iOweUx7686176AA= X-Google-Smtp-Source: AGHT+IFpuse/wjoJIpd7MsRbTq9MBX8de22TwUE3ut33oVZ4o+4YZ3qlRxuhnrmbHKt8KEf2TTBKAjghcdAfS80BckY= X-Received: by 2002:aa7:d988:0:b0:572:9960:c21 with SMTP id 4fb4d7f45d1cf-572bdc52749mr166142a12.7.1714666200186; Thu, 02 May 2024 09:10:00 -0700 (PDT) MIME-Version: 1.0 References: <076697e5-a594-40a7-8100-767d6a2f7520@quicinc.com> In-Reply-To: From: "Dionna Glaze via groups.io" Date: Thu, 2 May 2024 09:09:48 -0700 Message-ID: Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024 To: "Brian J. Johnson" Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com, michael.d.kinney@intel.com, "rfc@edk2.groups.io" , "Andrew Fish (afish@apple.com)" Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Thu, 02 May 2024 09:10:02 -0700 Resent-From: dionnaglaze@google.com Reply-To: devel@edk2.groups.io,dionnaglaze@google.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b="5Y/GW/gn"; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=pass (policy=none) header.from=groups.io On Thu, May 2, 2024 at 8:59=E2=80=AFAM Brian J. Johnson wrote: > > On 5/1/24 18:19, Dionna Glaze via groups.io wrote: > > On Wed, May 1, 2024 at 11:12=E2=80=AFAM Leif Lindholm via 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 emai= l > >>> based code reviews to GitHub Pull Requests based code reviews. > >>> > >>> The proposed date to switch would be immediately after the next stabl= e > >>> 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 beyo= nd > >> what github decides we need, but I don't think it justifies indefinite= ly > >> 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-De= velopment-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 i= n > >>> 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 t= he > >>> 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 Wik= i > >>> pages related to using GitHub Pull Request based code reviews. > >>> > >>> There is some prototype work to automate/simplify some of the PR base= d > >>> code review process steps. Those could be added over time as resource= s > >>> 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 > --=20 -Dionna Glaze, PhD, CISSP (she/her) -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-