[edk2-devel] GitHub PR Code Review process now active

[edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hello,

The GitHub PR code review process is now active.  Please 
use the new PR based code review process for all new 
submissions starting today.

* The Wiki has been updated with the process changes.

  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

  Big thanks to Michael Kubacki for writing up all the 
  changes based on the RFC proposal and community discussions.

  We will learn by using, so if you see anything missing or 
  incorrect or clarifications needed, please send feedback 
  here so the Wiki pages can be updated quickly for everyone.

* The edk2 repo settings have been updated to require
  a GitHub PR code review approval before merging and
  all conversations must be resolved before merging.

* A PR has been opened that removes the requirement for 
  Cc: tags in the commit messages and is the first PR 
  that will use the new process. This PR needs to be 
  reviewed and merged to support the revised commit 
  message format.

  https://github.com/tianocore/edk2/pull/5688

  https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

* Please use "Draft" PRs to run CI without any reviews.
  Once ready for reviews, convert from "Draft" to 
  "Ready for Review".

* For active code reviews that are almost complete and will
  be ready for merge in the next few days, the submitter may
  choose to either complete using the email review process,
  or switch to the PR based code review process.

* For active code reviews that are expected to take more than
  a few days to complete, please convert to a PR based code review.

Thanks,

Mike



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119311): https://edk2.groups.io/g/devel/message/119311
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Gerd Hoffmann]

> The GitHub PR code review process is now active.  Please 
> use the new PR based code review process for all new 
> submissions starting today.
> 
> * The Wiki has been updated with the process changes.
> 
>   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
>   Big thanks to Michael Kubacki for writing up all the 
>   changes based on the RFC proposal and community discussions.
> 
>   We will learn by using, so if you see anything missing or 
>   incorrect or clarifications needed, please send feedback 
>   here so the Wiki pages can be updated quickly for everyone.

$ gh pr edit kraxel:devel/emu-buildfix --add-reviewer niruiyu,ajfish
GraphQL: kraxel does not have the correct permissions to execute `RequestReviews` (requestReviews)

Looks like the permissions need some tweaks ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119314): https://edk2.groups.io/g/devel/message/119314
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hi Gerd,

You are in the EDK II Reviewers team.

The current settings only allow members of the EDK II Maintainers 
team to assign reviewers.

The maintainers of the OvmfPkg are required to add all reviewers to the PR.

I have added niruiyu,ajfish to keep this PR moving while we continue
the process discussion. 

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
> Sent: Tuesday, May 28, 2024 11:39 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> > The GitHub PR code review process is now active.  Please
> > use the new PR based code review process for all new
> > submissions starting today.
> >
> > * The Wiki has been updated with the process changes.
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> Process
> >
> >   Big thanks to Michael Kubacki for writing up all the
> >   changes based on the RFC proposal and community discussions.
> >
> >   We will learn by using, so if you see anything missing or
> >   incorrect or clarifications needed, please send feedback
> >   here so the Wiki pages can be updated quickly for everyone.
> 
> $ gh pr edit kraxel:devel/emu-buildfix --add-reviewer niruiyu,ajfish
> GraphQL: kraxel does not have the correct permissions to execute
> `RequestReviews` (requestReviews)
> 
> Looks like the permissions need some tweaks ...
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119353): https://edk2.groups.io/g/devel/message/119353
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Gerd Hoffmann]

On Wed, May 29, 2024 at 03:00:13PM GMT, Michael D Kinney wrote:
> Hi Gerd,
> 
> You are in the EDK II Reviewers team.
> 
> The current settings only allow members of the EDK II Maintainers 
> team to assign reviewers.

That contradicts the wiki instructions which say I should assign
reviewers myself.  And I think it makes sense to allow that for my
own PRs, at least as long as there is no automatic process which
consults Maintainers.txt and auto-assigns reviewers.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119356): https://edk2.groups.io/g/devel/message/119356
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hi Gerd,

We can clarify the Wiki.  A Maintainer has to be involved in every
code review.  The first action a Maintainer does is verify that the
code change should be considered at all or rejected.  Then add 
maintainers for the ones that look like good submissions.

I agree that this may add a bit of delay.  You are welcome to add
a comment with @<githubid> tags to the maintainers and reviewers so
they know to look at it and do the assignments.  If you like that
idea we can add that to the Wiki as well.

Since PRs can be also opened by outside contributors that have 
limited access, maintainers have to be involved in reviewing newly
submitted PRs.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
> Sent: Wednesday, May 29, 2024 9:38 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> On Wed, May 29, 2024 at 03:00:13PM GMT, Michael D Kinney wrote:
> > Hi Gerd,
> >
> > You are in the EDK II Reviewers team.
> >
> > The current settings only allow members of the EDK II Maintainers
> > team to assign reviewers.
> 
> That contradicts the wiki instructions which say I should assign
> reviewers myself.  And I think it makes sense to allow that for my
> own PRs, at least as long as there is no automatic process which
> consults Maintainers.txt and auto-assigns reviewers.
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119357): https://edk2.groups.io/g/devel/message/119357
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Rebecca Cran]

On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
> Hi Gerd,
>
> We can clarify the Wiki.  A Maintainer has to be involved in every
> code review.  The first action a Maintainer does is verify that the
> code change should be considered at all or rejected.  Then add
> maintainers for the ones that look like good submissions.
>
> I agree that this may add a bit of delay.  You are welcome to add
> a comment with @<githubid> tags to the maintainers and reviewers so
> they know to look at it and do the assignments.  If you like that
> idea we can add that to the Wiki as well.
>
> Since PRs can be also opened by outside contributors that have
> limited access, maintainers have to be involved in reviewing newly
> submitted PRs.

"Then add maintainers for the ones that look like good submissions."

Surely in order for the maintainers to know the PR should be looked at 
in the first place they need to already be added?

-- 

Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119359): https://edk2.groups.io/g/devel/message/119359
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

GitHub has notification settings.  Maintainers should configure
GitHub so they are notified of all PR submissions to edk2 repo.

The Wiki for the Maintainer Process provides a link to this page with the details:

https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications

https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications#about-custom-notifications

This allows Maintainers to receive notifications without being an
assigned reviewer.

Mike

> -----Original Message-----
> From: Rebecca Cran <rebecca@bsdio.com>
> Sent: Wednesday, May 29, 2024 11:19 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
> > Hi Gerd,
> >
> > We can clarify the Wiki.  A Maintainer has to be involved in every
> > code review.  The first action a Maintainer does is verify that the
> > code change should be considered at all or rejected.  Then add
> > maintainers for the ones that look like good submissions.
> >
> > I agree that this may add a bit of delay.  You are welcome to add
> > a comment with @<githubid> tags to the maintainers and reviewers so
> > they know to look at it and do the assignments.  If you like that
> > idea we can add that to the Wiki as well.
> >
> > Since PRs can be also opened by outside contributors that have
> > limited access, maintainers have to be involved in reviewing newly
> > submitted PRs.
> 
> "Then add maintainers for the ones that look like good submissions."
> 
> Surely in order for the maintainers to know the PR should be looked at
> in the first place they need to already be added?
> 
> --
> 
> Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119360): https://edk2.groups.io/g/devel/message/119360
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael Kubacki]

Mike,

I agree that automatically adding reviewers would be helpful.

Do you think we could add a CODEOWNERS file now to assist with this?

Benefits being:

1. CODEOWNERS is low overhead in that GitHub already supports it.

2. We do not need to require CODEOWNER enforcement yet. We can simply 
use it for automating the process of adding maintainers.

3. Maintainers have write access so they are required to approve and add 
the push label. This would add a relevant maintainer(s) with write 
access to fulfill that role.

Thanks,
Michael

On 5/29/2024 2:27 PM, Michael D Kinney wrote:
> GitHub has notification settings.  Maintainers should configure
> GitHub so they are notified of all PR submissions to edk2 repo.
> 
> The Wiki for the Maintainer Process provides a link to this page with the details:
> 
> https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications
> 
> https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications#about-custom-notifications
> 
> This allows Maintainers to receive notifications without being an
> assigned reviewer.
> 
> Mike
> 
>> -----Original Message-----
>> From: Rebecca Cran <rebecca@bsdio.com>
>> Sent: Wednesday, May 29, 2024 11:19 AM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>
>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
>>> Hi Gerd,
>>>
>>> We can clarify the Wiki.  A Maintainer has to be involved in every
>>> code review.  The first action a Maintainer does is verify that the
>>> code change should be considered at all or rejected.  Then add
>>> maintainers for the ones that look like good submissions.
>>>
>>> I agree that this may add a bit of delay.  You are welcome to add
>>> a comment with @<githubid> tags to the maintainers and reviewers so
>>> they know to look at it and do the assignments.  If you like that
>>> idea we can add that to the Wiki as well.
>>>
>>> Since PRs can be also opened by outside contributors that have
>>> limited access, maintainers have to be involved in reviewing newly
>>> submitted PRs.
>>
>> "Then add maintainers for the ones that look like good submissions."
>>
>> Surely in order for the maintainers to know the PR should be looked at
>> in the first place they need to already be added?
>>
>> --
>>
>> Rebecca Cran
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119362): https://edk2.groups.io/g/devel/message/119362
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

We could, but that would require manually syncing CODEOWNERS
with Maintainer.txt until that part of the process is automated.

We also need a way to verify that CODEOWNERS and Maintainers.txt
produce the exact same assignments.

This was on the list of future enhancements when resources are
available to help.  We need to focus on a process that works 
until those automations can be deployed and supported.

Mike

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Wednesday, May 29, 2024 12:25 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael
> <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> Mike,
> 
> I agree that automatically adding reviewers would be helpful.
> 
> Do you think we could add a CODEOWNERS file now to assist with this?
> 
> Benefits being:
> 
> 1. CODEOWNERS is low overhead in that GitHub already supports it.
> 
> 2. We do not need to require CODEOWNER enforcement yet. We can simply
> use it for automating the process of adding maintainers.
> 
> 3. Maintainers have write access so they are required to approve and add
> the push label. This would add a relevant maintainer(s) with write
> access to fulfill that role.
> 
> Thanks,
> Michael
> 
> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
> > GitHub has notification settings.  Maintainers should configure
> > GitHub so they are notified of all PR submissions to edk2 repo.
> >
> > The Wiki for the Maintainer Process provides a link to this page with the
> details:
> >
> > https://docs.github.com/en/account-and-profile/managing-subscriptions-and-
> notifications-on-github/setting-up-notifications/configuring-notifications
> >
> > https://docs.github.com/en/account-and-profile/managing-subscriptions-and-
> notifications-on-github/setting-up-notifications/configuring-
> notifications#about-custom-notifications
> >
> > This allows Maintainers to receive notifications without being an
> > assigned reviewer.
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Rebecca Cran <rebecca@bsdio.com>
> >> Sent: Wednesday, May 29, 2024 11:19 AM
> >> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> >> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> >> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >>
> >> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
> >>> Hi Gerd,
> >>>
> >>> We can clarify the Wiki.  A Maintainer has to be involved in every
> >>> code review.  The first action a Maintainer does is verify that the
> >>> code change should be considered at all or rejected.  Then add
> >>> maintainers for the ones that look like good submissions.
> >>>
> >>> I agree that this may add a bit of delay.  You are welcome to add
> >>> a comment with @<githubid> tags to the maintainers and reviewers so
> >>> they know to look at it and do the assignments.  If you like that
> >>> idea we can add that to the Wiki as well.
> >>>
> >>> Since PRs can be also opened by outside contributors that have
> >>> limited access, maintainers have to be involved in reviewing newly
> >>> submitted PRs.
> >>
> >> "Then add maintainers for the ones that look like good submissions."
> >>
> >> Surely in order for the maintainers to know the PR should be looked at
> >> in the first place they need to already be added?
> >>
> >> --
> >>
> >> Rebecca Cran
> >
> >
> >
> > 
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119365): https://edk2.groups.io/g/devel/message/119365
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael Kubacki]

I've updated the wiki process to clarify that maintainers need to ensure 
PR reviewers are added and that a contributor cannot do so unless they 
are a maintainer in this update:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process/_compare/b9d1f34e40d8ecbc7b59632302c9b0c823f52ab8...dd846b296754da31003f8c2a70e9a0b861e600b6

I did not include the @<github> ID process since there was not clear 
consensus on that. I think it's tedious and for this small window of 
time without automation, ideally the GitHub notifications mentioned 
earlier can be used.

Thanks,
Michael

On 5/29/2024 4:06 PM, Kinney, Michael D wrote:
> We could, but that would require manually syncing CODEOWNERS
> with Maintainer.txt until that part of the process is automated.
> 
> We also need a way to verify that CODEOWNERS and Maintainers.txt
> produce the exact same assignments.
> 
> This was on the list of future enhancements when resources are
> available to help.  We need to focus on a process that works
> until those automations can be deployed and supported.
> 
> Mike
> 
>> -----Original Message-----
>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>> Sent: Wednesday, May 29, 2024 12:25 PM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael
>> <michael.kubacki@microsoft.com>
>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>
>> Mike,
>>
>> I agree that automatically adding reviewers would be helpful.
>>
>> Do you think we could add a CODEOWNERS file now to assist with this?
>>
>> Benefits being:
>>
>> 1. CODEOWNERS is low overhead in that GitHub already supports it.
>>
>> 2. We do not need to require CODEOWNER enforcement yet. We can simply
>> use it for automating the process of adding maintainers.
>>
>> 3. Maintainers have write access so they are required to approve and add
>> the push label. This would add a relevant maintainer(s) with write
>> access to fulfill that role.
>>
>> Thanks,
>> Michael
>>
>> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
>>> GitHub has notification settings.  Maintainers should configure
>>> GitHub so they are notified of all PR submissions to edk2 repo.
>>>
>>> The Wiki for the Maintainer Process provides a link to this page with the
>> details:
>>>
>>> https://docs.github.com/en/account-and-profile/managing-subscriptions-and-
>> notifications-on-github/setting-up-notifications/configuring-notifications
>>>
>>> https://docs.github.com/en/account-and-profile/managing-subscriptions-and-
>> notifications-on-github/setting-up-notifications/configuring-
>> notifications#about-custom-notifications
>>>
>>> This allows Maintainers to receive notifications without being an
>>> assigned reviewer.
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: Rebecca Cran <rebecca@bsdio.com>
>>>> Sent: Wednesday, May 29, 2024 11:19 AM
>>>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>>>
>>>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
>>>>> Hi Gerd,
>>>>>
>>>>> We can clarify the Wiki.  A Maintainer has to be involved in every
>>>>> code review.  The first action a Maintainer does is verify that the
>>>>> code change should be considered at all or rejected.  Then add
>>>>> maintainers for the ones that look like good submissions.
>>>>>
>>>>> I agree that this may add a bit of delay.  You are welcome to add
>>>>> a comment with @<githubid> tags to the maintainers and reviewers so
>>>>> they know to look at it and do the assignments.  If you like that
>>>>> idea we can add that to the Wiki as well.
>>>>>
>>>>> Since PRs can be also opened by outside contributors that have
>>>>> limited access, maintainers have to be involved in reviewing newly
>>>>> submitted PRs.
>>>>
>>>> "Then add maintainers for the ones that look like good submissions."
>>>>
>>>> Surely in order for the maintainers to know the PR should be looked at
>>>> in the first place they need to already be added?
>>>>
>>>> --
>>>>
>>>> Rebecca Cran
>>>
>>>
>>>
>>> 
>>>
>>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119373): https://edk2.groups.io/g/devel/message/119373
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Saloni Kasbekar]

How are we planning to handle packages (like NetworkPkg) without a maintainer? Would the stewards add in the reviewers in that case?

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, May 29, 2024 5:51 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] GitHub PR Code Review process now active

I've updated the wiki process to clarify that maintainers need to ensure PR reviewers are added and that a contributor cannot do so unless they are a maintainer in this update:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process/_compare/b9d1f34e40d8ecbc7b59632302c9b0c823f52ab8...dd846b296754da31003f8c2a70e9a0b861e600b6

I did not include the @<github> ID process since there was not clear consensus on that. I think it's tedious and for this small window of time without automation, ideally the GitHub notifications mentioned earlier can be used.

Thanks,
Michael

On 5/29/2024 4:06 PM, Kinney, Michael D wrote:
> We could, but that would require manually syncing CODEOWNERS with 
> Maintainer.txt until that part of the process is automated.
> 
> We also need a way to verify that CODEOWNERS and Maintainers.txt 
> produce the exact same assignments.
> 
> This was on the list of future enhancements when resources are 
> available to help.  We need to focus on a process that works until 
> those automations can be deployed and supported.
> 
> Mike
> 
>> -----Original Message-----
>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>> Sent: Wednesday, May 29, 2024 12:25 PM
>> To: devel@edk2.groups.io; Kinney, Michael D 
>> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>; 
>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>
>> Mike,
>>
>> I agree that automatically adding reviewers would be helpful.
>>
>> Do you think we could add a CODEOWNERS file now to assist with this?
>>
>> Benefits being:
>>
>> 1. CODEOWNERS is low overhead in that GitHub already supports it.
>>
>> 2. We do not need to require CODEOWNER enforcement yet. We can simply 
>> use it for automating the process of adding maintainers.
>>
>> 3. Maintainers have write access so they are required to approve and 
>> add the push label. This would add a relevant maintainer(s) with 
>> write access to fulfill that role.
>>
>> Thanks,
>> Michael
>>
>> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
>>> GitHub has notification settings.  Maintainers should configure 
>>> GitHub so they are notified of all PR submissions to edk2 repo.
>>>
>>> The Wiki for the Maintainer Process provides a link to this page 
>>> with the
>> details:
>>>
>>> https://docs.github.com/en/account-and-profile/managing-subscription
>>> s-and-
>> notifications-on-github/setting-up-notifications/configuring-notifica
>> tions
>>>
>>> https://docs.github.com/en/account-and-profile/managing-subscription
>>> s-and-
>> notifications-on-github/setting-up-notifications/configuring-
>> notifications#about-custom-notifications
>>>
>>> This allows Maintainers to receive notifications without being an 
>>> assigned reviewer.
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: Rebecca Cran <rebecca@bsdio.com>
>>>> Sent: Wednesday, May 29, 2024 11:19 AM
>>>> To: devel@edk2.groups.io; Kinney, Michael D 
>>>> <michael.d.kinney@intel.com>; kraxel@redhat.com; Kubacki, Michael 
>>>> <michael.kubacki@microsoft.com>
>>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>>>
>>>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
>>>>> Hi Gerd,
>>>>>
>>>>> We can clarify the Wiki.  A Maintainer has to be involved in every 
>>>>> code review.  The first action a Maintainer does is verify that 
>>>>> the code change should be considered at all or rejected.  Then add 
>>>>> maintainers for the ones that look like good submissions.
>>>>>
>>>>> I agree that this may add a bit of delay.  You are welcome to add 
>>>>> a comment with @<githubid> tags to the maintainers and reviewers 
>>>>> so they know to look at it and do the assignments.  If you like 
>>>>> that idea we can add that to the Wiki as well.
>>>>>
>>>>> Since PRs can be also opened by outside contributors that have 
>>>>> limited access, maintainers have to be involved in reviewing newly 
>>>>> submitted PRs.
>>>>
>>>> "Then add maintainers for the ones that look like good submissions."
>>>>
>>>> Surely in order for the maintainers to know the PR should be looked 
>>>> at in the first place they need to already be added?
>>>>
>>>> --
>>>>
>>>> Rebecca Cran
>>>
>>>
>>>
>>> 
>>>
>>>







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119400): https://edk2.groups.io/g/devel/message/119400
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Yes.

Mike

> -----Original Message-----
> From: Kasbekar, Saloni <saloni.kasbekar@intel.com>
> Sent: Thursday, May 30, 2024 10:39 AM
> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> How are we planning to handle packages (like NetworkPkg) without a
> maintainer? Would the stewards add in the reviewers in that case?
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Wednesday, May 29, 2024 5:51 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
> Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael
> <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> I've updated the wiki process to clarify that maintainers need to ensure PR
> reviewers are added and that a contributor cannot do so unless they are a
> maintainer in this update:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> Process/_compare/b9d1f34e40d8ecbc7b59632302c9b0c823f52ab8...dd846b296754da310
> 03f8c2a70e9a0b861e600b6
> 
> I did not include the @<github> ID process since there was not clear
> consensus on that. I think it's tedious and for this small window of time
> without automation, ideally the GitHub notifications mentioned earlier can be
> used.
> 
> Thanks,
> Michael
> 
> On 5/29/2024 4:06 PM, Kinney, Michael D wrote:
> > We could, but that would require manually syncing CODEOWNERS with
> > Maintainer.txt until that part of the process is automated.
> >
> > We also need a way to verify that CODEOWNERS and Maintainers.txt
> > produce the exact same assignments.
> >
> > This was on the list of future enhancements when resources are
> > available to help.  We need to focus on a process that works until
> > those automations can be deployed and supported.
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> >> Sent: Wednesday, May 29, 2024 12:25 PM
> >> To: devel@edk2.groups.io; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
> >> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> >> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >>
> >> Mike,
> >>
> >> I agree that automatically adding reviewers would be helpful.
> >>
> >> Do you think we could add a CODEOWNERS file now to assist with this?
> >>
> >> Benefits being:
> >>
> >> 1. CODEOWNERS is low overhead in that GitHub already supports it.
> >>
> >> 2. We do not need to require CODEOWNER enforcement yet. We can simply
> >> use it for automating the process of adding maintainers.
> >>
> >> 3. Maintainers have write access so they are required to approve and
> >> add the push label. This would add a relevant maintainer(s) with
> >> write access to fulfill that role.
> >>
> >> Thanks,
> >> Michael
> >>
> >> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
> >>> GitHub has notification settings.  Maintainers should configure
> >>> GitHub so they are notified of all PR submissions to edk2 repo.
> >>>
> >>> The Wiki for the Maintainer Process provides a link to this page
> >>> with the
> >> details:
> >>>
> >>> https://docs.github.com/en/account-and-profile/managing-subscription
> >>> s-and-
> >> notifications-on-github/setting-up-notifications/configuring-notifica
> >> tions
> >>>
> >>> https://docs.github.com/en/account-and-profile/managing-subscription
> >>> s-and-
> >> notifications-on-github/setting-up-notifications/configuring-
> >> notifications#about-custom-notifications
> >>>
> >>> This allows Maintainers to receive notifications without being an
> >>> assigned reviewer.
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: Rebecca Cran <rebecca@bsdio.com>
> >>>> Sent: Wednesday, May 29, 2024 11:19 AM
> >>>> To: devel@edk2.groups.io; Kinney, Michael D
> >>>> <michael.d.kinney@intel.com>; kraxel@redhat.com; Kubacki, Michael
> >>>> <michael.kubacki@microsoft.com>
> >>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >>>>
> >>>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
> >>>>> Hi Gerd,
> >>>>>
> >>>>> We can clarify the Wiki.  A Maintainer has to be involved in every
> >>>>> code review.  The first action a Maintainer does is verify that
> >>>>> the code change should be considered at all or rejected.  Then add
> >>>>> maintainers for the ones that look like good submissions.
> >>>>>
> >>>>> I agree that this may add a bit of delay.  You are welcome to add
> >>>>> a comment with @<githubid> tags to the maintainers and reviewers
> >>>>> so they know to look at it and do the assignments.  If you like
> >>>>> that idea we can add that to the Wiki as well.
> >>>>>
> >>>>> Since PRs can be also opened by outside contributors that have
> >>>>> limited access, maintainers have to be involved in reviewing newly
> >>>>> submitted PRs.
> >>>>
> >>>> "Then add maintainers for the ones that look like good submissions."
> >>>>
> >>>> Surely in order for the maintainers to know the PR should be looked
> >>>> at in the first place they need to already be added?
> >>>>
> >>>> --
> >>>>
> >>>> Rebecca Cran
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119401): https://edk2.groups.io/g/devel/message/119401
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hi Michael,

Do you know why there would be dependabot commits in PRs not related to dependabot updates?

For example:

    https://github.com/tianocore/edk2/pull/5708/commits

Thanks,

Mike



> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, May 30, 2024 11:01 AM
> To: Kasbekar, Saloni <saloni.kasbekar@intel.com>; devel@edk2.groups.io;
> mikuback@linux.microsoft.com; Rebecca Cran <rebecca@bsdio.com>;
> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> Yes.
> 
> Mike
> 
> > -----Original Message-----
> > From: Kasbekar, Saloni <saloni.kasbekar@intel.com>
> > Sent: Thursday, May 30, 2024 10:39 AM
> > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
> > kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> > Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> >
> > How are we planning to handle packages (like NetworkPkg) without a
> > maintainer? Would the stewards add in the reviewers in that case?
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> > Kubacki
> > Sent: Wednesday, May 29, 2024 5:51 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
> > Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael
> > <michael.kubacki@microsoft.com>
> > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >
> > I've updated the wiki process to clarify that maintainers need to ensure PR
> > reviewers are added and that a contributor cannot do so unless they are a
> > maintainer in this update:
> >
> > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> >
> Process/_compare/b9d1f34e40d8ecbc7b59632302c9b0c823f52ab8...dd846b296754da310
> > 03f8c2a70e9a0b861e600b6
> >
> > I did not include the @<github> ID process since there was not clear
> > consensus on that. I think it's tedious and for this small window of time
> > without automation, ideally the GitHub notifications mentioned earlier can
> be
> > used.
> >
> > Thanks,
> > Michael
> >
> > On 5/29/2024 4:06 PM, Kinney, Michael D wrote:
> > > We could, but that would require manually syncing CODEOWNERS with
> > > Maintainer.txt until that part of the process is automated.
> > >
> > > We also need a way to verify that CODEOWNERS and Maintainers.txt
> > > produce the exact same assignments.
> > >
> > > This was on the list of future enhancements when resources are
> > > available to help.  We need to focus on a process that works until
> > > those automations can be deployed and supported.
> > >
> > > Mike
> > >
> > >> -----Original Message-----
> > >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> > >> Sent: Wednesday, May 29, 2024 12:25 PM
> > >> To: devel@edk2.groups.io; Kinney, Michael D
> > >> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
> > >> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
> > >> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> > >>
> > >> Mike,
> > >>
> > >> I agree that automatically adding reviewers would be helpful.
> > >>
> > >> Do you think we could add a CODEOWNERS file now to assist with this?
> > >>
> > >> Benefits being:
> > >>
> > >> 1. CODEOWNERS is low overhead in that GitHub already supports it.
> > >>
> > >> 2. We do not need to require CODEOWNER enforcement yet. We can simply
> > >> use it for automating the process of adding maintainers.
> > >>
> > >> 3. Maintainers have write access so they are required to approve and
> > >> add the push label. This would add a relevant maintainer(s) with
> > >> write access to fulfill that role.
> > >>
> > >> Thanks,
> > >> Michael
> > >>
> > >> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
> > >>> GitHub has notification settings.  Maintainers should configure
> > >>> GitHub so they are notified of all PR submissions to edk2 repo.
> > >>>
> > >>> The Wiki for the Maintainer Process provides a link to this page
> > >>> with the
> > >> details:
> > >>>
> > >>> https://docs.github.com/en/account-and-profile/managing-subscription
> > >>> s-and-
> > >> notifications-on-github/setting-up-notifications/configuring-notifica
> > >> tions
> > >>>
> > >>> https://docs.github.com/en/account-and-profile/managing-subscription
> > >>> s-and-
> > >> notifications-on-github/setting-up-notifications/configuring-
> > >> notifications#about-custom-notifications
> > >>>
> > >>> This allows Maintainers to receive notifications without being an
> > >>> assigned reviewer.
> > >>>
> > >>> Mike
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Rebecca Cran <rebecca@bsdio.com>
> > >>>> Sent: Wednesday, May 29, 2024 11:19 AM
> > >>>> To: devel@edk2.groups.io; Kinney, Michael D
> > >>>> <michael.d.kinney@intel.com>; kraxel@redhat.com; Kubacki, Michael
> > >>>> <michael.kubacki@microsoft.com>
> > >>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> > >>>>
> > >>>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
> > >>>>> Hi Gerd,
> > >>>>>
> > >>>>> We can clarify the Wiki.  A Maintainer has to be involved in every
> > >>>>> code review.  The first action a Maintainer does is verify that
> > >>>>> the code change should be considered at all or rejected.  Then add
> > >>>>> maintainers for the ones that look like good submissions.
> > >>>>>
> > >>>>> I agree that this may add a bit of delay.  You are welcome to add
> > >>>>> a comment with @<githubid> tags to the maintainers and reviewers
> > >>>>> so they know to look at it and do the assignments.  If you like
> > >>>>> that idea we can add that to the Wiki as well.
> > >>>>>
> > >>>>> Since PRs can be also opened by outside contributors that have
> > >>>>> limited access, maintainers have to be involved in reviewing newly
> > >>>>> submitted PRs.
> > >>>>
> > >>>> "Then add maintainers for the ones that look like good submissions."
> > >>>>
> > >>>> Surely in order for the maintainers to know the PR should be looked
> > >>>> at in the first place they need to already be added?
> > >>>>
> > >>>> --
> > >>>>
> > >>>> Rebecca Cran
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119403): https://edk2.groups.io/g/devel/message/119403
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael Kubacki]

I do not see the commits now but given dependabot commits are the latest 
in the master branch, I'd assume a branch was not rebased correctly.

Thanks,
Michael

On 5/30/2024 11:50 PM, Michael D Kinney wrote:
> Hi Michael,
> 
> Do you know why there would be dependabot commits in PRs not related to dependabot updates?
> 
> For example:
> 
>      https://github.com/tianocore/edk2/pull/5708/commits
> 
> Thanks,
> 
> Mike
> 
> 
> 
>> -----Original Message-----
>> From: Kinney, Michael D <michael.d.kinney@intel.com>
>> Sent: Thursday, May 30, 2024 11:01 AM
>> To: Kasbekar, Saloni <saloni.kasbekar@intel.com>; devel@edk2.groups.io;
>> mikuback@linux.microsoft.com; Rebecca Cran <rebecca@bsdio.com>;
>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
>>
>> Yes.
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Kasbekar, Saloni <saloni.kasbekar@intel.com>
>>> Sent: Thursday, May 30, 2024 10:39 AM
>>> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
>>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>>> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
>>>
>>> How are we planning to handle packages (like NetworkPkg) without a
>>> maintainer? Would the stewards add in the reviewers in that case?
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
>>> Kubacki
>>> Sent: Wednesday, May 29, 2024 5:51 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
>>> Rebecca Cran <rebecca@bsdio.com>; kraxel@redhat.com; Kubacki, Michael
>>> <michael.kubacki@microsoft.com>
>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>>
>>> I've updated the wiki process to clarify that maintainers need to ensure PR
>>> reviewers are added and that a contributor cannot do so unless they are a
>>> maintainer in this update:
>>>
>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
>>>
>> Process/_compare/b9d1f34e40d8ecbc7b59632302c9b0c823f52ab8...dd846b296754da310
>>> 03f8c2a70e9a0b861e600b6
>>>
>>> I did not include the @<github> ID process since there was not clear
>>> consensus on that. I think it's tedious and for this small window of time
>>> without automation, ideally the GitHub notifications mentioned earlier can
>> be
>>> used.
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 5/29/2024 4:06 PM, Kinney, Michael D wrote:
>>>> We could, but that would require manually syncing CODEOWNERS with
>>>> Maintainer.txt until that part of the process is automated.
>>>>
>>>> We also need a way to verify that CODEOWNERS and Maintainers.txt
>>>> produce the exact same assignments.
>>>>
>>>> This was on the list of future enhancements when resources are
>>>> available to help.  We need to focus on a process that works until
>>>> those automations can be deployed and supported.
>>>>
>>>> Mike
>>>>
>>>>> -----Original Message-----
>>>>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>>>>> Sent: Wednesday, May 29, 2024 12:25 PM
>>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; Rebecca Cran <rebecca@bsdio.com>;
>>>>> kraxel@redhat.com; Kubacki, Michael <michael.kubacki@microsoft.com>
>>>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>>>>
>>>>> Mike,
>>>>>
>>>>> I agree that automatically adding reviewers would be helpful.
>>>>>
>>>>> Do you think we could add a CODEOWNERS file now to assist with this?
>>>>>
>>>>> Benefits being:
>>>>>
>>>>> 1. CODEOWNERS is low overhead in that GitHub already supports it.
>>>>>
>>>>> 2. We do not need to require CODEOWNER enforcement yet. We can simply
>>>>> use it for automating the process of adding maintainers.
>>>>>
>>>>> 3. Maintainers have write access so they are required to approve and
>>>>> add the push label. This would add a relevant maintainer(s) with
>>>>> write access to fulfill that role.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>> On 5/29/2024 2:27 PM, Michael D Kinney wrote:
>>>>>> GitHub has notification settings.  Maintainers should configure
>>>>>> GitHub so they are notified of all PR submissions to edk2 repo.
>>>>>>
>>>>>> The Wiki for the Maintainer Process provides a link to this page
>>>>>> with the
>>>>> details:
>>>>>>
>>>>>> https://docs.github.com/en/account-and-profile/managing-subscription
>>>>>> s-and-
>>>>> notifications-on-github/setting-up-notifications/configuring-notifica
>>>>> tions
>>>>>>
>>>>>> https://docs.github.com/en/account-and-profile/managing-subscription
>>>>>> s-and-
>>>>> notifications-on-github/setting-up-notifications/configuring-
>>>>> notifications#about-custom-notifications
>>>>>>
>>>>>> This allows Maintainers to receive notifications without being an
>>>>>> assigned reviewer.
>>>>>>
>>>>>> Mike
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Rebecca Cran <rebecca@bsdio.com>
>>>>>>> Sent: Wednesday, May 29, 2024 11:19 AM
>>>>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>>>>> <michael.d.kinney@intel.com>; kraxel@redhat.com; Kubacki, Michael
>>>>>>> <michael.kubacki@microsoft.com>
>>>>>>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>>>>>>
>>>>>>> On 5/29/2024 12:09 PM, Michael D Kinney via groups.io wrote:
>>>>>>>> Hi Gerd,
>>>>>>>>
>>>>>>>> We can clarify the Wiki.  A Maintainer has to be involved in every
>>>>>>>> code review.  The first action a Maintainer does is verify that
>>>>>>>> the code change should be considered at all or rejected.  Then add
>>>>>>>> maintainers for the ones that look like good submissions.
>>>>>>>>
>>>>>>>> I agree that this may add a bit of delay.  You are welcome to add
>>>>>>>> a comment with @<githubid> tags to the maintainers and reviewers
>>>>>>>> so they know to look at it and do the assignments.  If you like
>>>>>>>> that idea we can add that to the Wiki as well.
>>>>>>>>
>>>>>>>> Since PRs can be also opened by outside contributors that have
>>>>>>>> limited access, maintainers have to be involved in reviewing newly
>>>>>>>> submitted PRs.
>>>>>>>
>>>>>>> "Then add maintainers for the ones that look like good submissions."
>>>>>>>
>>>>>>> Surely in order for the maintainers to know the PR should be looked
>>>>>>> at in the first place they need to already be added?
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Rebecca Cran
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119405): https://edk2.groups.io/g/devel/message/119405
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Dhaval Sharma]

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

Hi Michael,
Just to clarify my understanding. Once a PR is submitted (or it moves from draft to regular PR state), it automatically gets reviewers assigned? I submitted this one https://github.com/tianocore/edk2/pull/5802 and was wondering if I should be sending maintainers an email or be assured that they have seen it in which case I would not like to bother them.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119664): https://edk2.groups.io/g/devel/message/119664
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

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

We are working on auto assignment.

Right now, maintainers need to watch new PRs and do assignments.

Mike

From: Dhaval Sharma <dhaval@rivosinc.com>
Sent: Thursday, June 20, 2024 10:16 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] GitHub PR Code Review process now active

Hi Michael,
Just to clarify my understanding. Once a PR is submitted (or it moves from draft to regular PR state), it automatically gets reviewers assigned? I submitted this one https://github.com/tianocore/edk2/pull/5802 and was wondering if I should be sending maintainers an email or be assured that they have seen it in which case I would not like to bother them.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119665): https://edk2.groups.io/g/devel/message/119665
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] GitHub PR Code Review process now active

[Gerd Hoffmann]

On Wed, May 29, 2024 at 08:06:00PM GMT, Kinney, Michael D wrote:
> We could, but that would require manually syncing CODEOWNERS
> with Maintainer.txt until that part of the process is automated.

https://github.com/tianocore/edk2/pull/5703 ;)

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119382): https://edk2.groups.io/g/devel/message/119382
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

All Maintainer.txt rules are matches and all maintainers/reviewers are combined

CODEOWNERS only matches the last rule.

I do not think your suggestion works. 

Mike

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Thursday, May 30, 2024 1:33 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io;
> Rebecca Cran <rebecca@bsdio.com>; Kubacki, Michael
> <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> On Wed, May 29, 2024 at 08:06:00PM GMT, Kinney, Michael D wrote:
> > We could, but that would require manually syncing CODEOWNERS
> > with Maintainer.txt until that part of the process is automated.
> 
> https://github.com/tianocore/edk2/pull/5703 ;)
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119396): https://edk2.groups.io/g/devel/message/119396
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Chang, Abner via groups.io]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Mike,
Wondering if we also plan to apply GitHub PR process on edk2-platforms repo? Or other repos under tianocore? I found there is another email thread "Enable GitHub PR, protected branches, and 'push' label" on edk2-platforms, but no further discussions then.

Thanks
Abner

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney via groups.io
> Sent: Wednesday, May 29, 2024 2:54 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] GitHub PR Code Review process now active
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hello,
>
> The GitHub PR code review process is now active.  Please
> use the new PR based code review process for all new
> submissions starting today.
>
> * The Wiki has been updated with the process changes.
>
>   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process
>
>   Big thanks to Michael Kubacki for writing up all the
>   changes based on the RFC proposal and community discussions.
>
>   We will learn by using, so if you see anything missing or
>   incorrect or clarifications needed, please send feedback
>   here so the Wiki pages can be updated quickly for everyone.
>
> * The edk2 repo settings have been updated to require
>   a GitHub PR code review approval before merging and
>   all conversations must be resolved before merging.
>
> * A PR has been opened that removes the requirement for
>   Cc: tags in the commit messages and is the first PR
>   that will use the new process. This PR needs to be
>   reviewed and merged to support the revised commit
>   message format.
>
>   https://github.com/tianocore/edk2/pull/5688
>
>   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> Format
>
> * Please use "Draft" PRs to run CI without any reviews.
>   Once ready for reviews, convert from "Draft" to
>   "Ready for Review".
>
> * For active code reviews that are almost complete and will
>   be ready for merge in the next few days, the submitter may
>   choose to either complete using the email review process,
>   or switch to the PR based code review process.
>
> * For active code reviews that are expected to take more than
>   a few days to complete, please convert to a PR based code review.
>
> Thanks,
>
> Mike
>
>
>
> 
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119337): https://edk2.groups.io/g/devel/message/119337
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hi Abner,

Yes.  The plan is to apply to all repos.

We want to use it on edk2 for a while to make sure we get the
settings and process correct, then we will expand.

Mike

> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com>
> Sent: Wednesday, May 29, 2024 3:41 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: GitHub PR Code Review process now active
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Mike,
> Wondering if we also plan to apply GitHub PR process on edk2-platforms repo?
> Or other repos under tianocore? I found there is another email thread "Enable
> GitHub PR, protected branches, and 'push' label" on edk2-platforms, but no
> further discussions then.
> 
> Thanks
> Abner
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> > Kinney via groups.io
> > Sent: Wednesday, May 29, 2024 2:54 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2-devel] GitHub PR Code Review process now active
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > Hello,
> >
> > The GitHub PR code review process is now active.  Please
> > use the new PR based code review process for all new
> > submissions starting today.
> >
> > * The Wiki has been updated with the process changes.
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-Process
> >
> >   Big thanks to Michael Kubacki for writing up all the
> >   changes based on the RFC proposal and community discussions.
> >
> >   We will learn by using, so if you see anything missing or
> >   incorrect or clarifications needed, please send feedback
> >   here so the Wiki pages can be updated quickly for everyone.
> >
> > * The edk2 repo settings have been updated to require
> >   a GitHub PR code review approval before merging and
> >   all conversations must be resolved before merging.
> >
> > * A PR has been opened that removes the requirement for
> >   Cc: tags in the commit messages and is the first PR
> >   that will use the new process. This PR needs to be
> >   reviewed and merged to support the revised commit
> >   message format.
> >
> >   https://github.com/tianocore/edk2/pull/5688
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > Format
> >
> > * Please use "Draft" PRs to run CI without any reviews.
> >   Once ready for reviews, convert from "Draft" to
> >   "Ready for Review".
> >
> > * For active code reviews that are almost complete and will
> >   be ready for merge in the next few days, the submitter may
> >   choose to either complete using the email review process,
> >   or switch to the PR based code review process.
> >
> > * For active code reviews that are expected to take more than
> >   a few days to complete, please convert to a PR based code review.
> >
> > Thanks,
> >
> > Mike
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119351): https://edk2.groups.io/g/devel/message/119351
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Chang, Abner via groups.io]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Thanks for the clarification, Mike.

Thanks
Abner
________________________________
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Wednesday, May 29, 2024 10:44:41 PM
To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: GitHub PR Code Review process now active

[AMD Official Use Only - AMD Internal Distribution Only]

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Hi Abner,

Yes.  The plan is to apply to all repos.

We want to use it on edk2 for a while to make sure we get the
settings and process correct, then we will expand.

Mike

> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com>
> Sent: Wednesday, May 29, 2024 3:41 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: GitHub PR Code Review process now active
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Mike,
> Wondering if we also plan to apply GitHub PR process on edk2-platforms repo?
> Or other repos under tianocore? I found there is another email thread "Enable
> GitHub PR, protected branches, and 'push' label" on edk2-platforms, but no
> further discussions then.
>
> Thanks
> Abner
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> > Kinney via groups.io
> > Sent: Wednesday, May 29, 2024 2:54 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2-devel] GitHub PR Code Review process now active
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > Hello,
> >
> > The GitHub PR code review process is now active.  Please
> > use the new PR based code review process for all new
> > submissions starting today.
> >
> > * The Wiki has been updated with the process changes.
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-Process
> >
> >   Big thanks to Michael Kubacki for writing up all the
> >   changes based on the RFC proposal and community discussions.
> >
> >   We will learn by using, so if you see anything missing or
> >   incorrect or clarifications needed, please send feedback
> >   here so the Wiki pages can be updated quickly for everyone.
> >
> > * The edk2 repo settings have been updated to require
> >   a GitHub PR code review approval before merging and
> >   all conversations must be resolved before merging.
> >
> > * A PR has been opened that removes the requirement for
> >   Cc: tags in the commit messages and is the first PR
> >   that will use the new process. This PR needs to be
> >   reviewed and merged to support the revised commit
> >   message format.
> >
> >   https://github.com/tianocore/edk2/pull/5688
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > Format
> >
> > * Please use "Draft" PRs to run CI without any reviews.
> >   Once ready for reviews, convert from "Draft" to
> >   "Ready for Review".
> >
> > * For active code reviews that are almost complete and will
> >   be ready for merge in the next few days, the submitter may
> >   choose to either complete using the email review process,
> >   or switch to the PR based code review process.
> >
> > * For active code reviews that are expected to take more than
> >   a few days to complete, please convert to a PR based code review.
> >
> > Thanks,
> >
> > Mike
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119352): https://edk2.groups.io/g/devel/message/119352
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] GitHub PR Code Review process now active

[Yao, Jiewen]

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

Hey
It is great to see what we have moved to PR process finally.

Just tried today, and I confess I made a mistake - I forget to add PUSH label, but just click "Rebase and Merge" button directly when I see it. And the patch is merged successfully.

Using github native approval and merge process is very common in all other projects I have worked on. I am not sure why we still use PUSH label. Should we consider to retire it?

Or, if we cannot get rid of PUSH label, then I think we should disable "Rebase and Merge" button.
(For me, I cannot help to click the lovely green button when I see it.)

Thank you
Yao, Jiewen


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Wednesday, May 29, 2024 10:48 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] GitHub PR Code Review process now active


[AMD Official Use Only - AMD Internal Distribution Only]

Thanks for the clarification, Mike.

Thanks
Abner
________________________________
From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Wednesday, May 29, 2024 10:44:41 PM
To: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: RE: GitHub PR Code Review process now active

[AMD Official Use Only - AMD Internal Distribution Only]

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Hi Abner,

Yes.  The plan is to apply to all repos.

We want to use it on edk2 for a while to make sure we get the
settings and process correct, then we will expand.

Mike

> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>
> Sent: Wednesday, May 29, 2024 3:41 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Subject: RE: GitHub PR Code Review process now active
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Mike,
> Wondering if we also plan to apply GitHub PR process on edk2-platforms repo?
> Or other repos under tianocore? I found there is another email thread "Enable
> GitHub PR, protected branches, and 'push' label" on edk2-platforms, but no
> further discussions then.
>
> Thanks
> Abner
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Michael D
> > Kinney via groups.io
> > Sent: Wednesday, May 29, 2024 2:54 AM
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> > Subject: [edk2-devel] GitHub PR Code Review process now active
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > Hello,
> >
> > The GitHub PR code review process is now active.  Please
> > use the new PR based code review process for all new
> > submissions starting today.
> >
> > * The Wiki has been updated with the process changes.
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-Process
> >
> >   Big thanks to Michael Kubacki for writing up all the
> >   changes based on the RFC proposal and community discussions.
> >
> >   We will learn by using, so if you see anything missing or
> >   incorrect or clarifications needed, please send feedback
> >   here so the Wiki pages can be updated quickly for everyone.
> >
> > * The edk2 repo settings have been updated to require
> >   a GitHub PR code review approval before merging and
> >   all conversations must be resolved before merging.
> >
> > * A PR has been opened that removes the requirement for
> >   Cc: tags in the commit messages and is the first PR
> >   that will use the new process. This PR needs to be
> >   reviewed and merged to support the revised commit
> >   message format.
> >
> >   https://github.com/tianocore/edk2/pull/5688
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > Format
> >
> > * Please use "Draft" PRs to run CI without any reviews.
> >   Once ready for reviews, convert from "Draft" to
> >   "Ready for Review".
> >
> > * For active code reviews that are almost complete and will
> >   be ready for merge in the next few days, the submitter may
> >   choose to either complete using the email review process,
> >   or switch to the PR based code review process.
> >
> > * For active code reviews that are expected to take more than
> >   a few days to complete, please convert to a PR based code review.
> >
> > Thanks,
> >
> > Mike
> >
> >
> >
> >
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119372): https://edk2.groups.io/g/devel/message/119372
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] GitHub PR Code Review process now active

[Neal Gompa]

On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
<michael.d.kinney=intel.com@groups.io> wrote:
>
> Hello,
>
> The GitHub PR code review process is now active.  Please
> use the new PR based code review process for all new
> submissions starting today.
>
> * The Wiki has been updated with the process changes.
>
>   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
>   Big thanks to Michael Kubacki for writing up all the
>   changes based on the RFC proposal and community discussions.
>
>   We will learn by using, so if you see anything missing or
>   incorrect or clarifications needed, please send feedback
>   here so the Wiki pages can be updated quickly for everyone.
>
> * The edk2 repo settings have been updated to require
>   a GitHub PR code review approval before merging and
>   all conversations must be resolved before merging.
>
> * A PR has been opened that removes the requirement for
>   Cc: tags in the commit messages and is the first PR
>   that will use the new process. This PR needs to be
>   reviewed and merged to support the revised commit
>   message format.
>
>   https://github.com/tianocore/edk2/pull/5688
>
>   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
>
> * Please use "Draft" PRs to run CI without any reviews.
>   Once ready for reviews, convert from "Draft" to
>   "Ready for Review".
>

Generally GitHub doesn't allow CI to run on PRs created as draft pull
requests. Was this changed for edk2?


-- 
真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119429): https://edk2.groups.io/g/devel/message/119429
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

CI jobs are dispatched to both GitHub Actions and Azure Pipelines.

For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs running.

This must imply that edk2 repo allows this.  Do you happen to know where
this is configurable or a link to GitHub docs for configuration?

Mike

> -----Original Message-----
> From: Neal Gompa <ngompa13@gmail.com>
> Sent: Monday, June 3, 2024 9:13 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
> <michael.d.kinney=intel.com@groups.io> wrote:
> >
> > Hello,
> >
> > The GitHub PR code review process is now active.  Please
> > use the new PR based code review process for all new
> > submissions starting today.
> >
> > * The Wiki has been updated with the process changes.
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> Process
> >
> >   Big thanks to Michael Kubacki for writing up all the
> >   changes based on the RFC proposal and community discussions.
> >
> >   We will learn by using, so if you see anything missing or
> >   incorrect or clarifications needed, please send feedback
> >   here so the Wiki pages can be updated quickly for everyone.
> >
> > * The edk2 repo settings have been updated to require
> >   a GitHub PR code review approval before merging and
> >   all conversations must be resolved before merging.
> >
> > * A PR has been opened that removes the requirement for
> >   Cc: tags in the commit messages and is the first PR
> >   that will use the new process. This PR needs to be
> >   reviewed and merged to support the revised commit
> >   message format.
> >
> >   https://github.com/tianocore/edk2/pull/5688
> >
> >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> Format
> >
> > * Please use "Draft" PRs to run CI without any reviews.
> >   Once ready for reviews, convert from "Draft" to
> >   "Ready for Review".
> >
> 
> Generally GitHub doesn't allow CI to run on PRs created as draft pull
> requests. Was this changed for edk2?
> 
> 
> --
> 真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119430): https://edk2.groups.io/g/devel/message/119430
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Neal Gompa]

Hmm, I don't see a setting for it anymore, maybe that's not a thing anymore?

I seemingly recall that draft PRs didn't get CI runs, but if that's
not a thing anymore, then that's fine.

That said, draft PRs cannot be reviewed, so we should not be telling
people to make draft PRs.


On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io

<michael.d.kinney=intel.com@groups.io> wrote:
>
> CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
>
> For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs running.
>
> This must imply that edk2 repo allows this.  Do you happen to know where
> this is configurable or a link to GitHub docs for configuration?
>
> Mike
>
> > -----Original Message-----
> > From: Neal Gompa <ngompa13@gmail.com>
> > Sent: Monday, June 3, 2024 9:13 AM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >
> > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
> > <michael.d.kinney=intel.com@groups.io> wrote:
> > >
> > > Hello,
> > >
> > > The GitHub PR code review process is now active.  Please
> > > use the new PR based code review process for all new
> > > submissions starting today.
> > >
> > > * The Wiki has been updated with the process changes.
> > >
> > >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> > Process
> > >
> > >   Big thanks to Michael Kubacki for writing up all the
> > >   changes based on the RFC proposal and community discussions.
> > >
> > >   We will learn by using, so if you see anything missing or
> > >   incorrect or clarifications needed, please send feedback
> > >   here so the Wiki pages can be updated quickly for everyone.
> > >
> > > * The edk2 repo settings have been updated to require
> > >   a GitHub PR code review approval before merging and
> > >   all conversations must be resolved before merging.
> > >
> > > * A PR has been opened that removes the requirement for
> > >   Cc: tags in the commit messages and is the first PR
> > >   that will use the new process. This PR needs to be
> > >   reviewed and merged to support the revised commit
> > >   message format.
> > >
> > >   https://github.com/tianocore/edk2/pull/5688
> > >
> > >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > Format
> > >
> > > * Please use "Draft" PRs to run CI without any reviews.
> > >   Once ready for reviews, convert from "Draft" to
> > >   "Ready for Review".
> > >
> >
> > Generally GitHub doesn't allow CI to run on PRs created as draft pull
> > requests. Was this changed for edk2?
> >
> >
> > --
> > 真実はいつも一つ！/ Always, there's only one truth!
>
>
> 
>
>


--
真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119433): https://edk2.groups.io/g/devel/message/119433
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

The reason to allow a draft PR is to allow contributors to run all the
CI tests to see if they pass and resolve issues before starting a review.

The CI tests include combinations of OS/compiler that not all contributors
have available.

Mike

> -----Original Message-----
> From: Neal Gompa <ngompa13@gmail.com>
> Sent: Monday, June 3, 2024 11:47 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> Hmm, I don't see a setting for it anymore, maybe that's not a thing anymore?
> 
> I seemingly recall that draft PRs didn't get CI runs, but if that's
> not a thing anymore, then that's fine.
> 
> That said, draft PRs cannot be reviewed, so we should not be telling
> people to make draft PRs.
> 
> 
> On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io
> 
> <michael.d.kinney=intel.com@groups.io> wrote:
> >
> > CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
> >
> > For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs running.
> >
> > This must imply that edk2 repo allows this.  Do you happen to know where
> > this is configurable or a link to GitHub docs for configuration?
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Neal Gompa <ngompa13@gmail.com>
> > > Sent: Monday, June 3, 2024 9:13 AM
> > > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> > >
> > > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
> > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > >
> > > > Hello,
> > > >
> > > > The GitHub PR code review process is now active.  Please
> > > > use the new PR based code review process for all new
> > > > submissions starting today.
> > > >
> > > > * The Wiki has been updated with the process changes.
> > > >
> > > >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-
> > > Process
> > > >
> > > >   Big thanks to Michael Kubacki for writing up all the
> > > >   changes based on the RFC proposal and community discussions.
> > > >
> > > >   We will learn by using, so if you see anything missing or
> > > >   incorrect or clarifications needed, please send feedback
> > > >   here so the Wiki pages can be updated quickly for everyone.
> > > >
> > > > * The edk2 repo settings have been updated to require
> > > >   a GitHub PR code review approval before merging and
> > > >   all conversations must be resolved before merging.
> > > >
> > > > * A PR has been opened that removes the requirement for
> > > >   Cc: tags in the commit messages and is the first PR
> > > >   that will use the new process. This PR needs to be
> > > >   reviewed and merged to support the revised commit
> > > >   message format.
> > > >
> > > >   https://github.com/tianocore/edk2/pull/5688
> > > >
> > > >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > > Format
> > > >
> > > > * Please use "Draft" PRs to run CI without any reviews.
> > > >   Once ready for reviews, convert from "Draft" to
> > > >   "Ready for Review".
> > > >
> > >
> > > Generally GitHub doesn't allow CI to run on PRs created as draft pull
> > > requests. Was this changed for edk2?
> > >
> > >
> > > --
> > > 真実はいつも一つ！/ Always, there's only one truth!
> >
> >
> > 
> >
> >
> 
> 
> --
> 真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119434): https://edk2.groups.io/g/devel/message/119434
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hello,

The PR code review process has been active for a little over
a week now.

There have been about 17 PRs merged since the switch and it appears
to have been mostly working well. I also note that the emails per
day on this mailing list is much smaller as the code reviews have
migrated to PRs.

A few issues have been noted:

1) Contributors that are not EDK II Maintainers do not have permissions
   to assign reviewers 

   * Mitigation #1: EDK II Maintainers review new PRs and assign reviewers
   * Mitigation #2: Use CODEOWNERS to auto assign maintainers.  WIP.

* EDK II Maintainers must review the commit message in each commit and 
  to make sure it follows the required commit message format and has 
  an appropriate Signed-off-by tag. GitHub does not provide a way to
  provide review comments for a commit message. Instead, feedback on
  commit messages must be provided in the main PR conversation and quote
  commit message that requires changes as required.

* Slow CI Performance

  This appears to be due to longer than expected queues in Azure Pipelines.
  Azure Pipelines is working through the backlog. It may help if the 
  number of requests to rebase and number of new commits to open PRs are
  reduced. The Tools/CI team will continue to collect data and determine 
  if other changes are needed to reduce the CI overhead.

* Some PRs have been merged using the "Rebase and Merge" button in the 
  PR after all required reviews completed and all CI checks pass. Instead,
  the "push" label should continue to be used. There does not appear to be
  any unexpected side effects from the "Rebase and Merge" button, but that
  option is not available if the PR needs to be rebased. This is what 
  Mergify handles through a merge queue, so the easiest way to merge right
  now is the "push" label.

  If the most recent commit was not performed by an EDK II Maintainers, then
  Mergify attempt to rebase may fail.

    Mitigation #1: EDK II Maintainer perform a rebase
    Mitigation #2: Update Mergify to use a bot account with write permission
                   to perform rebase operations.

  There was feedback earlier in the year that the git commit history does
  not indicate which maintainer was the committer.  Instead it always shows
  Mergify.

  The use of GitHub Merge Queues will be evaluated to see if it can be used
  instead of Mergify and remove the need for the "push" label and allow the
  "Rebase and Merge" button to be used and avoid the Mergify permission issues.

* Some PRs do not complete all CI checks waiting for "Workflow Approval".
  this can occur when a PR is updated by an outside collaborator that does
  not have any previous "Workflow Approvals" accepted.

  Mitigation #1: EDK II Maintainers review PRs and accept the "Workflow Approval"
                 if the PR looks like a good change request.
  Mitigation #2: Relax the edk2 repo configuration settings related to workflows

* When a PR needs to be rebased, there are 2 options available through the 
  Web UI:

  * Update with merge commit (Never use - generates PatchCheck errors)
  * Update with rebase (Only use this one)

  If Update with merge commit is accidently applied, then redo again 
  Using "Update with rebase"

Please provide feedback if you are seeing other issues or have other suggestions
to improve the process.

Thanks,

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, June 3, 2024 12:38 PM
> To: Neal Gompa <ngompa13@gmail.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> The reason to allow a draft PR is to allow contributors to run all the
> CI tests to see if they pass and resolve issues before starting a review.
> 
> The CI tests include combinations of OS/compiler that not all contributors
> have available.
> 
> Mike
> 
> > -----Original Message-----
> > From: Neal Gompa <ngompa13@gmail.com>
> > Sent: Monday, June 3, 2024 11:47 AM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >
> > Hmm, I don't see a setting for it anymore, maybe that's not a thing
> anymore?
> >
> > I seemingly recall that draft PRs didn't get CI runs, but if that's
> > not a thing anymore, then that's fine.
> >
> > That said, draft PRs cannot be reviewed, so we should not be telling
> > people to make draft PRs.
> >
> >
> > On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io
> >
> > <michael.d.kinney=intel.com@groups.io> wrote:
> > >
> > > CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
> > >
> > > For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs
> running.
> > >
> > > This must imply that edk2 repo allows this.  Do you happen to know where
> > > this is configurable or a link to GitHub docs for configuration?
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Neal Gompa <ngompa13@gmail.com>
> > > > Sent: Monday, June 3, 2024 9:13 AM
> > > > To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > > > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> > > >
> > > > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
> > > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > The GitHub PR code review process is now active.  Please
> > > > > use the new PR based code review process for all new
> > > > > submissions starting today.
> > > > >
> > > > > * The Wiki has been updated with the process changes.
> > > > >
> > > > >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-
> > > > Process
> > > > >
> > > > >   Big thanks to Michael Kubacki for writing up all the
> > > > >   changes based on the RFC proposal and community discussions.
> > > > >
> > > > >   We will learn by using, so if you see anything missing or
> > > > >   incorrect or clarifications needed, please send feedback
> > > > >   here so the Wiki pages can be updated quickly for everyone.
> > > > >
> > > > > * The edk2 repo settings have been updated to require
> > > > >   a GitHub PR code review approval before merging and
> > > > >   all conversations must be resolved before merging.
> > > > >
> > > > > * A PR has been opened that removes the requirement for
> > > > >   Cc: tags in the commit messages and is the first PR
> > > > >   that will use the new process. This PR needs to be
> > > > >   reviewed and merged to support the revised commit
> > > > >   message format.
> > > > >
> > > > >   https://github.com/tianocore/edk2/pull/5688
> > > > >
> > > > >   https://github.com/tianocore/tianocore.github.io/wiki/Commit-
> Message-
> > > > Format
> > > > >
> > > > > * Please use "Draft" PRs to run CI without any reviews.
> > > > >   Once ready for reviews, convert from "Draft" to
> > > > >   "Ready for Review".
> > > > >
> > > >
> > > > Generally GitHub doesn't allow CI to run on PRs created as draft pull
> > > > requests. Was this changed for edk2?
> > > >
> > > >
> > > > --
> > > > 真実はいつも一つ！/ Always, there's only one truth!
> > >
> > >
> > > 
> > >
> > >
> >
> >
> > --
> > 真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119488): https://edk2.groups.io/g/devel/message/119488
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Rebecca Cran via groups.io]

On 6/5/2024 4:21 PM, Michael D Kinney via groups.io wrote:
> * Some PRs have been merged using the "Rebase and Merge" button in the
>    PR after all required reviews completed and all CI checks pass. Instead,
>    the "push" label should continue to be used. There does not appear to be
>    any unexpected side effects from the "Rebase and Merge" button, but that
>    option is not available if the PR needs to be rebased. This is what
>    Mergify handles through a merge queue, so the easiest way to merge right
>    now is the "push" label.
>
>    If the most recent commit was not performed by an EDK II Maintainers, then
>    Mergify attempt to rebase may fail.
>
>      Mitigation #1: EDK II Maintainer perform a rebase
>      Mitigation #2: Update Mergify to use a bot account with write permission
>                     to perform rebase operations.
>
>    There was feedback earlier in the year that the git commit history does
>    not indicate which maintainer was the committer.  Instead it always shows
>    Mergify.
>
>    The use of GitHub Merge Queues will be evaluated to see if it can be used
>    instead of Mergify and remove the need for the "push" label and allow the
>    "Rebase and Merge" button to be used and avoid the Mergify permission issues.

So it sounds like using the "Merge" button is fine as long as the user 
understands they may need to rebase, wait for CI to finish and then merge?

Also, is there a timeline on enabling PRs for the other repos? I'd 
really like to switch to them for edk2-platforms, even if it means 
having to update settings in multiple repos as we find issues with the 
process.


-- 
Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119489): https://edk2.groups.io/g/devel/message/119489
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

The Merge button may not work in that case either if there is
parallel activity by other developers or Mergify.  Using the
"push" label is recommended so Maintainers do not have to wait
and rebase.

I understand the desire to apply to other repos quickly.  Let's
work through some of these known issues for another week and
then evaluate if it should be applied to edk2-platforms yet.

Mike

> -----Original Message-----
> From: Rebecca Cran <rebecca@os.amperecomputing.com>
> Sent: Wednesday, June 5, 2024 3:48 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> On 6/5/2024 4:21 PM, Michael D Kinney via groups.io wrote:
> > * Some PRs have been merged using the "Rebase and Merge" button in the
> >    PR after all required reviews completed and all CI checks pass. Instead,
> >    the "push" label should continue to be used. There does not appear to be
> >    any unexpected side effects from the "Rebase and Merge" button, but that
> >    option is not available if the PR needs to be rebased. This is what
> >    Mergify handles through a merge queue, so the easiest way to merge right
> >    now is the "push" label.
> >
> >    If the most recent commit was not performed by an EDK II Maintainers,
> then
> >    Mergify attempt to rebase may fail.
> >
> >      Mitigation #1: EDK II Maintainer perform a rebase
> >      Mitigation #2: Update Mergify to use a bot account with write
> permission
> >                     to perform rebase operations.
> >
> >    There was feedback earlier in the year that the git commit history does
> >    not indicate which maintainer was the committer.  Instead it always
> shows
> >    Mergify.
> >
> >    The use of GitHub Merge Queues will be evaluated to see if it can be
> used
> >    instead of Mergify and remove the need for the "push" label and allow
> the
> >    "Rebase and Merge" button to be used and avoid the Mergify permission
> issues.
> 
> So it sounds like using the "Merge" button is fine as long as the user
> understands they may need to rebase, wait for CI to finish and then merge?
> 
> Also, is there a timeline on enabling PRs for the other repos? I'd
> really like to switch to them for edk2-platforms, even if it means
> having to update settings in multiple repos as we find issues with the
> process.
> 
> 
> --
> Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119491): https://edk2.groups.io/g/devel/message/119491
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Rebecca Cran]

Hi Mike,


I was wondering if you think we're ready to switch over to PRs on the 
other repos yet?


I'm pushing for this because I have a set of changes waiting to go in 
but we've decided to

wait until we can use the easier PR process.


Should we discuss it in tomorrow's (Monday) Tools call?


-- 
Rebecca Cran


On 6/5/24 5:27 PM, Michael D Kinney wrote:
> The Merge button may not work in that case either if there is
> parallel activity by other developers or Mergify.  Using the
> "push" label is recommended so Maintainers do not have to wait
> and rebase.
>
> I understand the desire to apply to other repos quickly.  Let's
> work through some of these known issues for another week and
> then evaluate if it should be applied to edk2-platforms yet.
>
> Mike
>
>> -----Original Message-----
>> From: Rebecca Cran <rebecca@os.amperecomputing.com>
>> Sent: Wednesday, June 5, 2024 3:48 PM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
>>
>> On 6/5/2024 4:21 PM, Michael D Kinney via groups.io wrote:
>>> * Some PRs have been merged using the "Rebase and Merge" button in the
>>>     PR after all required reviews completed and all CI checks pass. Instead,
>>>     the "push" label should continue to be used. There does not appear to be
>>>     any unexpected side effects from the "Rebase and Merge" button, but that
>>>     option is not available if the PR needs to be rebased. This is what
>>>     Mergify handles through a merge queue, so the easiest way to merge right
>>>     now is the "push" label.
>>>
>>>     If the most recent commit was not performed by an EDK II Maintainers,
>> then
>>>     Mergify attempt to rebase may fail.
>>>
>>>       Mitigation #1: EDK II Maintainer perform a rebase
>>>       Mitigation #2: Update Mergify to use a bot account with write
>> permission
>>>                      to perform rebase operations.
>>>
>>>     There was feedback earlier in the year that the git commit history does
>>>     not indicate which maintainer was the committer.  Instead it always
>> shows
>>>     Mergify.
>>>
>>>     The use of GitHub Merge Queues will be evaluated to see if it can be
>> used
>>>     instead of Mergify and remove the need for the "push" label and allow
>> the
>>>     "Rebase and Merge" button to be used and avoid the Mergify permission
>> issues.
>>
>> So it sounds like using the "Merge" button is fine as long as the user
>> understands they may need to rebase, wait for CI to finish and then merge?
>>
>> Also, is there a timeline on enabling PRs for the other repos? I'd
>> really like to switch to them for edk2-platforms, even if it means
>> having to update settings in multiple repos as we find issues with the
>> process.
>>
>>
>> --
>> Rebecca Cran
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119590): https://edk2.groups.io/g/devel/message/119590
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Guo Dong]

Hi Mike,

Glad to see EDK2 PR code review process is active.
In Slim Bootloader project, it runs BaseTools/Scripts/PatchCheck.py to check the PR commit message when running QEMU CI test  
Maybe you could refer https://github.com/slimbootloader/slimbootloader/blob/48a24b87824321c053cccf367c7c3637ff581fdf/.azurepipelines/azure-pipelines.yml#L38 to check if EDK2 could use similar check.

Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Wednesday, June 5, 2024 3:21 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] GitHub PR Code Review process now active

Hello,

The PR code review process has been active for a little over a week now.

There have been about 17 PRs merged since the switch and it appears to have been mostly working well. I also note that the emails per day on this mailing list is much smaller as the code reviews have migrated to PRs.

A few issues have been noted:

1) Contributors that are not EDK II Maintainers do not have permissions
   to assign reviewers 

   * Mitigation #1: EDK II Maintainers review new PRs and assign reviewers
   * Mitigation #2: Use CODEOWNERS to auto assign maintainers.  WIP.

* EDK II Maintainers must review the commit message in each commit and
  to make sure it follows the required commit message format and has
  an appropriate Signed-off-by tag. GitHub does not provide a way to
  provide review comments for a commit message. Instead, feedback on
  commit messages must be provided in the main PR conversation and quote
  commit message that requires changes as required.

* Slow CI Performance

  This appears to be due to longer than expected queues in Azure Pipelines.
  Azure Pipelines is working through the backlog. It may help if the
  number of requests to rebase and number of new commits to open PRs are
  reduced. The Tools/CI team will continue to collect data and determine
  if other changes are needed to reduce the CI overhead.

* Some PRs have been merged using the "Rebase and Merge" button in the
  PR after all required reviews completed and all CI checks pass. Instead,
  the "push" label should continue to be used. There does not appear to be
  any unexpected side effects from the "Rebase and Merge" button, but that
  option is not available if the PR needs to be rebased. This is what
  Mergify handles through a merge queue, so the easiest way to merge right
  now is the "push" label.

  If the most recent commit was not performed by an EDK II Maintainers, then
  Mergify attempt to rebase may fail.

    Mitigation #1: EDK II Maintainer perform a rebase
    Mitigation #2: Update Mergify to use a bot account with write permission
                   to perform rebase operations.

  There was feedback earlier in the year that the git commit history does
  not indicate which maintainer was the committer.  Instead it always shows
  Mergify.

  The use of GitHub Merge Queues will be evaluated to see if it can be used
  instead of Mergify and remove the need for the "push" label and allow the
  "Rebase and Merge" button to be used and avoid the Mergify permission issues.

* Some PRs do not complete all CI checks waiting for "Workflow Approval".
  this can occur when a PR is updated by an outside collaborator that does
  not have any previous "Workflow Approvals" accepted.

  Mitigation #1: EDK II Maintainers review PRs and accept the "Workflow Approval"
                 if the PR looks like a good change request.
  Mitigation #2: Relax the edk2 repo configuration settings related to workflows

* When a PR needs to be rebased, there are 2 options available through the
  Web UI:

  * Update with merge commit (Never use - generates PatchCheck errors)
  * Update with rebase (Only use this one)

  If Update with merge commit is accidently applied, then redo again
  Using "Update with rebase"

Please provide feedback if you are seeing other issues or have other suggestions to improve the process.

Thanks,

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, June 3, 2024 12:38 PM
> To: Neal Gompa <ngompa13@gmail.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> The reason to allow a draft PR is to allow contributors to run all the 
> CI tests to see if they pass and resolve issues before starting a review.
> 
> The CI tests include combinations of OS/compiler that not all 
> contributors have available.
> 
> Mike
> 
> > -----Original Message-----
> > From: Neal Gompa <ngompa13@gmail.com>
> > Sent: Monday, June 3, 2024 11:47 AM
> > To: devel@edk2.groups.io; Kinney, Michael D 
> > <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >
> > Hmm, I don't see a setting for it anymore, maybe that's not a thing
> anymore?
> >
> > I seemingly recall that draft PRs didn't get CI runs, but if that's 
> > not a thing anymore, then that's fine.
> >
> > That said, draft PRs cannot be reviewed, so we should not be telling 
> > people to make draft PRs.
> >
> >
> > On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io
> >
> > <michael.d.kinney=intel.com@groups.io> wrote:
> > >
> > > CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
> > >
> > > For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs
> running.
> > >
> > > This must imply that edk2 repo allows this.  Do you happen to know 
> > > where this is configurable or a link to GitHub docs for configuration?
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Neal Gompa <ngompa13@gmail.com>
> > > > Sent: Monday, June 3, 2024 9:13 AM
> > > > To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > > > Subject: Re: [edk2-devel] GitHub PR Code Review process now 
> > > > active
> > > >
> > > > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io 
> > > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > The GitHub PR code review process is now active.  Please use 
> > > > > the new PR based code review process for all new submissions 
> > > > > starting today.
> > > > >
> > > > > * The Wiki has been updated with the process changes.
> > > > >
> > > > >   
> > > > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-
> > > > Process
> > > > >
> > > > >   Big thanks to Michael Kubacki for writing up all the
> > > > >   changes based on the RFC proposal and community discussions.
> > > > >
> > > > >   We will learn by using, so if you see anything missing or
> > > > >   incorrect or clarifications needed, please send feedback
> > > > >   here so the Wiki pages can be updated quickly for everyone.
> > > > >
> > > > > * The edk2 repo settings have been updated to require
> > > > >   a GitHub PR code review approval before merging and
> > > > >   all conversations must be resolved before merging.
> > > > >
> > > > > * A PR has been opened that removes the requirement for
> > > > >   Cc: tags in the commit messages and is the first PR
> > > > >   that will use the new process. This PR needs to be
> > > > >   reviewed and merged to support the revised commit
> > > > >   message format.
> > > > >
> > > > >   https://github.com/tianocore/edk2/pull/5688
> > > > >
> > > > >   
> > > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-
> Message-
> > > > Format
> > > > >
> > > > > * Please use "Draft" PRs to run CI without any reviews.
> > > > >   Once ready for reviews, convert from "Draft" to
> > > > >   "Ready for Review".
> > > > >
> > > >
> > > > Generally GitHub doesn't allow CI to run on PRs created as draft 
> > > > pull requests. Was this changed for edk2?
> > > >
> > > >
> > > > --
> > > > 真実はいつも一つ！/ Always, there's only one truth!
> > >
> > >
> > > 
> > >
> > >
> >
> >
> > --
> > 真実はいつも一つ！/ Always, there's only one truth!







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119492): https://edk2.groups.io/g/devel/message/119492
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Michael D Kinney]

Hi Guo,

Thanks.  Edk2 is running PatchCheck.py as part of CI.

The Maintainer still needs to verify the commit message contents
even if it passes PatchCheck.py.

Mike

> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Wednesday, June 5, 2024 6:22 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> 
> Hi Mike,
> 
> Glad to see EDK2 PR code review process is active.
> In Slim Bootloader project, it runs BaseTools/Scripts/PatchCheck.py to check
> the PR commit message when running QEMU CI test
> Maybe you could refer
> https://github.com/slimbootloader/slimbootloader/blob/48a24b87824321c053cccf3
> 67c7c3637ff581fdf/.azurepipelines/azure-pipelines.yml#L38 to check if EDK2
> could use similar check.
> 
> Thanks,
> Guo
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney
> Sent: Wednesday, June 5, 2024 3:21 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> 
> Hello,
> 
> The PR code review process has been active for a little over a week now.
> 
> There have been about 17 PRs merged since the switch and it appears to have
> been mostly working well. I also note that the emails per day on this mailing
> list is much smaller as the code reviews have migrated to PRs.
> 
> A few issues have been noted:
> 
> 1) Contributors that are not EDK II Maintainers do not have permissions
>    to assign reviewers
> 
>    * Mitigation #1: EDK II Maintainers review new PRs and assign reviewers
>    * Mitigation #2: Use CODEOWNERS to auto assign maintainers.  WIP.
> 
> * EDK II Maintainers must review the commit message in each commit and
>   to make sure it follows the required commit message format and has
>   an appropriate Signed-off-by tag. GitHub does not provide a way to
>   provide review comments for a commit message. Instead, feedback on
>   commit messages must be provided in the main PR conversation and quote
>   commit message that requires changes as required.
> 
> * Slow CI Performance
> 
>   This appears to be due to longer than expected queues in Azure Pipelines.
>   Azure Pipelines is working through the backlog. It may help if the
>   number of requests to rebase and number of new commits to open PRs are
>   reduced. The Tools/CI team will continue to collect data and determine
>   if other changes are needed to reduce the CI overhead.
> 
> * Some PRs have been merged using the "Rebase and Merge" button in the
>   PR after all required reviews completed and all CI checks pass. Instead,
>   the "push" label should continue to be used. There does not appear to be
>   any unexpected side effects from the "Rebase and Merge" button, but that
>   option is not available if the PR needs to be rebased. This is what
>   Mergify handles through a merge queue, so the easiest way to merge right
>   now is the "push" label.
> 
>   If the most recent commit was not performed by an EDK II Maintainers, then
>   Mergify attempt to rebase may fail.
> 
>     Mitigation #1: EDK II Maintainer perform a rebase
>     Mitigation #2: Update Mergify to use a bot account with write permission
>                    to perform rebase operations.
> 
>   There was feedback earlier in the year that the git commit history does
>   not indicate which maintainer was the committer.  Instead it always shows
>   Mergify.
> 
>   The use of GitHub Merge Queues will be evaluated to see if it can be used
>   instead of Mergify and remove the need for the "push" label and allow the
>   "Rebase and Merge" button to be used and avoid the Mergify permission
> issues.
> 
> * Some PRs do not complete all CI checks waiting for "Workflow Approval".
>   this can occur when a PR is updated by an outside collaborator that does
>   not have any previous "Workflow Approvals" accepted.
> 
>   Mitigation #1: EDK II Maintainers review PRs and accept the "Workflow
> Approval"
>                  if the PR looks like a good change request.
>   Mitigation #2: Relax the edk2 repo configuration settings related to
> workflows
> 
> * When a PR needs to be rebased, there are 2 options available through the
>   Web UI:
> 
>   * Update with merge commit (Never use - generates PatchCheck errors)
>   * Update with rebase (Only use this one)
> 
>   If Update with merge commit is accidently applied, then redo again
>   Using "Update with rebase"
> 
> Please provide feedback if you are seeing other issues or have other
> suggestions to improve the process.
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, June 3, 2024 12:38 PM
> > To: Neal Gompa <ngompa13@gmail.com>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> >
> > The reason to allow a draft PR is to allow contributors to run all the
> > CI tests to see if they pass and resolve issues before starting a review.
> >
> > The CI tests include combinations of OS/compiler that not all
> > contributors have available.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Neal Gompa <ngompa13@gmail.com>
> > > Sent: Monday, June 3, 2024 11:47 AM
> > > To: devel@edk2.groups.io; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> > >
> > > Hmm, I don't see a setting for it anymore, maybe that's not a thing
> > anymore?
> > >
> > > I seemingly recall that draft PRs didn't get CI runs, but if that's
> > > not a thing anymore, then that's fine.
> > >
> > > That said, draft PRs cannot be reviewed, so we should not be telling
> > > people to make draft PRs.
> > >
> > >
> > > On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io
> > >
> > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > >
> > > > CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
> > > >
> > > > For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs
> > running.
> > > >
> > > > This must imply that edk2 repo allows this.  Do you happen to know
> > > > where this is configurable or a link to GitHub docs for configuration?
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: Neal Gompa <ngompa13@gmail.com>
> > > > > Sent: Monday, June 3, 2024 9:13 AM
> > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > > > > Subject: Re: [edk2-devel] GitHub PR Code Review process now
> > > > > active
> > > > >
> > > > > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io
> > > > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > The GitHub PR code review process is now active.  Please use
> > > > > > the new PR based code review process for all new submissions
> > > > > > starting today.
> > > > > >
> > > > > > * The Wiki has been updated with the process changes.
> > > > > >
> > > > > >
> > > > > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > > Development-
> > > > > Process
> > > > > >
> > > > > >   Big thanks to Michael Kubacki for writing up all the
> > > > > >   changes based on the RFC proposal and community discussions.
> > > > > >
> > > > > >   We will learn by using, so if you see anything missing or
> > > > > >   incorrect or clarifications needed, please send feedback
> > > > > >   here so the Wiki pages can be updated quickly for everyone.
> > > > > >
> > > > > > * The edk2 repo settings have been updated to require
> > > > > >   a GitHub PR code review approval before merging and
> > > > > >   all conversations must be resolved before merging.
> > > > > >
> > > > > > * A PR has been opened that removes the requirement for
> > > > > >   Cc: tags in the commit messages and is the first PR
> > > > > >   that will use the new process. This PR needs to be
> > > > > >   reviewed and merged to support the revised commit
> > > > > >   message format.
> > > > > >
> > > > > >   https://github.com/tianocore/edk2/pull/5688
> > > > > >
> > > > > >
> > > > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-
> > Message-
> > > > > Format
> > > > > >
> > > > > > * Please use "Draft" PRs to run CI without any reviews.
> > > > > >   Once ready for reviews, convert from "Draft" to
> > > > > >   "Ready for Review".
> > > > > >
> > > > >
> > > > > Generally GitHub doesn't allow CI to run on PRs created as draft
> > > > > pull requests. Was this changed for edk2?
> > > > >
> > > > >
> > > > > --
> > > > > 真実はいつも一つ！/ Always, there's only one truth!
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > > --
> > > 真実はいつも一つ！/ Always, there's only one truth!
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119493): https://edk2.groups.io/g/devel/message/119493
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Gerd Hoffmann]

On Mon, Jun 03, 2024 at 02:46:30PM GMT, Neal Gompa wrote:
> That said, draft PRs cannot be reviewed, so we should not be telling
> people to make draft PRs.

It makes sense to open draft PRs, work in the PR until CI is clean,
only then flip the PR to 'ready' and bother maintainers to review.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119447): https://edk2.groups.io/g/devel/message/119447
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-