public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
@ 2023-03-15 20:02 Michael D Kinney
  2023-03-15 22:24 ` [edk2-devel] " Marvin Häuser
  0 siblings, 1 reply; 17+ messages in thread
From: Michael D Kinney @ 2023-03-15 20:02 UTC (permalink / raw)
  To: devel@edk2.groups.io; +Cc: Kinney, Michael D

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

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
@ 2023-03-17 12:32 Marvin Häuser
  2023-03-17 12:57 ` Rebecca Cran
  2023-03-17 13:44 ` Gerd Hoffmann
  0 siblings, 2 replies; 17+ messages in thread
From: Marvin Häuser @ 2023-03-17 12:32 UTC (permalink / raw)
  To: Rebecca Cran; +Cc: Gerd Hoffmann, edk2-devel-groups-io, Michael Kinney

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

Hi Rebecca and Gerd,

Replying to 2 mails at once...

> On 17. Mar 2023, at 11:36, Rebecca Cran <rebecca@bsdio.com> wrote:
> 
> 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).

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

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

I have no experience with things like large-scale patch set review in, say, projects like the Linux kernel. However, in about 7 years of watching edk2-devel 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 commenting, I cannot easily see more context to the changed lines, and I cannot easily see the end result after all patches in a series have been applied. These are 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 sharing? :)

>>  (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/acidanthera/audk/pull/23
Note the review comments that say "Outdated" and thus refer to previous revisions of the PR. I can expand the comments to read their content, see their immediate 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 snapshot that was reviewed. What is missing?
(I’m not sure whether the old stuff isn’t eventually wiped, though, 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.

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

That would at least be a lot better than what we have now.

Best regards,
Marvin

>> 
>> take care,
>>   Gerd


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-03-17 15:37 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-15 20:02 [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label Michael D Kinney
2023-03-15 22:24 ` [edk2-devel] " Marvin Häuser
2023-03-15 22:34   ` Michael D Kinney
2023-03-16 19:54     ` Marvin Häuser
2023-03-16 19:59     ` Rebecca Cran
2023-03-17  9:33       ` Gerd Hoffmann
2023-03-17 10:36         ` Rebecca Cran
2023-03-17 10:48         ` Rebecca Cran
     [not found]         ` <174D2F2BAAB7643C.10271@groups.io>
2023-03-17 10:57           ` Rebecca Cran
2023-03-17 15:27             ` Michael D Kinney
  -- strict thread matches above, loose matches on Subject: below --
2023-03-17 12:32 Marvin Häuser
2023-03-17 12:57 ` Rebecca Cran
2023-03-17 13:44 ` Gerd Hoffmann
2023-03-17 14:08   ` Marvin Häuser
2023-03-17 14:20     ` Rebecca Cran
2023-03-17 15:37       ` Michael D Kinney
2023-03-17 14:22   ` Rebecca Cran

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox