[edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Michael D Kinney]

This is a proposal to enable the GitHub PR feature on the edk2-platforms
repository and enable branch protections that would require maintainers
to set a 'push' label to merge change into edk2-platforms.  The same
process that is already in place on the edk2 repository.

The initial change would not have any CI checks enabled.  Plans to enable
CI on the edk2-platforms repository are being evaluated and will be 
communicated before they are enabled.

Please provide feedback on this proposal by 3/24/2023.

Thanks,

Mike

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

Hi Mike,

Could this be extended to allow for a full PR workflow, if the package maintainers would prefer so? We would like to utilise this for Ext4Pkg. It could be considered a trial. :)

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 236 bytes --]

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

Hi Marvin,

One of the long standing requirements for tianocore is to have a history of all patch series and all
code review activity archived in the mailing list.

Adopting the full PR workflow right now for even a portion of edk2-platforms would have a gap in the
history of patch series and review comments.

edk2-staging repo branches can choose to use this style if they want, but when content is migrated from
edk2-staging into main repos, we want the email archive for that activity.

Mike

From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Wednesday, March 15, 2023 3:24 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

Hi Mike,

Could this be extended to allow for a full PR workflow, if the package maintainers would prefer so? We would like to utilise this for Ext4Pkg. It could be considered a trial. :)

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 41127 bytes --]

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 143 bytes --]

Well, in this form, it complicates our workflow and adds no value. NACK from Pedro and me till there at least is CI.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 155 bytes --]

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Rebecca Cran]

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 requirement to have emails of 
everything.


-- 

Rebecca Cran


On 3/15/23 4:34 PM, Michael D Kinney wrote:
>
> Hi Marvin,
>
> One of the long standing requirements for tianocore is to have a 
> history of all patch series and all
>
> code review activity archived in the mailing list.
>
> Adopting the full PR workflow right now for even a portion of 
> edk2-platforms would have a gap in the
>
> history of patch series and review comments.
>
> edk2-staging repo branches can choose to use this style if they want, 
> but when content is migrated from
>
> edk2-staging into main repos, we want the email archive for that activity.
>
> Mike
>
> *From:*Marvin Häuser <mhaeuser@posteo.de>
> *Sent:* Wednesday, March 15, 2023 3:24 PM
> *To:* Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> *Subject:* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, 
> protected branches, and 'push' label
>
> Hi Mike,
>
> Could this be extended to allow for a full PR workflow, if the package 
> maintainers would prefer so? We would like to utilise this for 
> Ext4Pkg. It could be considered a trial. :)
>
> Best regards,
> Marvin
>
> _._,_._,_
> ------------------------------------------------------------------------
>

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Gerd Hoffmann]

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?
> 
> 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:

 (1) In my experience reviewing patches, especially more complex ones,
     works better in email than in github PR workflows.
 (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.
 (3) Search engines seem to be better in indexing mail list archives
     than github pull requests.

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:

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

take care,
  Gerd

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Rebecca Cran]

I like that proposed workflow.

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

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 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 wrote:
>> 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 requirement 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 github PR workflows.
>   (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.
>   (3) Search engines seem to be better in indexing mail list archives
>       than github pull requests.
>
> 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:
>
>   * 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.
>
> take care,
>    Gerd
>

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Rebecca Cran]

Talking about mailing lists, I'm still disappointed that we lost so much 
history of discussion and reviews around the project when the edk2-devel 
archive at lists.01.org was deleted.

I've sometimes wanted to go back and take a look at the review history 
of a certain commit only to find it's been lost.


-- 
Rebecca Cran


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?
>>
>> 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:
>
>   (1) In my experience reviewing patches, especially more complex ones,
>       works better in email than in github PR workflows.
>   (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.
>   (3) Search engines seem to be better in indexing mail list archives
>       than github pull requests.
>
> 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:
>
>   * 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.
>
> take care,
>    Gerd
>

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Rebecca Cran]

Sorry, it might be the sourceforge mailing list that got lost, not 
lists.01.org. I was wanting to see the review of the following commit, 
but Google isn't finding anything:


commit a61331e8b78ba264f0ccd011b6dc5b9e809730a5
Author: Liming Gao <liming.gao@intel.com>
Date:   Mon Aug 22 14:32:23 2016 +0800

     BaseTools GnuMakefile: Update GCC Flags to the specific one with 
BUILD_ prefix



On 3/17/23 4:48 AM, Rebecca Cran wrote:
> Talking about mailing lists, I'm still disappointed that we lost so 
> much history of discussion and reviews around the project when the 
> edk2-devel archive at lists.01.org was deleted.
>
> I've sometimes wanted to go back and take a look at the review history 
> of a certain commit only to find it's been lost.
>
>

Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label

[Michael D Kinney]

The full lists.01.org history was imported into groups.io.

Stephano did a great job working with groups.io to make that happen.

Mike

> -----Original Message-----
> From: Rebecca Cran <rebecca@bsdio.com>
> Sent: Friday, March 17, 2023 3:58 AM
> To: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Marvin Häuser <mhaeuser@posteo.de>
> Subject: Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
> 
> Sorry, it might be the sourceforge mailing list that got lost, not
> lists.01.org. I was wanting to see the review of the following commit,
> but Google isn't finding anything:
> 
> 
> commit a61331e8b78ba264f0ccd011b6dc5b9e809730a5
> Author: Liming Gao <liming.gao@intel.com>
> Date:   Mon Aug 22 14:32:23 2016 +0800
> 
>      BaseTools GnuMakefile: Update GCC Flags to the specific one with
> BUILD_ prefix
> 
> 
> 
> On 3/17/23 4:48 AM, Rebecca Cran wrote:
> > Talking about mailing lists, I'm still disappointed that we lost so
> > much history of discussion and reviews around the project when the
> > edk2-devel archive at lists.01.org was deleted.
> >
> > I've sometimes wanted to go back and take a look at the review history
> > of a certain commit only to find it's been lost.
> >
> >