* [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-15 20:02 [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label Michael D Kinney
@ 2023-03-15 22:24 ` Marvin Häuser
2023-03-15 22:34 ` Michael D Kinney
0 siblings, 1 reply; 17+ messages in thread
From: Marvin Häuser @ 2023-03-15 22:24 UTC (permalink / raw)
To: Michael D Kinney, devel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
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
0 siblings, 2 replies; 17+ messages in thread
From: Michael D Kinney @ 2023-03-15 22:34 UTC (permalink / raw)
To: Marvin Häuser, devel@edk2.groups.io; +Cc: Kinney, Michael D
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
2023-03-15 22:34 ` Michael D Kinney
@ 2023-03-16 19:54 ` Marvin Häuser
2023-03-16 19:59 ` Rebecca Cran
1 sibling, 0 replies; 17+ messages in thread
From: Marvin Häuser @ 2023-03-16 19:54 UTC (permalink / raw)
To: Michael D Kinney, devel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
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
1 sibling, 1 reply; 17+ messages in thread
From: Rebecca Cran @ 2023-03-16 19:59 UTC (permalink / raw)
To: devel, michael.d.kinney, Marvin Häuser
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
>
> _._,_._,_
> ------------------------------------------------------------------------
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
2023-03-16 19:59 ` Rebecca Cran
@ 2023-03-17 9:33 ` Gerd Hoffmann
2023-03-17 10:36 ` Rebecca Cran
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Gerd Hoffmann @ 2023-03-17 9:33 UTC (permalink / raw)
To: devel, rebecca; +Cc: michael.d.kinney, Marvin Häuser
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
^ 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 9:33 ` Gerd Hoffmann
@ 2023-03-17 10:36 ` Rebecca Cran
2023-03-17 10:48 ` Rebecca Cran
[not found] ` <174D2F2BAAB7643C.10271@groups.io>
2 siblings, 0 replies; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 10:36 UTC (permalink / raw)
To: Gerd Hoffmann, devel; +Cc: michael.d.kinney, Marvin Häuser
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
>
^ 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 9:33 ` Gerd Hoffmann
2023-03-17 10:36 ` Rebecca Cran
@ 2023-03-17 10:48 ` Rebecca Cran
[not found] ` <174D2F2BAAB7643C.10271@groups.io>
2 siblings, 0 replies; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 10:48 UTC (permalink / raw)
To: Gerd Hoffmann, devel; +Cc: michael.d.kinney, Marvin Häuser
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <174D2F2BAAB7643C.10271@groups.io>]
* Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
[not found] ` <174D2F2BAAB7643C.10271@groups.io>
@ 2023-03-17 10:57 ` Rebecca Cran
2023-03-17 15:27 ` Michael D Kinney
0 siblings, 1 reply; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 10:57 UTC (permalink / raw)
To: Gerd Hoffmann, devel; +Cc: michael.d.kinney, Marvin Häuser
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.
>
>
^ 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 10:57 ` Rebecca Cran
@ 2023-03-17 15:27 ` Michael D Kinney
0 siblings, 0 replies; 17+ messages in thread
From: Michael D Kinney @ 2023-03-17 15:27 UTC (permalink / raw)
To: devel@edk2.groups.io, rebecca@bsdio.com, Gerd Hoffmann
Cc: Marvin Häuser, Kinney, Michael D
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.
> >
> >
^ 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
* 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
1 sibling, 0 replies; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 12:57 UTC (permalink / raw)
To: Marvin Häuser; +Cc: Gerd Hoffmann, edk2-devel-groups-io, Michael Kinney
On 3/17/23 6:32 AM, Marvin Häuser wrote:
>
>> 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.
That's easily solved by good documentation.
If we had a server we hosted ourselves, we'd at least solve one source
of confusion: today, our Bugzilla instance is hosted by "Advanced
Computer Graphics" (bugzilla.tianocore.org is an alias for
tianocore.acgmultimedia.com.).
--
Rebecca Cran
^ 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
2023-03-17 14:08 ` Marvin Häuser
2023-03-17 14:22 ` Rebecca Cran
1 sibling, 2 replies; 17+ messages in thread
From: Gerd Hoffmann @ 2023-03-17 13:44 UTC (permalink / raw)
To: Marvin Häuser; +Cc: Rebecca Cran, edk2-devel-groups-io, Michael Kinney
On Fri, Mar 17, 2023 at 12:32:15PM +0000, Marvin Häuser wrote:
> 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.
Agree. Also from the web-based review tools I've worked with so far
(not much, only github and gitlab) github is the better one.
> >> (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? :)
(1) Navigation works better for me. On the email side I have the freedom
to pick whatever client I like and can configure it the way I like.
(2) I can easily automate things. For example it's just two key strokes
in the mail client to run a script which creates a new branch and
applies the whole patch series.
The latter is what I usually do when I want compile and test the series,
or when I need something plain email doesn't give me (like getting more
patch context, which indeed is a nice github feature).
> >> (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.
> (I’m not sure whether the old stuff isn’t eventually wiped, though,
> maybe worth carefully inspecting the documentation for options).
Yes, this. For active PRs this usually isn't much of a problem. But
try come back after a few months, or even a few years (see Rebecca
trying to lookup context for a 2016 commit in the archives).
> >> * 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.
While discussing tooling: Can we move from bugzilla to github issues
for bug tracking? That will give us some nice automation and
integration benefits. As far I know the blocker for doing that was
github issues not having a permission system, which is bad for reporting
security bugs. But with security bug reporting and processing using
github security advisories now this point should be moot, no?
The big problem here is what to do with bugzilla. Migrate all bugs
over? Not sure whenever any tooling exists for that already. I suspect
we would not be the first ones trying to do that. Or switch bugzilla
into readonly mode and keep it running that way for archive purposes?
Would have the advantage that all the bugzilla links in the commit
messages continue working.
take care,
Gerd
^ 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 13:44 ` Gerd Hoffmann
@ 2023-03-17 14:08 ` Marvin Häuser
2023-03-17 14:20 ` Rebecca Cran
2023-03-17 14:22 ` Rebecca Cran
1 sibling, 1 reply; 17+ messages in thread
From: Marvin Häuser @ 2023-03-17 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Rebecca Cran, edk2-devel-groups-io, Michael Kinney
> On 17. Mar 2023, at 14:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Mar 17, 2023 at 12:32:15PM +0000, Marvin Häuser wrote:
>> 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.
>
> Agree. Also from the web-based review tools I've worked with so far
> (not much, only github and gitlab) github is the better one.
>
>>>> (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? :)
>
> (1) Navigation works better for me. On the email side I have the freedom
> to pick whatever client I like and can configure it the way I like.
Well, customization is not the kind of point one can argue against. :) OK, thanks.
>
> (2) I can easily automate things. For example it's just two key strokes
> in the mail client to run a script which creates a new branch and
> applies the whole patch series.
>
> The latter is what I usually do when I want compile and test the series,
> or when I need something plain email doesn't give me (like getting more
> patch context, which indeed is a nice github feature).
Right, but I’d say especially with tools like VS Code, just checking out the PR branch (or even opening remotely without downloading it fully) is equally easy. This sounds more like a mitigation for the shortcomings of mail patches.
>
>>>> (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.
>
>> (I’m not sure whether the old stuff isn’t eventually wiped, though,
>> maybe worth carefully inspecting the documentation for options).
>
> Yes, this. For active PRs this usually isn't much of a problem. But
> try come back after a few months, or even a few years (see Rebecca
> trying to lookup context for a 2016 commit in the archives).
I also didn’t find anything in the GutHub docs regarding guarantees for archival. Disgusting.
>
>>>> * 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.
>
> While discussing tooling: Can we move from bugzilla to github issues
> for bug tracking? That will give us some nice automation and
> integration benefits. As far I know the blocker for doing that was
> github issues not having a permission system, which is bad for reporting
> security bugs. But with security bug reporting and processing using
> github security advisories now this point should be moot, no?
Hmm, is there any big problem that requires a solution? While I always like streamlining and centralisation, and I would vote for GitHub Issues if it was about a new bugtracker, I never heard many complaints about bugzilla. Everyone is used to it and there’s a ton of links in the wild. Even if the old service is archived, any currently active ticket would not easily point to its active GitHub Issues counterpart, would it? Just wondering whether it is worth the disruption.
Best regards,
Marvin
>
> The big problem here is what to do with bugzilla. Migrate all bugs
> over? Not sure whenever any tooling exists for that already. I suspect
> we would not be the first ones trying to do that. Or switch bugzilla
> into readonly mode and keep it running that way for archive purposes?
> Would have the advantage that all the bugzilla links in the commit
> messages continue working.
>
> take care,
> Gerd
>
^ 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 14:08 ` Marvin Häuser
@ 2023-03-17 14:20 ` Rebecca Cran
2023-03-17 15:37 ` Michael D Kinney
0 siblings, 1 reply; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 14:20 UTC (permalink / raw)
To: devel, mhaeuser, Gerd Hoffmann; +Cc: Michael Kinney
On 3/17/23 8:08 AM, Marvin Häuser wrote:
>> On 17. Mar 2023, at 14:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> Yes, this. For active PRs this usually isn't much of a problem. But
>> try come back after a few months, or even a few years (see Rebecca
>> trying to lookup context for a 2016 commit in the archives).
> I also didn’t find anything in the GutHub docs regarding guarantees for archival. Disgusting.
There's a PR from npm cli from 2018 that still appears to have all data
about it: https://github.com/npm/cli/pull/1
But, in other places there do seem to be concerns about data retention,
for example
https://github.com/tianocore-docs/edk2-VfrSpecification/actions is empty
and almost certainly had a run in November 2020. I don't know if that's
something we configured, or if it deletes history for GitHub Actions
after a few years.
--
Rebecca Cran
^ 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 14:20 ` Rebecca Cran
@ 2023-03-17 15:37 ` Michael D Kinney
0 siblings, 0 replies; 17+ messages in thread
From: Michael D Kinney @ 2023-03-17 15:37 UTC (permalink / raw)
To: devel@edk2.groups.io, rebecca@bsdio.com, mhaeuser@posteo.de,
Gerd Hoffmann
Cc: Kinney, Michael D
I believe the policies for GitHub Actions artifacts and GitHub PR data retention
are different. Max 90 days for GitHub Actions for public repos:
https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#artifact-and-log-retention-policy
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca Cran
> Sent: Friday, March 17, 2023 7:20 AM
> To: devel@edk2.groups.io; mhaeuser@posteo.de; Gerd Hoffmann <kraxel@redhat.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms] Enable GitHub PR, protected branches, and 'push' label
>
> On 3/17/23 8:08 AM, Marvin Häuser wrote:
> >> On 17. Mar 2023, at 14:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>
> >> Yes, this. For active PRs this usually isn't much of a problem. But
> >> try come back after a few months, or even a few years (see Rebecca
> >> trying to lookup context for a 2016 commit in the archives).
> > I also didn’t find anything in the GutHub docs regarding guarantees for archival. Disgusting.
>
> There's a PR from npm cli from 2018 that still appears to have all data
> about it: https://github.com/npm/cli/pull/1
>
> But, in other places there do seem to be concerns about data retention,
> for example
> https://github.com/tianocore-docs/edk2-VfrSpecification/actions is empty
> and almost certainly had a run in November 2020. I don't know if that's
> something we configured, or if it deletes history for GitHub Actions
> after a few years.
>
>
> --
>
> Rebecca Cran
>
>
>
>
>
^ 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 13:44 ` Gerd Hoffmann
2023-03-17 14:08 ` Marvin Häuser
@ 2023-03-17 14:22 ` Rebecca Cran
1 sibling, 0 replies; 17+ messages in thread
From: Rebecca Cran @ 2023-03-17 14:22 UTC (permalink / raw)
To: Gerd Hoffmann, Marvin Häuser; +Cc: edk2-devel-groups-io, Michael Kinney
On 3/17/23 7:44 AM, Gerd Hoffmann wrote:
> Agree. Also from the web-based review tools I've worked with so far
> (not much, only github and gitlab) github is the better one.
Having used Review Board, Gitlab, Github, Phabricator, Gerrit and
probably others, Gerrit is by far my least favorite.
I haven't enjoyed the GitHub UI compared to Gitlab or Phabricator, but I
can live with it without too much frustration.
--
Rebecca Cran
^ 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