From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.17885.1679056338478210276 for ; Fri, 17 Mar 2023 05:32:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=NCQei7yT; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id A765E240466 for ; Fri, 17 Mar 2023 13:32:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1679056336; bh=zlK4XqKuUNn6i6SmKN+MYwnt3yfumPXVmj+2+T3OmpU=; h=From:Subject:Date:Cc:To:From; b=NCQei7yTR4pi1FaADJBua7qWL8abSDekx3W7i9PLULXihKtQAorhMOF3mJd6pMHBs MKLfHpOmx0q7tmCUTL1pp1QP46f3/nkRpiwErDaHJWMDZqg2WCLOkr0muchMEwdckp 0DjgdNC+zrEgkQKPM83nq53gzqrHyOQZn5UQ53FR6gPbi3uT3g7fKXvzQzVtn8t6Jn azxppDXFYkfXbgYznHNllnsYz6VokIoDx2Xe6uQgsLqZSbBfcDxYYvjXo1vITbAI7z ntva3iW/cxjLGNODi1CIWFB9LoiQ+R/TFJLcpm29jv2LirO5k4TAlVuT/fcl4YEQyb HQZUD9kZ7G5BQ== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PdNmv3Skwz6trh; Fri, 17 Mar 2023 13:32:15 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label Message-Id: <7D4DC093-E77D-469C-A1C1-E2BB6B12DE0C@posteo.de> Date: Fri, 17 Mar 2023 12:32:15 +0000 Cc: Gerd Hoffmann , edk2-devel-groups-io , Michael Kinney To: Rebecca Cran Content-Type: multipart/alternative; boundary=Apple-Mail-C9FD301A-FC55-428C-9C3F-6A69AC06ACE5 Content-Transfer-Encoding: 7bit --Apple-Mail-C9FD301A-FC55-428C-9C3F-6A69AC06ACE5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable =EF=BB=BFHi Rebecca and Gerd, Replying to 2 mails at once... > On 17. Mar 2023, at 11:36, Rebecca Cran wrote: >=20 > I like that proposed workflow. >=20 > I've also been wondering if we could consider choosing a different product= for patch reviews that supports our desired workflow better, such as Gitlab= or Phorge (the new Phabricator project). I'd be very cautious with suggesting / approving more tooling. It gets more c= onfusing (what is hosted where), it gets more complicated to maintain (who h= osts what and is "guaranteed" to be available to fix things), and so on. >=20 > If anyone would be willing to donate money for colocation, I'd be happy to= move one or more of my servers into a datacenter and use them for hosting T= ianoCore services. >=20 >=20 > --=20 >=20 > Rebecca Cran >=20 >=20 > On 3/17/23 3:33 AM, Gerd Hoffmann wrote: >> On Thu, Mar 16, 2023 at 01:59:49PM -0600, Rebecca Cran wrote: >>> Is this still a requirement since Laszlo's departure from the project? >>>=20 >>> I seem to recall it was him who made it a sticking point of moving to a >>> GitHub PR workflow originally with the requirement to have emails of >>> everything. >> I think it is very useful to have everything on the mailing list for >> a number of reasons: >>=20 >> (1) In my experience reviewing patches, especially more complex ones, >> works better in email than in github PR workflows. I have no experience with things like large-scale patch set review in, say, p= rojects like the Linux kernel. However, in about 7 years of watching edk2-de= vel and opportunistically participating in patch review myself, I never once= encountered something about mail patch review that made me think "oh, that'= s neat". Quite the opposite - I cannot easily cross-reference when commentin= g, I cannot easily see more context to the changed lines, and I cannot easil= y see the end result after all patches in a series have been applied. These a= re all things that GitHub allows me to do. I keep hearing mail patches "work= better", but I never found convincing reasons for these claims. Mind sharin= g? :) >> (2) github doesn't preserve stuff like a mail archive does. When a >> patch series goes through multiple revision github only preserves >> the latest revision which was actually merged. Is that so? Taking this AUDK PR as an example: https://github.com/acidanther= a/audk/pull/23 Note the review comments that say "Outdated" and thus refer to previous revi= sions of the PR. I can expand the comments to read their content, see their i= mmediate context (of the previous change that did not end up getting merged)= and I can click the file name to get an "all files changed" view of the sna= pshot that was reviewed. What is missing? (I=E2=80=99m not sure whether the old stuff isn=E2=80=99t eventually wiped, t= hough, maybe worth carefully inspecting the documentation for options). >> (3) Search engines seem to be better in indexing mail list archives >> than github pull requests. That, unfortunately, undoubtably is true. >>=20 >> Nevertheless I see some room for improvement in our current workflow. >> Developers often open a PR anyway for to run the CI. So maybe we could >> automate sending the emails and also avoid running CI twice by avoiding >> both developer and maintainer opening a PR, with a workflow like this: >>=20 >> * developer opens a draft PR to run CI for the patch series. >> * when the series passes CI and is ready un-draft the PR. >> * github action sends the patch series to the edk2-devel list >> for review (maybe only after CI passed ...). >> * patch review happens on the list. >> * in case the developer pushes updates to the branch in response to >> review comments the github action posts v2/v3 of the series too. >> * once review is done merge the PR. That would at least be a lot better than what we have now. Best regards, Marvin >>=20 >> take care, >> Gerd --Apple-Mail-C9FD301A-FC55-428C-9C3F-6A69AC06ACE5 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
=EF=BB=BFHi Rebecca and Ge= rd,

Replying to 2 mails at once...

On 17. Mar 2023, at 11:36, Rebecca Cran <rebecca@bsdio.com> wrote:<= /div>
I like that proposed w= orkflow.

I've also been wondering if we could consider choosing a dif= ferent product for patch reviews that supports our desired workflow better, s= uch as Gitlab or Phorge (the new Phabricator project).

I'd be ve= ry cautious with suggesting / approving more tooling. It gets more confusing= (what is hosted where), it gets more complicated to maintain (who hosts wha= t and is "guaranteed" to be available to fix things), and so on.


If anyone would be willing to donate m= oney for colocation, I'd be happy to move one or more of my servers into a d= atacenter and use them for hosting TianoCore services.


--
Rebecca Cran


On 3/17/23 3:33 AM, Gerd Hoffmann wrote:
On Thu, Mar 16, 2023 at 01:59:49PM -0600, Rebecca Cran wr= ote:
Is this still a requirement since Laszlo's= departure from the project?

I seem to recall it was him who made it a= sticking point of moving to a
GitHub PR workflow originally with the req= uirement to have emails of
everything.
I think it is very= useful to have everything on the mailing list for
a number of reasons:
 (1) In my experience reviewing patches, especially more complex= ones,
     works better in email than in githu= b PR workflows.

I have no experience with things li= ke large-scale patch set review in, say, projects like the Linux kernel. How= ever, in about 7 years of watching edk2-devel and opportunistically particip= ating in patch review myself, I never once encountered something about mail p= atch review that made me think "oh, that's neat". Quite the opposite - I can= not easily cross-reference when commenting, I cannot easily see more context= to the changed lines, and I cannot easily see the end result after all patc= hes in a series have been applied. These are all things that GitHub allows m= e to do. I keep hearing mail patches "work better", but I never found convin= cing reasons for these claims. Mind sharing? :)

 (2) github doesn't preserv= e stuff like a mail archive does.  When a
    &= nbsp;patch series goes through multiple revision github only preserves
&= nbsp;    the latest revision which was actually merged.<= br>

Is that so? Taking this AUDK PR as an example: = ;https://github.com/= acidanthera/audk/pull/23
Note the review comments that say "Outdated" and thus r= efer to previous revisions of the PR. I can expand the comments to read thei= r content, see their immediate context (of the previous change that did not e= nd up getting merged) and I can click the file name to get an "all files cha= nged" view of the snapshot that was reviewed. What is missing?
(I=E2=80=99m not sure= whether the old stuff isn=E2=80=99t eventually wiped, though, maybe worth c= arefully inspecting the documentation for options).

 (3) Search engines see= m to be better in indexing mail list archives
    &n= bsp;than github pull requests.

That, unfortunately,= undoubtably is true.


Nevertheless I see some room for improvement in our cur= rent workflow.
Developers often open a PR anyway for to run the CI.  = ;So maybe we could
automate sending the emails and also avoid running CI t= wice by avoiding
both developer and maintainer opening a PR, with a workf= low like this:

 * developer opens a draft PR to run CI for the p= atch series.
 * when the series passes CI and is ready un-draft the= PR.
 * github action sends the patch series to the edk2-devel list=
   for review (maybe only after CI passed ...).
&nbs= p;* patch review happens on the list.
 * in case the developer push= es updates to the branch in response to
   review comment= s the github action posts v2/v3 of the series too.
 * once review i= s done merge the PR.

That would at least be a lot b= etter than what we have now.

Best regards,
Marvin

=

take care,
  Gerd


= --Apple-Mail-C9FD301A-FC55-428C-9C3F-6A69AC06ACE5--