[edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

Hello,

I would like to propose that TianoCore move all code review from email
based code reviews to GitHub Pull Requests based code reviews.

The proposed date to switch would be immediately after the next stable
tag which is currently scheduled for May 24, 2024.

Updates to the following Wiki page would be required to describe the 
required process when using GitHub Pull Requests for all code review
related activity.

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

A couple examples of the changes that would need to be documented are:

* All contributors, maintainers, and reviewers must have GitHub IDs.
* The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
  or Tested-by: tags.  The only required tag would be Signed-off-by.
* The Pull Request submitter is required to invite the required
  maintainers and reviewers to the pull request. This is the same 
  set of maintainers and reviewers that are required to be listed in
  Cc: tags in today's process.
* Maintainers are responsible for verifying that all conversations in
  the code review are resolved and that all review approvals from the
  required set of maintainers are present before setting the 'push' label.


Please provide feedback
1) If you are not in favor of this change.
2) If you are not in favor of the proposed date of this change.
3) On the process changes you would like to see documented in the Wiki
   pages related to using GitHub Pull Request based code reviews.

There is some prototype work to automate/simplify some of the PR based
code review process steps. Those could be added over time as resources
are available to finish and support them. 

Best regards,

Mike


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Leif Lindholm]

On 2024-05-01 18:43, Michael D Kinney wrote:
> Hello,
> 
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
> 
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.

Thanks Mike.

I'm in favour of this change, and the date.

I still want us to try to figure out how to retain review history beyond 
what github decides we need, but I don't think it justifies indefinitely 
delaying the switchover. And frankly, it will be easier to experiment 
with what works and not after the switch.

/
     Leif

> Updates to the following Wiki page would be required to describe the
> required process when using GitHub Pull Requests for all code review
> related activity.
> 
>      https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
> A couple examples of the changes that would need to be documented are:
> 
> * All contributors, maintainers, and reviewers must have GitHub IDs.
> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>    or Tested-by: tags.  The only required tag would be Signed-off-by.
> * The Pull Request submitter is required to invite the required
>    maintainers and reviewers to the pull request. This is the same
>    set of maintainers and reviewers that are required to be listed in
>    Cc: tags in today's process.
> * Maintainers are responsible for verifying that all conversations in
>    the code review are resolved and that all review approvals from the
>    required set of maintainers are present before setting the 'push' label.
> 
> 
> Please provide feedback
> 1) If you are not in favor of this change.
> 2) If you are not in favor of the proposed date of this change.
> 3) On the process changes you would like to see documented in the Wiki
>     pages related to using GitHub Pull Request based code reviews.
> 
> There is some prototype work to automate/simplify some of the PR based
> code review process steps. Those could be added over time as resources
> are available to finish and support them.
> 
> Best regards,
> 
> Mike
> 
> 
> 
> 
> 



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Dionna Glaze via groups.io]

On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via groups.io
<quic_llindhol=quicinc.com@groups.io> wrote:
>
> On 2024-05-01 18:43, Michael D Kinney wrote:
> > Hello,
> >
> > I would like to propose that TianoCore move all code review from email
> > based code reviews to GitHub Pull Requests based code reviews.
> >
> > The proposed date to switch would be immediately after the next stable
> > tag which is currently scheduled for May 24, 2024.
>
> Thanks Mike.
>
> I'm in favour of this change, and the date.
>
> I still want us to try to figure out how to retain review history beyond
> what github decides we need, but I don't think it justifies indefinitely
> delaying the switchover. And frankly, it will be easier to experiment
> with what works and not after the switch.

+1. UI-based interactions don't export well for archival-permalinking
reasons, and Github archive behavior is for repositories only, not the
reviews.
But yes, wouldn't want to delay for a bot to echo conversations to
devel@edk2.groups.io or some other solution.

>
> /
>      Leif
>
> > Updates to the following Wiki page would be required to describe the
> > required process when using GitHub Pull Requests for all code review
> > related activity.
> >
> >      https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> >
> > A couple examples of the changes that would need to be documented are:
> >
> > * All contributors, maintainers, and reviewers must have GitHub IDs.

It looks like this is already resolved for the existing
Maintainers.txt with the `[username]` syntax, but I don't see any
explanation of that expectation. Seems fine to me.

> > * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
> >    or Tested-by: tags.  The only required tag would be Signed-off-by.
> > * The Pull Request submitter is required to invite the required
> >    maintainers and reviewers to the pull request. This is the same
> >    set of maintainers and reviewers that are required to be listed in
> >    Cc: tags in today's process.

This is not configured on tianocore/edk2 at the moment. I have no way
to invite a reviewer. Is this a planned fix?

> > * Maintainers are responsible for verifying that all conversations in
> >    the code review are resolved and that all review approvals from the
> >    required set of maintainers are present before setting the 'push' label.
> >
> >
> > Please provide feedback
> > 1) If you are not in favor of this change.
> > 2) If you are not in favor of the proposed date of this change.
> > 3) On the process changes you would like to see documented in the Wiki
> >     pages related to using GitHub Pull Request based code reviews.
> >
> > There is some prototype work to automate/simplify some of the PR based
> > code review process steps. Those could be added over time as resources
> > are available to finish and support them.
> >
> > Best regards,
> >
> > Mike
> >
> >
> >
> >
> >
>
>
>
> 
>
>


-- 
-Dionna Glaze, PhD, CISSP (she/her)


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Rebecca Cran]

On Wed, May 1, 2024, at 11:43 AM, Michael D Kinney wrote:
> * The Pull Request submitter is required to invite the required
>   maintainers and reviewers to the pull request. This is the same 
>   set of maintainers and reviewers that are required to be listed in
>   Cc: tags in today's process.

This seems like something that could rather easily be automated by parsing the maintainers file.

Rebecca


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

Thank you for this proposal. We've been anticipating this change for 
years and are excited to help support it.


Here's some items we'd like to raise for feedback that we could help 
implement. Many could likely be done in time for the transition.

1. Automate reviewers - We've discussed CODEOWNERS in the past. However, 
a simpler approach (in maintaining/syncing less files) would be to use 
Maintainers.txt directly with a GitHub workflow since the file already 
contains GitHub IDs.

2. Make PR completion contingent on a GitHub review from at least one 
package maintainer/reviewer for each package in the PR.

3. Dependabot is already used today to automatically create PRs when 
dependencies like pip modules have updates. To allow this to more 
effectively keep dependencies up-to-date, allow dependabot PRs to be 
completed (after normal acceptance criteria like CI and review 
requirements) without a separate human creating a duplicate PR.

4. Potentially warn users (with an automated comment on the PR) if they 
add a push label to a PR that is less than 24 hours old.

5. Leave reminder comments on PRs with absolutely no activity after some 
agreed upon time so reviewers are notified to review the PR without the 
submitter having to watch it and send notifications.

6. Leave reminder comments on PRs that meet all requirements to be 
completed (all reviews accounted for and status checks pass) but are 
still open so those on the PR are notified to complete it without the 
submitter having to manually watch and send reminders.

7. We are happy to help with process documentation.

These are items we think can help facilitate consistency and efficiency 
of contributions.

---

Question: Are you planning to reset review state upon force pushes to 
the PR or allow prior reviews to apply?

---

Notes:

- We've found a PR template helps produce higher quality and consistent 
PR messages. That could be added if there's interest.

- We've also found an automated PR description review tool helps produce 
higher quality PR descriptions which allow reviewers to understand the 
PR more quickly and allow any release note automation to produce higher 
quality release notes.

- We might want to consider utilizing labels better to categorize PR 
impact. For example, "bug", "breaking-change", "new-feature", etc. These 
help with PR searches and PR data queries.

- We've automated release note generation which can categorize changes 
by impact (using labels). This could be useful to produce more detailed 
and informational GitHub release notes for stable tags.

- As you likely know, conversation resolution is a simple option to enforce.

Thanks,
Michael

On 5/1/2024 1:43 PM, Michael D Kinney wrote:
> Hello,
> 
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
> 
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.
> 
> Updates to the following Wiki page would be required to describe the
> required process when using GitHub Pull Requests for all code review
> related activity.
> 
>      https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
> A couple examples of the changes that would need to be documented are:
> 
> * All contributors, maintainers, and reviewers must have GitHub IDs.
> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>    or Tested-by: tags.  The only required tag would be Signed-off-by.
> * The Pull Request submitter is required to invite the required
>    maintainers and reviewers to the pull request. This is the same
>    set of maintainers and reviewers that are required to be listed in
>    Cc: tags in today's process.
> * Maintainers are responsible for verifying that all conversations in
>    the code review are resolved and that all review approvals from the
>    required set of maintainers are present before setting the 'push' label.
> 
> 
> Please provide feedback
> 1) If you are not in favor of this change.
> 2) If you are not in favor of the proposed date of this change.
> 3) On the process changes you would like to see documented in the Wiki
>     pages related to using GitHub Pull Request based code reviews.
> 
> There is some prototype work to automate/simplify some of the PR based
> code review process steps. Those could be added over time as resources
> are available to finish and support them.
> 
> Best regards,
> 
> Mike
> 
> 
> 
> 


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Marcin Juszkiewicz]

W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
> 
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.

O yes! Fully for it!

Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and other 
tianocore/ repositories?


> * The Pull Request submitter is required to invite the required
>    maintainers and reviewers to the pull request. This is the same
>    set of maintainers and reviewers that are required to be listed in
>    Cc: tags in today's process.

That can be done by github action started automatically after opening 
PR. May require changes to GetMaintainer.py script. Would be good to 
have in case someone forget to add one of maintainers.

Also would be nice to have a bot running PatchCheck and uncrustify on PR.


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Ard Biesheuvel]

On Wed, 1 May 2024 at 19:44, Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Hello,
>
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
>
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.
>

+1

Some kind of off-github reflector similar to rebecca's public inbox
[0] would be appreciated to at least be able to keep track of
different versions of a series exactly as they were submitted. One of
the things that annoys me about doing GitHub reviews is dealing with
PRs where the underlying branches gets updated through a force-push
and there is no way to find out what changed, and whether the existing
review comments are still in sync.

Would it be possible to require a separate PR for each revision of a
series, lock the underlying branch, and archive it for future
reference (if desired?)


[0] https://openfw.io/edk2-devel/


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Leif Lindholm]

On 2024-05-02 02:28, Rebecca Cran wrote:
> On Wed, May 1, 2024, at 11:43 AM, Michael D Kinney wrote:
>> * The Pull Request submitter is required to invite the required
>>    maintainers and reviewers to the pull request. This is the same
>>    set of maintainers and reviewers that are required to be listed in
>>    Cc: tags in today's process.
> 
> This seems like something that could rather easily be automated by parsing the maintainers file.

Yes, I pushed an example update to GetMaintainer.py here:
https://github.com/leiflindholm/edk2/tree/getmaintainer-forgeusername

But I think the actual option name, and output behaviour, needs to be 
discussed.

/
     Leif



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Leif Lindholm]

On 2024-05-02 07:33, Marcin Juszkiewicz wrote:
> W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
>> I would like to propose that TianoCore move all code review from email
>> based code reviews to GitHub Pull Requests based code reviews.
>>
>> The proposed date to switch would be immediately after the next stable
>> tag which is currently scheduled for May 24, 2024.
> 
> O yes! Fully for it!
> 
> Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and other 
> tianocore/ repositories?

I don't see why we couldn't switch all of them. Other than we need to 
get all the Maintainers.txt updated with code forge usernames first.

We may want to do one at a time though.

/
     Leif

>> * The Pull Request submitter is required to invite the required
>>    maintainers and reviewers to the pull request. This is the same
>>    set of maintainers and reviewers that are required to be listed in
>>    Cc: tags in today's process.
> 
> That can be done by github action started automatically after opening 
> PR. May require changes to GetMaintainer.py script. Would be good to 
> have in case someone forget to add one of maintainers.
> 
> Also would be nice to have a bot running PatchCheck and uncrustify on PR.
> 
> 
> 
> 
> 



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Leif Lindholm]

On 2024-05-02 04:08, Michael Kubacki wrote:
> Thank you for this proposal. We've been anticipating this change for 
> years and are excited to help support it.
> 
> Here's some items we'd like to raise for feedback that we could help 
> implement. Many could likely be done in time for the transition.
> 
> 1. Automate reviewers - We've discussed CODEOWNERS in the past. However, 
> a simpler approach (in maintaining/syncing less files) would be to use 
> Maintainers.txt directly with a GitHub workflow since the file already 
> contains GitHub IDs.

That would be ideal. I know Mike worked on autogenerating CODEOWNERS 
from Maintainers.txt, but ultimately the latter supports more flexible 
use of wildcards (things like */AArch64/ currently requires reconciling 
against the repo contents).

> 2. Make PR completion contingent on a GitHub review from at least one 
> package maintainer/reviewer for each package in the PR.

Yes.

> 3. Dependabot is already used today to automatically create PRs when 
> dependencies like pip modules have updates. To allow this to more 
> effectively keep dependencies up-to-date, allow dependabot PRs to be 
> completed (after normal acceptance criteria like CI and review 
> requirements) without a separate human creating a duplicate PR.

I am not sure what this means in practice :)
This doesn't sound like one we need to worry about before switchover though.

> 4. Potentially warn users (with an automated comment on the PR) if they 
> add a push label to a PR that is less than 24 hours old.

That sounds good.
Is there any way to prevent force-pushes within 24h of previous push?
That would make setting up a transitional review-scraper less lossy.

> 5. Leave reminder comments on PRs with absolutely no activity after some 
> agreed upon time so reviewers are notified to review the PR without the 
> submitter having to watch it and send notifications.

Yes. But should take priority below 1, 2, and 4. Unless you have a 
pre-cooked thing to drop in of course.

> 6. Leave reminder comments on PRs that meet all requirements to be 
> completed (all reviews accounted for and status checks pass) but are 
> still open so those on the PR are notified to complete it without the 
> submitter having to manually watch and send reminders.

Not a response to this, but triggered by reading this:
Is there any way to approve changes within a PR on a commit by commit basis?

> 7. We are happy to help with process documentation.

Always appreciated,thanks.

Regards,

Leif



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

On 5/2/2024 5:37 AM, Ard Biesheuvel wrote:
> On Wed, 1 May 2024 at 19:44, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
>>
>> Hello,
>>
>> I would like to propose that TianoCore move all code review from email
>> based code reviews to GitHub Pull Requests based code reviews.
>>
>> The proposed date to switch would be immediately after the next stable
>> tag which is currently scheduled for May 24, 2024.
>>
> 
> +1
> 
> Some kind of off-github reflector similar to rebecca's public inbox
> [0] would be appreciated to at least be able to keep track of
> different versions of a series exactly as they were submitted. One of
> the things that annoys me about doing GitHub reviews is dealing with
> PRs where the underlying branches gets updated through a force-push
> and there is no way to find out what changed, and whether the existing
> review comments are still in sync.
> 
> Would it be possible to require a separate PR for each revision of a
> series, lock the underlying branch, and archive it for future
> reference (if desired?)
> 

In order for this to be as intuitive and effective as possible, I 
suggest we stick with conventional usage. A single PR allows:

1. A single place to track all of the changes to the contribution 
including force pushes.

2. Clean (less cluttered) PR history where each unique contribution is 
represented by a single PR.

3. References to the PR such as comments, other PRs, and GitHub issues 
to automatically be reflected on the PR. This allows work related to the 
PR such as tracking issue items, future bug fix PRs, etc. to be easily 
found from the original PR.

The consistent PR number is important as it allows data queries that 
many other tools and GitHub workflows use to operate as expected. For 
example, many scripts and applications use the GitHub REST API [0] 
and/or GitHub GraphQL API [1] to query and present PR information. Logic 
would be complicated everywhere sorting through PRs that are dead ends 
to find if a PR is finally the last in a series, etc. Many off-the-shelf 
tools would produce incorrect results.

In addition, many tools and existing processes depend on a single PR 
link to track the status of a code change. Whether in public bug 
tracking tools or internal tools. That would produce stale links constantly.

It is possible to see force pushed diffs in a PR. For example, in this 
PR [2], I force pushed several times. You can simply click 
"force-pushed" or "Compare" on each force push to get a diff of the 
force pushed changes. On a simple rebase, as is the case in the last 
force push, the diff is empty and it reports the contents as identical.

With this it is possible to easily see branch (and its commit details) 
and checkout a specific push state using its respective commit hash.

[0] https://docs.github.com/en/rest?apiVersion=2022-11-28
[1] https://docs.github.com/en/graphql/overview
[2] https://github.com/tianocore/edk2/pull/4835

> [0] https://openfw.io/edk2-devel/
> 
> 
> 
> 


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

On 5/2/2024 6:34 AM, Leif Lindholm wrote:
> On 2024-05-02 07:33, Marcin Juszkiewicz wrote:
>> W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
>>> I would like to propose that TianoCore move all code review from email
>>> based code reviews to GitHub Pull Requests based code reviews.
>>>
>>> The proposed date to switch would be immediately after the next stable
>>> tag which is currently scheduled for May 24, 2024.
>>
>> O yes! Fully for it!
>>
>> Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and other 
>> tianocore/ repositories?
> 
> I don't see why we couldn't switch all of them. Other than we need to 
> get all the Maintainers.txt updated with code forge usernames first.
> 
> We may want to do one at a time though.
> 
> /
>      Leif
> 
>>> * The Pull Request submitter is required to invite the required
>>>    maintainers and reviewers to the pull request. This is the same
>>>    set of maintainers and reviewers that are required to be listed in
>>>    Cc: tags in today's process.
>>
>> That can be done by github action started automatically after opening 
>> PR. May require changes to GetMaintainer.py script. Would be good to 
>> have in case someone forget to add one of maintainers.
>>
>> Also would be nice to have a bot running PatchCheck and uncrustify on PR.
>>
Yes, this would need to be in a GitHub workflow so it could parse the 
file and ultimately use the GitHub API to add the maintainers. As I 
mentioned in another email, my team has experience doing this and we're 
happy to help where we can.

>>
>>
>>
>>
> 
> 
> 
> 
> 


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Brian J. Johnson]

On 5/1/24 18:19, Dionna Glaze via groups.io wrote:
> On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via groups.io
> <quic_llindhol=quicinc.com@groups.io> wrote:
>>
>> On 2024-05-01 18:43, Michael D Kinney wrote:
>>> Hello,
>>>
>>> I would like to propose that TianoCore move all code review from email
>>> based code reviews to GitHub Pull Requests based code reviews.
>>>
>>> The proposed date to switch would be immediately after the next stable
>>> tag which is currently scheduled for May 24, 2024.
>>
>> Thanks Mike.
>>
>> I'm in favour of this change, and the date.
>>
>> I still want us to try to figure out how to retain review history beyond
>> what github decides we need, but I don't think it justifies indefinitely
>> delaying the switchover. And frankly, it will be easier to experiment
>> with what works and not after the switch.
> 
> +1. UI-based interactions don't export well for archival-permalinking
> reasons, and Github archive behavior is for repositories only, not the
> reviews.
> But yes, wouldn't want to delay for a bot to echo conversations to
> devel@edk2.groups.io or some other solution.
> 

+1 from me as well.  We need to maintain review history in some fairly 
permanent manner, both the reviewed code and review comments.

>>
>> /
>>       Leif
>>
>>> Updates to the following Wiki page would be required to describe the
>>> required process when using GitHub Pull Requests for all code review
>>> related activity.
>>>
>>>       https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>>>
>>> A couple examples of the changes that would need to be documented are:
>>>
>>> * All contributors, maintainers, and reviewers must have GitHub IDs.
> 
> It looks like this is already resolved for the existing
> Maintainers.txt with the `[username]` syntax, but I don't see any
> explanation of that expectation. Seems fine to me.
> 
>>> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>>>     or Tested-by: tags.  The only required tag would be Signed-off-by.

Would those tags be optional?  Test and ack info can be helpful when 
researching a change, to find people who may be knowledgeable about it.

Similarly, the Reviewed-by info is nice to have in the history, without 
having to dig it out of archives.  But it's a bit awkward to add on 
github:  you have to push new commits with the Reviewed-by tags, but 
that changes the SHAs, so it's not obvious that the commits you're 
merging have the same code as the ones which were reviewed.  For the 
email flow, we trust maintainers to get this right.  For the github 
flow, are we deciding to rely exclusively on the PR archives?

What if a maintainer decides to tweak a commit before merging it, eg. to 
fix a trivial typo?  With the email flow they just go ahead and do it. 
With the github flow, would they need to post another PR, so it could 
make it through the process and be merged?

>>> * The Pull Request submitter is required to invite the required
>>>     maintainers and reviewers to the pull request. This is the same
>>>     set of maintainers and reviewers that are required to be listed in
>>>     Cc: tags in today's process.
> 
> This is not configured on tianocore/edk2 at the moment. I have no way
> to invite a reviewer. Is this a planned fix?
> 
>>> * Maintainers are responsible for verifying that all conversations in
>>>     the code review are resolved and that all review approvals from the
>>>     required set of maintainers are present before setting the 'push' label.

Will there be documentation on how to use the conversation resolution 
feature?  It's unclear to me whether the PR owner or the reviewer is 
responsible for marking a conversation "resolved."

>>>
>>>
>>> Please provide feedback
>>> 1) If you are not in favor of this change.
>>> 2) If you are not in favor of the proposed date of this change.
>>> 3) On the process changes you would like to see documented in the Wiki
>>>      pages related to using GitHub Pull Request based code reviews.
>>>
>>> There is some prototype work to automate/simplify some of the PR based
>>> code review process steps. Those could be added over time as resources
>>> are available to finish and support them.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
> 
> 

-- 

                                                 Brian

--------------------------------------------------------------------

   "The answers we have found only serve to raise a whole set of new
    questions.  In some ways we feel we are as confused as ever, but we
    are confused on a higher level and about more important things."
                                            -- Anonymous



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Dionna Glaze via groups.io]

On Thu, May 2, 2024 at 8:59 AM Brian J. Johnson <brian.johnson@hpe.com> wrote:
>
> On 5/1/24 18:19, Dionna Glaze via groups.io wrote:
> > On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via groups.io
> > <quic_llindhol=quicinc.com@groups.io> wrote:
> >>
> >> On 2024-05-01 18:43, Michael D Kinney wrote:
> >>> Hello,
> >>>
> >>> I would like to propose that TianoCore move all code review from email
> >>> based code reviews to GitHub Pull Requests based code reviews.
> >>>
> >>> The proposed date to switch would be immediately after the next stable
> >>> tag which is currently scheduled for May 24, 2024.
> >>
> >> Thanks Mike.
> >>
> >> I'm in favour of this change, and the date.
> >>
> >> I still want us to try to figure out how to retain review history beyond
> >> what github decides we need, but I don't think it justifies indefinitely
> >> delaying the switchover. And frankly, it will be easier to experiment
> >> with what works and not after the switch.
> >
> > +1. UI-based interactions don't export well for archival-permalinking
> > reasons, and Github archive behavior is for repositories only, not the
> > reviews.
> > But yes, wouldn't want to delay for a bot to echo conversations to
> > devel@edk2.groups.io or some other solution.
> >
>
> +1 from me as well.  We need to maintain review history in some fairly
> permanent manner, both the reviewed code and review comments.
>
> >>
> >> /
> >>       Leif
> >>
> >>> Updates to the following Wiki page would be required to describe the
> >>> required process when using GitHub Pull Requests for all code review
> >>> related activity.
> >>>
> >>>       https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> >>>
> >>> A couple examples of the changes that would need to be documented are:
> >>>
> >>> * All contributors, maintainers, and reviewers must have GitHub IDs.
> >
> > It looks like this is already resolved for the existing
> > Maintainers.txt with the `[username]` syntax, but I don't see any
> > explanation of that expectation. Seems fine to me.
> >
> >>> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
> >>>     or Tested-by: tags.  The only required tag would be Signed-off-by.
>
> Would those tags be optional?  Test and ack info can be helpful when
> researching a change, to find people who may be knowledgeable about it.
>
> Similarly, the Reviewed-by info is nice to have in the history, without
> having to dig it out of archives.  But it's a bit awkward to add on
> github:  you have to push new commits with the Reviewed-by tags, but
> that changes the SHAs, so it's not obvious that the commits you're
> merging have the same code as the ones which were reviewed.  For the
> email flow, we trust maintainers to get this right.  For the github
> flow, are we deciding to rely exclusively on the PR archives?
>
> What if a maintainer decides to tweak a commit before merging it, eg. to
> fix a trivial typo?  With the email flow they just go ahead and do it.
> With the github flow, would they need to post another PR, so it could
> make it through the process and be merged?
>
> >>> * The Pull Request submitter is required to invite the required
> >>>     maintainers and reviewers to the pull request. This is the same
> >>>     set of maintainers and reviewers that are required to be listed in
> >>>     Cc: tags in today's process.
> >
> > This is not configured on tianocore/edk2 at the moment. I have no way
> > to invite a reviewer. Is this a planned fix?
> >
> >>> * Maintainers are responsible for verifying that all conversations in
> >>>     the code review are resolved and that all review approvals from the
> >>>     required set of maintainers are present before setting the 'push' label.
>
> Will there be documentation on how to use the conversation resolution
> feature?  It's unclear to me whether the PR owner or the reviewer is
> responsible for marking a conversation "resolved."
>

Github has branch security features that let you _require_ that all
messages are resolved before merging, so that could be turned on.

> >>>
> >>>
> >>> Please provide feedback
> >>> 1) If you are not in favor of this change.
> >>> 2) If you are not in favor of the proposed date of this change.
> >>> 3) On the process changes you would like to see documented in the Wiki
> >>>      pages related to using GitHub Pull Request based code reviews.
> >>>
> >>> There is some prototype work to automate/simplify some of the PR based
> >>> code review process steps. Those could be added over time as resources
> >>> are available to finish and support them.
> >>>
> >>> Best regards,
> >>>
> >>> Mike
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >
>
> --
>
>                                                  Brian
>
> --------------------------------------------------------------------
>
>    "The answers we have found only serve to raise a whole set of new
>     questions.  In some ways we feel we are as confused as ever, but we
>     are confused on a higher level and about more important things."
>                                             -- Anonymous
>


-- 
-Dionna Glaze, PhD, CISSP (she/her)


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

Hi Michael,

I have a version of the auto assignment working, but needs to be 
migrated to TianoCore and synced with the latest Maintainers.txt.

My experience getting this running even as a POC was that it took a lot
of effort to make sure the best security practices were followed and
to configure the empty GitHub App with tokens and permissions. Anytime
custom actions are added, resources to implement, validate, and 
support if they ever fail must be in place.

My question is if there is a manual process that can be used to 
start and these type of automations can be added over time as 
dedicated resources are identified.

Dionna's feedback about contributors not being able to add reviewers
to a PR is correct.  Contributors that are not members of the 
EDK II Maintainers or EDK II Reviewers teams will either need to wait
for a Maintainer or Reviewer to add reviewers, or the contributor must
be added as an "outside collaborator" by an admin. For a manual process
this would require Maintainers to monitor new PRs and make sure the
correct set of Maintainers and Reviewers are added. Perhaps the 
contributor can include @GitHubId mentions in the PR for the required
maintainers/reviewers so there are email notifications.

Details on the auto assignment POC
==================================
It uses CODEOWNERS so maintainers are auto assigned and can use GitHub
features to prevent merges without maintainer approval.  The idea is to
minimize the custom behavior and use as many built-in GitHub features as
possible.  It then reuses the CODEOWNERS syntax for a new file called
REVIEWERS along with some GitHub Actions to auto assign reviewers.  The
actions are run by a registered empty GitHub application so it has a
TianoCore organization bot that is executing the actions with TianoCore
org permissions instead of an individual TianoCore member permissions. 
Example PR with the bot with the name "tianocore-assign-reviewers" 
performing assignments:
    https://github.com/tianocore/edk2-codereview/pull/91

It was activated on this repo to run experiments:
    https://github.com/tianocore/edk2-codereview/tree/master/.github

Example CODEOWNERS file:
    https://github.com/tianocore/edk2-codereview/blob/master/.github/CODEOWNERS

Example REVIEWERS file using same format as CODEOWNERS
    https://github.com/tianocore/edk2-codereview/blob/master/.github/REVIEWERS

Action to assign reviewers from REVIEWERS file:
    https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/AssignReviewers.yml

    Depends on: https://github.com/mdkinney/github-action-assign-reviewers

Action to verify that all files in a repo have CODEOWNES coverage
    https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/CheckCodeOwnerFiles.yml

Action to verify that Maintainers.txt, CODEOWNERS, and REVIEWERS are synced.
This is to support transition from using Maintainers.txt to using CODEOWNERS
and REVIEWERS and can be dropped when Maintainers.txt is removed.
    https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/CheckCodeOwnerMaintainers.yml

    Depends on: https://github.com/mdkinney/github-action-check-codeowners-maintainers

Thanks,

Mike

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Thursday, May 2, 2024 8:21 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> marcin.juszkiewicz@linaro.org; Kinney, Michael D
> <michael.d.kinney@intel.com>; rfc@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>; Andrew Fish (afish@apple.com)
> <afish@apple.com>
> Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review from
> email to GitHub Pull Requests on 5-24-2024
> 
> On 5/2/2024 6:34 AM, Leif Lindholm wrote:
> > On 2024-05-02 07:33, Marcin Juszkiewicz wrote:
> >> W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
> >>> I would like to propose that TianoCore move all code review from
> email
> >>> based code reviews to GitHub Pull Requests based code reviews.
> >>>
> >>> The proposed date to switch would be immediately after the next
> stable
> >>> tag which is currently scheduled for May 24, 2024.
> >>
> >> O yes! Fully for it!
> >>
> >> Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and other
> >> tianocore/ repositories?
> >
> > I don't see why we couldn't switch all of them. Other than we need to
> > get all the Maintainers.txt updated with code forge usernames first.
> >
> > We may want to do one at a time though.
> >
> > /
> >      Leif
> >
> >>> * The Pull Request submitter is required to invite the required
> >>>    maintainers and reviewers to the pull request. This is the same
> >>>    set of maintainers and reviewers that are required to be listed
> in
> >>>    Cc: tags in today's process.
> >>
> >> That can be done by github action started automatically after opening
> >> PR. May require changes to GetMaintainer.py script. Would be good to
> >> have in case someone forget to add one of maintainers.
> >>
> >> Also would be nice to have a bot running PatchCheck and uncrustify on
> PR.
> >>
> Yes, this would need to be in a GitHub workflow so it could parse the
> file and ultimately use the GitHub API to add the maintainers. As I
> mentioned in another email, my team has experience doing this and we're
> happy to help where we can.
> 
> >>
> >>
> >>
> >>
> >
> >
> >
> > 
> >


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Brian J.
> Johnson
> Sent: Thursday, May 2, 2024 8:59 AM
> To: devel@edk2.groups.io; dionnaglaze@google.com;
> quic_llindhol@quicinc.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; rfc@edk2.groups.io;
> Andrew Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> On 5/1/24 18:19, Dionna Glaze via groups.io wrote:
> > On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via groups.io
> > <quic_llindhol=quicinc.com@groups.io> wrote:
> >>
> >> On 2024-05-01 18:43, Michael D Kinney wrote:
> >>> Hello,
> >>>
> >>> I would like to propose that TianoCore move all code review from
> email
> >>> based code reviews to GitHub Pull Requests based code reviews.
> >>>
> >>> The proposed date to switch would be immediately after the next
> stable
> >>> tag which is currently scheduled for May 24, 2024.
> >>
> >> Thanks Mike.
> >>
> >> I'm in favour of this change, and the date.
> >>
> >> I still want us to try to figure out how to retain review history
> beyond
> >> what github decides we need, but I don't think it justifies
> indefinitely
> >> delaying the switchover. And frankly, it will be easier to experiment
> >> with what works and not after the switch.
> >
> > +1. UI-based interactions don't export well for archival-permalinking
> > reasons, and Github archive behavior is for repositories only, not the
> > reviews.
> > But yes, wouldn't want to delay for a bot to echo conversations to
> > devel@edk2.groups.io or some other solution.
> >
> 
> +1 from me as well.  We need to maintain review history in some fairly
> permanent manner, both the reviewed code and review comments.
> 
> >>
> >> /
> >>       Leif
> >>
> >>> Updates to the following Wiki page would be required to describe the
> >>> required process when using GitHub Pull Requests for all code review
> >>> related activity.
> >>>
> >>>       https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process
> >>>
> >>> A couple examples of the changes that would need to be documented
> are:
> >>>
> >>> * All contributors, maintainers, and reviewers must have GitHub IDs.
> >
> > It looks like this is already resolved for the existing
> > Maintainers.txt with the `[username]` syntax, but I don't see any
> > explanation of that expectation. Seems fine to me.
> >
> >>> * The commit message would no longer require Cc:, Reviewed-by:,
> Acked-by:
> >>>     or Tested-by: tags.  The only required tag would be Signed-off-
> by.
> 
> Would those tags be optional?  Test and ack info can be helpful when
> researching a change, to find people who may be knowledgeable about it.
> 
> Similarly, the Reviewed-by info is nice to have in the history, without
> having to dig it out of archives.  But it's a bit awkward to add on
> github:  you have to push new commits with the Reviewed-by tags, but
> that changes the SHAs, so it's not obvious that the commits you're
> merging have the same code as the ones which were reviewed.  For the
> email flow, we trust maintainers to get this right.  For the github
> flow, are we deciding to rely exclusively on the PR archives?
> 
> What if a maintainer decides to tweak a commit before merging it, eg. to
> fix a trivial typo?  With the email flow they just go ahead and do it.
> With the github flow, would they need to post another PR, so it could
> make it through the process and be merged?
> 

Editing commit messages after reviews is something we want to avoid so
the PR does not need to be re-reviewed to be merged.  GitHub has history
of who did the reviews and the conversations that can include information
on testing and review activity.

Yes.  Even trivial changes to the code would require the PR to be updated
and re-reviewed.  It is a different process than email where we allowed
maintainers to make minor changes.  However, even a maintainer making a 
minor change could break things in unexpected ways.  It seems better to
have stricter control over changes and all changes are reviewed and go 
through CI.

> >>> * The Pull Request submitter is required to invite the required
> >>>     maintainers and reviewers to the pull request. This is the same
> >>>     set of maintainers and reviewers that are required to be listed
> in
> >>>     Cc: tags in today's process.
> >
> > This is not configured on tianocore/edk2 at the moment. I have no way
> > to invite a reviewer. Is this a planned fix?
> >
> >>> * Maintainers are responsible for verifying that all conversations
> in
> >>>     the code review are resolved and that all review approvals from
> the
> >>>     required set of maintainers are present before setting the
> 'push' label.
> 
> Will there be documentation on how to use the conversation resolution
> feature?  It's unclear to me whether the PR owner or the reviewer is
> responsible for marking a conversation "resolved."

Please help with proposals for the updated documentation that covers 
this topic.

> 
> >>>
> >>>
> >>> Please provide feedback
> >>> 1) If you are not in favor of this change.
> >>> 2) If you are not in favor of the proposed date of this change.
> >>> 3) On the process changes you would like to see documented in the
> Wiki
> >>>      pages related to using GitHub Pull Request based code reviews.
> >>>
> >>> There is some prototype work to automate/simplify some of the PR
> based
> >>> code review process steps. Those could be added over time as
> resources
> >>> are available to finish and support them.
> >>>
> >>> Best regards,
> >>>
> >>> Mike
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >
> 
> --
> 
>                                                  Brian
> 
> --------------------------------------------------------------------
> 
>    "The answers we have found only serve to raise a whole set of new
>     questions.  In some ways we feel we are as confused as ever, but we
>     are confused on a higher level and about more important things."
>                                             -- Anonymous
> 
> 
> 
> 
> 



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Thursday, May 2, 2024 3:57 AM
> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael
> D <michael.d.kinney@intel.com>; rfc@edk2.groups.io
> Cc: Andrew Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review from
> email to GitHub Pull Requests on 5-24-2024
> 
> On 2024-05-02 04:08, Michael Kubacki wrote:
> > Thank you for this proposal. We've been anticipating this change for
> > years and are excited to help support it.
> >
> > Here's some items we'd like to raise for feedback that we could help
> > implement. Many could likely be done in time for the transition.
> >
> > 1. Automate reviewers - We've discussed CODEOWNERS in the past.
> However,
> > a simpler approach (in maintaining/syncing less files) would be to use
> > Maintainers.txt directly with a GitHub workflow since the file already
> > contains GitHub IDs.
> 
> That would be ideal. I know Mike worked on autogenerating CODEOWNERS
> from Maintainers.txt, but ultimately the latter supports more flexible
> use of wildcards (things like */AArch64/ currently requires reconciling
> against the repo contents).

I have a python script that can verify if there are any differences 
Between CODEOWNERS and Maintainers.txt.  I do not any tools that can
convert Maintainers.txt to CODEOWNERS.  That has been a manual process.

I recommend we identify a plan to switch to CODEOWNERS and drop 
Maintainers.txt. May take a while for everyone to get used to the
differences.

> 
> > 2. Make PR completion contingent on a GitHub review from at least one
> > package maintainer/reviewer for each package in the PR.
> 
> Yes.
> 
> > 3. Dependabot is already used today to automatically create PRs when
> > dependencies like pip modules have updates. To allow this to more
> > effectively keep dependencies up-to-date, allow dependabot PRs to be
> > completed (after normal acceptance criteria like CI and review
> > requirements) without a separate human creating a duplicate PR.
> 
> I am not sure what this means in practice :)
> This doesn't sound like one we need to worry about before switchover
> though.
> 
> > 4. Potentially warn users (with an automated comment on the PR) if
> they
> > add a push label to a PR that is less than 24 hours old.
> 
> That sounds good.
> Is there any way to prevent force-pushes within 24h of previous push?
> That would make setting up a transitional review-scraper less lossy.
> 
> > 5. Leave reminder comments on PRs with absolutely no activity after
> some
> > agreed upon time so reviewers are notified to review the PR without
> the
> > submitter having to watch it and send notifications.
> 
> Yes. But should take priority below 1, 2, and 4. Unless you have a
> pre-cooked thing to drop in of course.
> 
> > 6. Leave reminder comments on PRs that meet all requirements to be
> > completed (all reviews accounted for and status checks pass) but are
> > still open so those on the PR are notified to complete it without the
> > submitter having to manually watch and send reminders.
> 
> Not a response to this, but triggered by reading this:
> Is there any way to approve changes within a PR on a commit by commit
> basis?

Unfortunately, this is not supported.  The approval is al the PR level.
What this means in practice is if there is a single PR with changes
across multiple packages or maintainer responsibility, then the PR
will need approvals from all the required maintainers, and the 
maintainers that wants to apply the 'push' label to merge must
review the set of approvals and make sure all the required ones 
are present.

This check could be automated in the future.

> 
> > 7. We are happy to help with process documentation.
> 
> Always appreciated,thanks.
> 
> Regards,
> 
> Leif



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
<michael.d.kinney=intel.com@groups.io> wrote:
>
> Hello,
>
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
>
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.
>
> Updates to the following Wiki page would be required to describe the
> required process when using GitHub Pull Requests for all code review
> related activity.
>
>     https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> A couple examples of the changes that would need to be documented are:
>
> * All contributors, maintainers, and reviewers must have GitHub IDs.
> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>   or Tested-by: tags.  The only required tag would be Signed-off-by.

I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
way to pull that off with github actions, but I haven't looked). It'll
be a mess if I have to go through online GH PR backlogs just to find
who to CC/add-to-review. It kills the decentralized bit off of git too
:)

> * The Pull Request submitter is required to invite the required
>   maintainers and reviewers to the pull request. This is the same
>   set of maintainers and reviewers that are required to be listed in
>   Cc: tags in today's process.
> * Maintainers are responsible for verifying that all conversations in
>   the code review are resolved and that all review approvals from the
>   required set of maintainers are present before setting the 'push' label.
>
>
> Please provide feedback
> 1) If you are not in favor of this change.

It is sad that we're moving to PRs after I finally got a nice and
sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
it's better than edk2.git's half-email half-PR frankenprocess.
I'd guess this change only encompasses edk2.git? How about the other
repos? Any timeline for those?

-- 
Pedro


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro Falcato
> Sent: Thursday, May 2, 2024 10:51 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew Fish
> (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> <michael.d.kinney=intel.com@groups.io> wrote:
> >
> > Hello,
> >
> > I would like to propose that TianoCore move all code review from email
> > based code reviews to GitHub Pull Requests based code reviews.
> >
> > The proposed date to switch would be immediately after the next stable
> > tag which is currently scheduled for May 24, 2024.
> >
> > Updates to the following Wiki page would be required to describe the
> > required process when using GitHub Pull Requests for all code review
> > related activity.
> >
> >     https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process
> >
> > A couple examples of the changes that would need to be documented are:
> >
> > * All contributors, maintainers, and reviewers must have GitHub IDs.
> > * The commit message would no longer require Cc:, Reviewed-by:, Acked-
> by:
> >   or Tested-by: tags.  The only required tag would be Signed-off-by.
> 
> I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
> way to pull that off with github actions, but I haven't looked). It'll
> be a mess if I have to go through online GH PR backlogs just to find
> who to CC/add-to-review. It kills the decentralized bit off of git too
> :)
> 

Can you provide more details on the impact of the loss?

I am curious how other GitHub projects handle this topic. I see it
is possible with squash and merge to amend the commit message, but
I do not see any features that allow all the commits in a series to
be amended.  TianoCore does not use squash and merge.

The main reason to remove this edit of the commit messages is that
the commit message edits look like a new version of the series to 
git/GitHub and requires review again.  This extra edit is also a
manual process and there is a history of maintainers missing who
did the reviews and acks.  The state in the GitHub PR is always
accurate.

> > * The Pull Request submitter is required to invite the required
> >   maintainers and reviewers to the pull request. This is the same
> >   set of maintainers and reviewers that are required to be listed in
> >   Cc: tags in today's process.
> > * Maintainers are responsible for verifying that all conversations in
> >   the code review are resolved and that all review approvals from the
> >   required set of maintainers are present before setting the 'push'
> label.
> >
> >
> > Please provide feedback
> > 1) If you are not in favor of this change.
> 
> It is sad that we're moving to PRs after I finally got a nice and
> sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
> it's better than edk2.git's half-email half-PR frankenprocess.
> I'd guess this change only encompasses edk2.git? How about the other
> repos? Any timeline for those?

The plan is to apply this to all repos, one at a time.  Need to get the
revised process documented and working in one repo before applying to all.

> 
> --
> Pedro
> 
> 
> 
> 



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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Rebecca Cran]

On 5/2/2024 9:14 AM, Michael Kubacki wrote:
> [2] https://github.com/tianocore/edk2/pull/4835 

I can't believe there's no UI for this, but in case anyone else is 
wondering: the diff can be downloaded by adding ".diff" or ".patch" onto 
the end of the URL - e.g. https://github.com/tianocore/edk2/pull/4835.diff

Also, there's a command-line interface available via the 'gh' command - 
https://cli.github.com/ (see https://cli.github.com/manual/gh_pr for the 
PR related commands).


-- 
Rebecca Cran



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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

On 5/2/2024 6:57 AM, Leif Lindholm wrote:
> On 2024-05-02 04:08, Michael Kubacki wrote:
>> Thank you for this proposal. We've been anticipating this change for 
>> years and are excited to help support it.
>>
>> Here's some items we'd like to raise for feedback that we could help 
>> implement. Many could likely be done in time for the transition.
>>
>> 1. Automate reviewers - We've discussed CODEOWNERS in the past. 
>> However, a simpler approach (in maintaining/syncing less files) would 
>> be to use Maintainers.txt directly with a GitHub workflow since the 
>> file already contains GitHub IDs.
> 
> That would be ideal. I know Mike worked on autogenerating CODEOWNERS 
> from Maintainers.txt, but ultimately the latter supports more flexible 
> use of wildcards (things like */AArch64/ currently requires reconciling 
> against the repo contents).
> 
>> 2. Make PR completion contingent on a GitHub review from at least one 
>> package maintainer/reviewer for each package in the PR.
> 
> Yes.
> 
>> 3. Dependabot is already used today to automatically create PRs when 
>> dependencies like pip modules have updates. To allow this to more 
>> effectively keep dependencies up-to-date, allow dependabot PRs to be 
>> completed (after normal acceptance criteria like CI and review 
>> requirements) without a separate human creating a duplicate PR.
> 
> I am not sure what this means in practice :)
> This doesn't sound like one we need to worry about before switchover 
> though.

It was a minor point to reduce extra work for keeping dependencies 
up-to-date. It will naturally lead to a PR review in the new process 
where we can discuss it further in the review.

> 
>> 4. Potentially warn users (with an automated comment on the PR) if 
>> they add a push label to a PR that is less than 24 hours old.
> 
> That sounds good.
> Is there any way to prevent force-pushes within 24h of previous push?
> That would make setting up a transitional review-scraper less lossy.

We can. There have been cases in the past where we need to critically 
get a change in to unblock CI for example so I didn't want to complicate 
that initially. This could serve as a reminder to those with push access 
in the meantime if we'd like.

> 
>> 5. Leave reminder comments on PRs with absolutely no activity after 
>> some agreed upon time so reviewers are notified to review the PR 
>> without the submitter having to watch it and send notifications.
> 
> Yes. But should take priority below 1, 2, and 4. Unless you have a 
> pre-cooked thing to drop in of course.

Your priorities look good.

> 
>> 6. Leave reminder comments on PRs that meet all requirements to be 
>> completed (all reviews accounted for and status checks pass) but are 
>> still open so those on the PR are notified to complete it without the 
>> submitter having to manually watch and send reminders.
> 
> Not a response to this, but triggered by reading this:
> Is there any way to approve changes within a PR on a commit by commit 
> basis?

Unfortunately not. We typically resolve all conversations before giving 
an approval that would apply to all relevant commits for the given reviewer.

> 
>> 7. We are happy to help with process documentation.
> 
> Always appreciated,thanks.
> 
> Regards,
> 
> Leif
> 
> 
> 
> 
> 


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

On 5/2/2024 12:37 PM, Kinney, Michael D wrote:
> 
> 
>> -----Original Message-----
>> From: Leif Lindholm <quic_llindhol@quicinc.com>
>> Sent: Thursday, May 2, 2024 3:57 AM
>> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael
>> D <michael.d.kinney@intel.com>; rfc@edk2.groups.io
>> Cc: Andrew Fish (afish@apple.com) <afish@apple.com>
>> Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review from
>> email to GitHub Pull Requests on 5-24-2024
>>
>> On 2024-05-02 04:08, Michael Kubacki wrote:
>>> Thank you for this proposal. We've been anticipating this change for
>>> years and are excited to help support it.
>>>
>>> Here's some items we'd like to raise for feedback that we could help
>>> implement. Many could likely be done in time for the transition.
>>>
>>> 1. Automate reviewers - We've discussed CODEOWNERS in the past.
>> However,
>>> a simpler approach (in maintaining/syncing less files) would be to use
>>> Maintainers.txt directly with a GitHub workflow since the file already
>>> contains GitHub IDs.
>>
>> That would be ideal. I know Mike worked on autogenerating CODEOWNERS
>> from Maintainers.txt, but ultimately the latter supports more flexible
>> use of wildcards (things like */AArch64/ currently requires reconciling
>> against the repo contents).
> 
> I have a python script that can verify if there are any differences
> Between CODEOWNERS and Maintainers.txt.  I do not any tools that can
> convert Maintainers.txt to CODEOWNERS.  That has been a manual process.
> 
> I recommend we identify a plan to switch to CODEOWNERS and drop
> Maintainers.txt. May take a while for everyone to get used to the
> differences.
> 

I agree it would be nice to switch to CODEOWNERS to reuse the native 
functionality built into GitHub for it. We could build upon that if 
necessary.


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

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

Hi Mike,

I've seen that repo in that past.

Are the steps defined for what's needed to move to CODEWONERS (and 
REVIEWERS) in terms of technical and process changes needed?

For example, could we start with CODEOWNERS manually synced to 
Maintainers.txt, Maintainers.txt dropped, and then add REVIEWERS in the 
future with additional actions to verify file coverage, etc.?

Thanks,
Michael

On 5/2/2024 12:24 PM, Michael D Kinney wrote:
> Hi Michael,
> 
> I have a version of the auto assignment working, but needs to be
> migrated to TianoCore and synced with the latest Maintainers.txt.
> 
> My experience getting this running even as a POC was that it took a lot
> of effort to make sure the best security practices were followed and
> to configure the empty GitHub App with tokens and permissions. Anytime
> custom actions are added, resources to implement, validate, and
> support if they ever fail must be in place.
> 
> My question is if there is a manual process that can be used to
> start and these type of automations can be added over time as
> dedicated resources are identified.
> 
> Dionna's feedback about contributors not being able to add reviewers
> to a PR is correct.  Contributors that are not members of the
> EDK II Maintainers or EDK II Reviewers teams will either need to wait
> for a Maintainer or Reviewer to add reviewers, or the contributor must
> be added as an "outside collaborator" by an admin. For a manual process
> this would require Maintainers to monitor new PRs and make sure the
> correct set of Maintainers and Reviewers are added. Perhaps the
> contributor can include @GitHubId mentions in the PR for the required
> maintainers/reviewers so there are email notifications.
> 
> Details on the auto assignment POC
> ==================================
> It uses CODEOWNERS so maintainers are auto assigned and can use GitHub
> features to prevent merges without maintainer approval.  The idea is to
> minimize the custom behavior and use as many built-in GitHub features as
> possible.  It then reuses the CODEOWNERS syntax for a new file called
> REVIEWERS along with some GitHub Actions to auto assign reviewers.  The
> actions are run by a registered empty GitHub application so it has a
> TianoCore organization bot that is executing the actions with TianoCore
> org permissions instead of an individual TianoCore member permissions.
> Example PR with the bot with the name "tianocore-assign-reviewers"
> performing assignments:
>      https://github.com/tianocore/edk2-codereview/pull/91
> 
> It was activated on this repo to run experiments:
>      https://github.com/tianocore/edk2-codereview/tree/master/.github
> 
> Example CODEOWNERS file:
>      https://github.com/tianocore/edk2-codereview/blob/master/.github/CODEOWNERS
> 
> Example REVIEWERS file using same format as CODEOWNERS
>      https://github.com/tianocore/edk2-codereview/blob/master/.github/REVIEWERS
> 
> Action to assign reviewers from REVIEWERS file:
>      https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/AssignReviewers.yml
> 
>      Depends on: https://github.com/mdkinney/github-action-assign-reviewers
> 
> Action to verify that all files in a repo have CODEOWNES coverage
>      https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/CheckCodeOwnerFiles.yml
> 
> Action to verify that Maintainers.txt, CODEOWNERS, and REVIEWERS are synced.
> This is to support transition from using Maintainers.txt to using CODEOWNERS
> and REVIEWERS and can be dropped when Maintainers.txt is removed.
>      https://github.com/tianocore/edk2-codereview/blob/master/.github/workflows/CheckCodeOwnerMaintainers.yml
> 
>      Depends on: https://github.com/mdkinney/github-action-check-codeowners-maintainers
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>> Sent: Thursday, May 2, 2024 8:21 AM
>> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
>> marcin.juszkiewicz@linaro.org; Kinney, Michael D
>> <michael.d.kinney@intel.com>; rfc@edk2.groups.io
>> Cc: Leif Lindholm <leif@nuviainc.com>; Andrew Fish (afish@apple.com)
>> <afish@apple.com>
>> Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review from
>> email to GitHub Pull Requests on 5-24-2024
>>
>> On 5/2/2024 6:34 AM, Leif Lindholm wrote:
>>> On 2024-05-02 07:33, Marcin Juszkiewicz wrote:
>>>> W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
>>>>> I would like to propose that TianoCore move all code review from
>> email
>>>>> based code reviews to GitHub Pull Requests based code reviews.
>>>>>
>>>>> The proposed date to switch would be immediately after the next
>> stable
>>>>> tag which is currently scheduled for May 24, 2024.
>>>>
>>>> O yes! Fully for it!
>>>>
>>>> Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and other
>>>> tianocore/ repositories?
>>>
>>> I don't see why we couldn't switch all of them. Other than we need to
>>> get all the Maintainers.txt updated with code forge usernames first.
>>>
>>> We may want to do one at a time though.
>>>
>>> /
>>>       Leif
>>>
>>>>> * The Pull Request submitter is required to invite the required
>>>>>     maintainers and reviewers to the pull request. This is the same
>>>>>     set of maintainers and reviewers that are required to be listed
>> in
>>>>>     Cc: tags in today's process.
>>>>
>>>> That can be done by github action started automatically after opening
>>>> PR. May require changes to GetMaintainer.py script. Would be good to
>>>> have in case someone forget to add one of maintainers.
>>>>
>>>> Also would be nice to have a bot running PatchCheck and uncrustify on
>> PR.
>>>>
>> Yes, this would need to be in a GitHub workflow so it could parse the
>> file and ultimately use the GitHub API to add the maintainers. As I
>> mentioned in another email, my team has experience doing this and we're
>> happy to help where we can.
>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
> 
> 
> 
> 
> 


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro Falcato
> > Sent: Thursday, May 2, 2024 10:51 AM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew Fish
> > (afish@apple.com) <afish@apple.com>
> > Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> > Review from email to GitHub Pull Requests on 5-24-2024
> >
> > On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> > <michael.d.kinney=intel.com@groups.io> wrote:
<snip>
> > > * All contributors, maintainers, and reviewers must have GitHub IDs.
> > > * The commit message would no longer require Cc:, Reviewed-by:, Acked-
> > by:
> > >   or Tested-by: tags.  The only required tag would be Signed-off-by.
> >
> > I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
> > loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
> > way to pull that off with github actions, but I haven't looked). It'll
> > be a mess if I have to go through online GH PR backlogs just to find
> > who to CC/add-to-review. It kills the decentralized bit off of git too
> > :)
> >
>
> Can you provide more details on the impact of the loss?

In my view, commits should be fairly self-describing. What changes,
why, are obvious, but who looked at it, who reviewed it, who was cc'd
but didn't respond, who tested are also pretty important. Git is
supposed to be decentralized, let's not forget. If we ever migrate
from GH, if GH ever goes down, if the links ever go down, you'll never
be able to know who looked at it. If you're looking at an EDK2 commit
deep into an Intel-internal fork, you won't know what "PR #478" is
(heck, rebase-and-merge doesn't reference PRs either).

Side-note: How are we supposed to find the PR for a given commit?
Searching doesn't seem to work well. For instance, I picked a random
non-trivial commit out of the current open PRs:
MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg%2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
has no matches?

>
> I am curious how other GitHub projects handle this topic. I see it

I don't think they do, sadly. But I also don't know many people with a
positive opinion on GH PRs :)

<snip>
> > It is sad that we're moving to PRs after I finally got a nice and
> > sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
> > it's better than edk2.git's half-email half-PR frankenprocess.
> > I'd guess this change only encompasses edk2.git? How about the other
> > repos? Any timeline for those?
>
> The plan is to apply this to all repos, one at a time.  Need to get the
> revised process documented and working in one repo before applying to all.

Gotcha, thanks!

-- 
Pedro


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

Yes.  Many options on transition.  I would suggest we consider the
following steps.

1) Define a manual process where Maintainers/Reviewers look for 
   new PRs and make sure all the required maintainers/reviewers
   are invited to the review.  Current process has many manual
   steps.  Having this one does not seem like a bad transition 
   option.

2) Add CODEOWNERS support to automate assignment of maintainers
   And enforce maintainer approval requirements.

3) Add REVIEWEWS support to automate assignment of reviewers.

Mike

> -----Original Message-----
> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Friday, May 3, 2024 10:22 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; quic_llindhol@quicinc.com;
> marcin.juszkiewicz@linaro.org; rfc@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>; Andrew Fish (afish@apple.com)
> <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> Hi Mike,
> 
> I've seen that repo in that past.
> 
> Are the steps defined for what's needed to move to CODEWONERS (and
> REVIEWERS) in terms of technical and process changes needed?
> 
> For example, could we start with CODEOWNERS manually synced to
> Maintainers.txt, Maintainers.txt dropped, and then add REVIEWERS in the
> future with additional actions to verify file coverage, etc.?
> 
> Thanks,
> Michael
> 
> On 5/2/2024 12:24 PM, Michael D Kinney wrote:
> > Hi Michael,
> >
> > I have a version of the auto assignment working, but needs to be
> > migrated to TianoCore and synced with the latest Maintainers.txt.
> >
> > My experience getting this running even as a POC was that it took a
> lot
> > of effort to make sure the best security practices were followed and
> > to configure the empty GitHub App with tokens and permissions. Anytime
> > custom actions are added, resources to implement, validate, and
> > support if they ever fail must be in place.
> >
> > My question is if there is a manual process that can be used to
> > start and these type of automations can be added over time as
> > dedicated resources are identified.
> >
> > Dionna's feedback about contributors not being able to add reviewers
> > to a PR is correct.  Contributors that are not members of the
> > EDK II Maintainers or EDK II Reviewers teams will either need to wait
> > for a Maintainer or Reviewer to add reviewers, or the contributor must
> > be added as an "outside collaborator" by an admin. For a manual
> process
> > this would require Maintainers to monitor new PRs and make sure the
> > correct set of Maintainers and Reviewers are added. Perhaps the
> > contributor can include @GitHubId mentions in the PR for the required
> > maintainers/reviewers so there are email notifications.
> >
> > Details on the auto assignment POC
> > ==================================
> > It uses CODEOWNERS so maintainers are auto assigned and can use GitHub
> > features to prevent merges without maintainer approval.  The idea is
> to
> > minimize the custom behavior and use as many built-in GitHub features
> as
> > possible.  It then reuses the CODEOWNERS syntax for a new file called
> > REVIEWERS along with some GitHub Actions to auto assign reviewers.
> The
> > actions are run by a registered empty GitHub application so it has a
> > TianoCore organization bot that is executing the actions with
> TianoCore
> > org permissions instead of an individual TianoCore member permissions.
> > Example PR with the bot with the name "tianocore-assign-reviewers"
> > performing assignments:
> >      https://github.com/tianocore/edk2-codereview/pull/91
> >
> > It was activated on this repo to run experiments:
> >      https://github.com/tianocore/edk2-codereview/tree/master/.github
> >
> > Example CODEOWNERS file:
> >      https://github.com/tianocore/edk2-
> codereview/blob/master/.github/CODEOWNERS
> >
> > Example REVIEWERS file using same format as CODEOWNERS
> >      https://github.com/tianocore/edk2-
> codereview/blob/master/.github/REVIEWERS
> >
> > Action to assign reviewers from REVIEWERS file:
> >      https://github.com/tianocore/edk2-
> codereview/blob/master/.github/workflows/AssignReviewers.yml
> >
> >      Depends on: https://github.com/mdkinney/github-action-assign-
> reviewers
> >
> > Action to verify that all files in a repo have CODEOWNES coverage
> >      https://github.com/tianocore/edk2-
> codereview/blob/master/.github/workflows/CheckCodeOwnerFiles.yml
> >
> > Action to verify that Maintainers.txt, CODEOWNERS, and REVIEWERS are
> synced.
> > This is to support transition from using Maintainers.txt to using
> CODEOWNERS
> > and REVIEWERS and can be dropped when Maintainers.txt is removed.
> >      https://github.com/tianocore/edk2-
> codereview/blob/master/.github/workflows/CheckCodeOwnerMaintainers.yml
> >
> >      Depends on: https://github.com/mdkinney/github-action-check-
> codeowners-maintainers
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> >> Sent: Thursday, May 2, 2024 8:21 AM
> >> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> >> marcin.juszkiewicz@linaro.org; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; rfc@edk2.groups.io
> >> Cc: Leif Lindholm <leif@nuviainc.com>; Andrew Fish (afish@apple.com)
> >> <afish@apple.com>
> >> Subject: Re: [edk2-devel] Proposal to switch TianoCore Code Review
> from
> >> email to GitHub Pull Requests on 5-24-2024
> >>
> >> On 5/2/2024 6:34 AM, Leif Lindholm wrote:
> >>> On 2024-05-02 07:33, Marcin Juszkiewicz wrote:
> >>>> W dniu 1.05.2024 o 19:43, Michael D Kinney via groups.io pisze:
> >>>>> I would like to propose that TianoCore move all code review from
> >> email
> >>>>> based code reviews to GitHub Pull Requests based code reviews.
> >>>>>
> >>>>> The proposed date to switch would be immediately after the next
> >> stable
> >>>>> tag which is currently scheduled for May 24, 2024.
> >>>>
> >>>> O yes! Fully for it!
> >>>>
> >>>> Does it mean edk2 only or edk2/edk2-platforms/edk2-non-osi and
> other
> >>>> tianocore/ repositories?
> >>>
> >>> I don't see why we couldn't switch all of them. Other than we need
> to
> >>> get all the Maintainers.txt updated with code forge usernames first.
> >>>
> >>> We may want to do one at a time though.
> >>>
> >>> /
> >>>       Leif
> >>>
> >>>>> * The Pull Request submitter is required to invite the required
> >>>>>     maintainers and reviewers to the pull request. This is the
> same
> >>>>>     set of maintainers and reviewers that are required to be
> listed
> >> in
> >>>>>     Cc: tags in today's process.
> >>>>
> >>>> That can be done by github action started automatically after
> opening
> >>>> PR. May require changes to GetMaintainer.py script. Would be good
> to
> >>>> have in case someone forget to add one of maintainers.
> >>>>
> >>>> Also would be nice to have a bot running PatchCheck and uncrustify
> on
> >> PR.
> >>>>
> >> Yes, this would need to be in a GitHub workflow so it could parse the
> >> file and ultimately use the GitHub API to add the maintainers. As I
> >> mentioned in another email, my team has experience doing this and
> we're
> >> happy to help where we can.
> >>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >
> >
> >
> >
> >
> 
> 
> 
> 



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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Friday, May 3, 2024 10:39 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro
> Falcato
> > > Sent: Thursday, May 2, 2024 10:51 AM
> > > To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > > Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew
> Fish
> > > (afish@apple.com) <afish@apple.com>
> > > Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore
> Code
> > > Review from email to GitHub Pull Requests on 5-24-2024
> > >
> > > On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> > > <michael.d.kinney=intel.com@groups.io> wrote:
> <snip>
> > > > * All contributors, maintainers, and reviewers must have GitHub
> IDs.
> > > > * The commit message would no longer require Cc:, Reviewed-by:,
> Acked-
> > > by:
> > > >   or Tested-by: tags.  The only required tag would be Signed-off-
> by.
> > >
> > > I'd just like to note that losing the CC:, Reviewed-by:, etc is a
> big
> > > loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
> > > way to pull that off with github actions, but I haven't looked).
> It'll
> > > be a mess if I have to go through online GH PR backlogs just to find
> > > who to CC/add-to-review. It kills the decentralized bit off of git
> too
> > > :)
> > >
> >
> > Can you provide more details on the impact of the loss?
> 
> In my view, commits should be fairly self-describing. What changes,
> why, are obvious, but who looked at it, who reviewed it, who was cc'd
> but didn't respond, who tested are also pretty important. Git is
> supposed to be decentralized, let's not forget. If we ever migrate
> from GH, if GH ever goes down, if the links ever go down, you'll never
> be able to know who looked at it. If you're looking at an EDK2 commit
> deep into an Intel-internal fork, you won't know what "PR #478" is
> (heck, rebase-and-merge doesn't reference PRs either).
> 
> Side-note: How are we supposed to find the PR for a given commit?
> Searching doesn't seem to work well. For instance, I picked a random
> non-trivial commit out of the current open PRs:
> MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg
> %2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> has no matches?

If you have the sha of the commit, you can search in GitHub

For example, I selected a commit at random from recent edk2 commit history:

	https://github.com/tianocore/edk2/commit/032830e96841f2a752e364378c3428ac5d2f59d1

Goto the "Pull Requests" tab for the repo and in the "Filters" search box enter 

	is:pr is:merged <sha>

In this example:

	is:pr is:merged 032830e96841f2a752e364378c3428ac5d2f59d1

This returns a single hit on PR #5560

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

There is also a 'gh' command line utility that can be used to write
small scripts to collect this information 

> 
> >
> > I am curious how other GitHub projects handle this topic. I see it
> 
> I don't think they do, sadly. But I also don't know many people with a
> positive opinion on GH PRs :)
> 
> <snip>
> > > It is sad that we're moving to PRs after I finally got a nice and
> > > sane(ish!) email workflow (openfw.io + b4). Otherwise, no
> objections,
> > > it's better than edk2.git's half-email half-PR frankenprocess.
> > > I'd guess this change only encompasses edk2.git? How about the other
> > > repos? Any timeline for those?
> >
> > The plan is to apply this to all repos, one at a time.  Need to get
> the
> > revised process documented and working in one repo before applying to
> all.
> 
> Gotcha, thanks!
> 
> --
> Pedro


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, May 3, 2024 1:13 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> 
> 
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Friday, May 3, 2024 10:39 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> > <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>
> > Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> > Review from email to GitHub Pull Requests on 5-24-2024
> >
> > On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro
> > Falcato
> > > > Sent: Thursday, May 2, 2024 10:51 AM
> > > > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > > > Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew
> > Fish
> > > > (afish@apple.com) <afish@apple.com>
> > > > Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore
> > Code
> > > > Review from email to GitHub Pull Requests on 5-24-2024
> > > >
> > > > On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> > > > <michael.d.kinney=intel.com@groups.io> wrote:
> > <snip>
> > > > > * All contributors, maintainers, and reviewers must have GitHub
> > IDs.
> > > > > * The commit message would no longer require Cc:, Reviewed-by:,
> > Acked-
> > > > by:
> > > > >   or Tested-by: tags.  The only required tag would be Signed-
> off-
> > by.
> > > >
> > > > I'd just like to note that losing the CC:, Reviewed-by:, etc is a
> > big
> > > > loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's
> a
> > > > way to pull that off with github actions, but I haven't looked).
> > It'll
> > > > be a mess if I have to go through online GH PR backlogs just to
> find
> > > > who to CC/add-to-review. It kills the decentralized bit off of git
> > too
> > > > :)
> > > >
> > >
> > > Can you provide more details on the impact of the loss?
> >
> > In my view, commits should be fairly self-describing. What changes,
> > why, are obvious, but who looked at it, who reviewed it, who was cc'd
> > but didn't respond, who tested are also pretty important. Git is
> > supposed to be decentralized, let's not forget. If we ever migrate
> > from GH, if GH ever goes down, if the links ever go down, you'll never
> > be able to know who looked at it. If you're looking at an EDK2 commit
> > deep into an Intel-internal fork, you won't know what "PR #478" is
> > (heck, rebase-and-merge doesn't reference PRs either).
> >
> > Side-note: How are we supposed to find the PR for a given commit?
> > Searching doesn't seem to work well. For instance, I picked a random
> > non-trivial commit out of the current open PRs:
> > MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> >
> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg
> > %2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> > has no matches?
> 
> If you have the sha of the commit, you can search in GitHub
> 
> For example, I selected a commit at random from recent edk2 commit
> history:
> 
> 	https://github.com/tianocore/edk2/commit/032830e96841f2a752e364378c
> 3428ac5d2f59d1
> 
> Goto the "Pull Requests" tab for the repo and in the "Filters" search
> box enter
> 
> 	is:pr is:merged <sha>
> 
> In this example:
> 
> 	is:pr is:merged 032830e96841f2a752e364378c3428ac5d2f59d1
> 
> This returns a single hit on PR #5560
> 
> 	https://github.com/tianocore/edk2/pull/5560
> 
> There is also a 'gh' command line utility that can be used to write
> small scripts to collect this information

Here is the equivalent query and output using 'gh' CLI command:

    gh pr list --repo tianocore/edk2 --state merged --search 032830e96841f2a752e364378c3428ac5d2f59d1

    Showing 1 of 1 pull request in tianocore/edk2 that matches your search

    ID     TITLE     BRANCH            CREATED AT
    #5560  Loongcpu  niruiyu:loongcpu  about 17 days ago


> 
> >
> > >
> > > I am curious how other GitHub projects handle this topic. I see it
> >
> > I don't think they do, sadly. But I also don't know many people with a
> > positive opinion on GH PRs :)
> >
> > <snip>
> > > > It is sad that we're moving to PRs after I finally got a nice and
> > > > sane(ish!) email workflow (openfw.io + b4). Otherwise, no
> > objections,
> > > > it's better than edk2.git's half-email half-PR frankenprocess.
> > > > I'd guess this change only encompasses edk2.git? How about the
> other
> > > > repos? Any timeline for those?
> > >
> > > The plan is to apply this to all repos, one at a time.  Need to get
> > the
> > > revised process documented and working in one repo before applying
> to
> > all.
> >
> > Gotcha, thanks!
> >
> > --
> > Pedro


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

On 5/3/2024 4:38 PM, Michael D Kinney wrote:
> 
> 
>> -----Original Message-----
>> From: Kinney, Michael D <michael.d.kinney@intel.com>
>> Sent: Friday, May 3, 2024 1:13 PM
>> To: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
>> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>;
>> Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: RE: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
>> Review from email to GitHub Pull Requests on 5-24-2024
>>
>>
>>
>>> -----Original Message-----
>>> From: Pedro Falcato <pedro.falcato@gmail.com>
>>> Sent: Friday, May 3, 2024 10:39 AM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
>>> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>
>>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
>>> Review from email to GitHub Pull Requests on 5-24-2024
>>>
>>> On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
>>> <michael.d.kinney@intel.com> wrote:
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro
>>> Falcato
>>>>> Sent: Thursday, May 2, 2024 10:51 AM
>>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>> <michael.d.kinney@intel.com>
>>>>> Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew
>>> Fish
>>>>> (afish@apple.com) <afish@apple.com>
>>>>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore
>>> Code
>>>>> Review from email to GitHub Pull Requests on 5-24-2024
>>>>>
>>>>> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
>>>>> <michael.d.kinney=intel.com@groups.io> wrote:
>>> <snip>
>>>>>> * All contributors, maintainers, and reviewers must have GitHub
>>> IDs.
>>>>>> * The commit message would no longer require Cc:, Reviewed-by:,
>>> Acked-
>>>>> by:
>>>>>>    or Tested-by: tags.  The only required tag would be Signed-
>> off-
>>> by.
>>>>>
>>>>> I'd just like to note that losing the CC:, Reviewed-by:, etc is a
>>> big
>>>>> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's
>> a
>>>>> way to pull that off with github actions, but I haven't looked).
>>> It'll
>>>>> be a mess if I have to go through online GH PR backlogs just to
>> find
>>>>> who to CC/add-to-review. It kills the decentralized bit off of git
>>> too
>>>>> :)
>>>>>
>>>>
>>>> Can you provide more details on the impact of the loss?
>>>
>>> In my view, commits should be fairly self-describing. What changes,
>>> why, are obvious, but who looked at it, who reviewed it, who was cc'd
>>> but didn't respond, who tested are also pretty important. Git is
>>> supposed to be decentralized, let's not forget. If we ever migrate
>>> from GH, if GH ever goes down, if the links ever go down, you'll never
>>> be able to know who looked at it. If you're looking at an EDK2 commit
>>> deep into an Intel-internal fork, you won't know what "PR #478" is
>>> (heck, rebase-and-merge doesn't reference PRs either).
>>>
>>> Side-note: How are we supposed to find the PR for a given commit?
>>> Searching doesn't seem to work well. For instance, I picked a random
>>> non-trivial commit out of the current open PRs:
>>> MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
>>>
>> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg
>>> %2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
>>> has no matches?
>>
>> If you have the sha of the commit, you can search in GitHub
>>
>> For example, I selected a commit at random from recent edk2 commit
>> history:
>>
>> 	https://github.com/tianocore/edk2/commit/032830e96841f2a752e364378c
>> 3428ac5d2f59d1
>>
>> Goto the "Pull Requests" tab for the repo and in the "Filters" search
>> box enter
>>
>> 	is:pr is:merged <sha>
>>
>> In this example:
>>
>> 	is:pr is:merged 032830e96841f2a752e364378c3428ac5d2f59d1
>>
>> This returns a single hit on PR #5560
>>
>> 	https://github.com/tianocore/edk2/pull/5560
>>
>> There is also a 'gh' command line utility that can be used to write
>> small scripts to collect this information
> 
> Here is the equivalent query and output using 'gh' CLI command:
> 
>      gh pr list --repo tianocore/edk2 --state merged --search 032830e96841f2a752e364378c3428ac5d2f59d1
> 
>      Showing 1 of 1 pull request in tianocore/edk2 that matches your search
> 
>      ID     TITLE     BRANCH            CREATED AT
>      #5560  Loongcpu  niruiyu:loongcpu  about 17 days ago

I didn't see this explicitly mentioned but the easiest way to get to the 
PR if you already have a commit hash/URL like 
https://github.com/tianocore/edk2/commit/032830e is to click the PR link 
next to the branch name at the bottom of the commit message.

In that commit you'll see: "master (#5560)"

Where "#5560" is the PR number and the link to the PR.


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Sat, May 4, 2024 at 1:57 AM Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> On 5/3/2024 4:38 PM, Michael D Kinney wrote:
> >
> >
> >> -----Original Message-----
> >> From: Kinney, Michael D <michael.d.kinney@intel.com>
> >> Sent: Friday, May 3, 2024 1:13 PM
> >> To: Pedro Falcato <pedro.falcato@gmail.com>
> >> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> >> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>;
> >> Kinney, Michael D <michael.d.kinney@intel.com>
> >> Subject: RE: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> >> Review from email to GitHub Pull Requests on 5-24-2024
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Pedro Falcato <pedro.falcato@gmail.com>
> >>> Sent: Friday, May 3, 2024 10:39 AM
> >>> To: Kinney, Michael D <michael.d.kinney@intel.com>
> >>> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> >>> <leif@nuviainc.com>; Andrew Fish (afish@apple.com) <afish@apple.com>
> >>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> >>> Review from email to GitHub Pull Requests on 5-24-2024
> >>>
> >>> On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> >>> <michael.d.kinney@intel.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro
> >>> Falcato
> >>>>> Sent: Thursday, May 2, 2024 10:51 AM
> >>>>> To: devel@edk2.groups.io; Kinney, Michael D
> >>> <michael.d.kinney@intel.com>
> >>>>> Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew
> >>> Fish
> >>>>> (afish@apple.com) <afish@apple.com>
> >>>>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore
> >>> Code
> >>>>> Review from email to GitHub Pull Requests on 5-24-2024
> >>>>>
> >>>>> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> >>>>> <michael.d.kinney=intel.com@groups.io> wrote:
> >>> <snip>
> >>>>>> * All contributors, maintainers, and reviewers must have GitHub
> >>> IDs.
> >>>>>> * The commit message would no longer require Cc:, Reviewed-by:,
> >>> Acked-
> >>>>> by:
> >>>>>>    or Tested-by: tags.  The only required tag would be Signed-
> >> off-
> >>> by.
> >>>>>
> >>>>> I'd just like to note that losing the CC:, Reviewed-by:, etc is a
> >>> big
> >>>>> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's
> >> a
> >>>>> way to pull that off with github actions, but I haven't looked).
> >>> It'll
> >>>>> be a mess if I have to go through online GH PR backlogs just to
> >> find
> >>>>> who to CC/add-to-review. It kills the decentralized bit off of git
> >>> too
> >>>>> :)
> >>>>>
> >>>>
> >>>> Can you provide more details on the impact of the loss?
> >>>
> >>> In my view, commits should be fairly self-describing. What changes,
> >>> why, are obvious, but who looked at it, who reviewed it, who was cc'd
> >>> but didn't respond, who tested are also pretty important. Git is
> >>> supposed to be decentralized, let's not forget. If we ever migrate
> >>> from GH, if GH ever goes down, if the links ever go down, you'll never
> >>> be able to know who looked at it. If you're looking at an EDK2 commit
> >>> deep into an Intel-internal fork, you won't know what "PR #478" is
> >>> (heck, rebase-and-merge doesn't reference PRs either).
> >>>
> >>> Side-note: How are we supposed to find the PR for a given commit?
> >>> Searching doesn't seem to work well. For instance, I picked a random
> >>> non-trivial commit out of the current open PRs:
> >>> MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> >>>
> >> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg
> >>> %2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> >>> has no matches?
> >>
> >> If you have the sha of the commit, you can search in GitHub
> >>
> >> For example, I selected a commit at random from recent edk2 commit
> >> history:
> >>
> >>      https://github.com/tianocore/edk2/commit/032830e96841f2a752e364378c
> >> 3428ac5d2f59d1
> >>
> >> Goto the "Pull Requests" tab for the repo and in the "Filters" search
> >> box enter
> >>
> >>      is:pr is:merged <sha>
> >>
> >> In this example:
> >>
> >>      is:pr is:merged 032830e96841f2a752e364378c3428ac5d2f59d1
> >>
> >> This returns a single hit on PR #5560
> >>
> >>      https://github.com/tianocore/edk2/pull/5560
> >>
> >> There is also a 'gh' command line utility that can be used to write
> >> small scripts to collect this information
> >
> > Here is the equivalent query and output using 'gh' CLI command:
> >
> >      gh pr list --repo tianocore/edk2 --state merged --search 032830e96841f2a752e364378c3428ac5d2f59d1
> >
> >      Showing 1 of 1 pull request in tianocore/edk2 that matches your search
> >
> >      ID     TITLE     BRANCH            CREATED AT
> >      #5560  Loongcpu  niruiyu:loongcpu  about 17 days ago
>
> I didn't see this explicitly mentioned but the easiest way to get to the
> PR if you already have a commit hash/URL like
> https://github.com/tianocore/edk2/commit/032830e is to click the PR link
> next to the branch name at the bottom of the commit message.
>
> In that commit you'll see: "master (#5560)"
>
> Where "#5560" is the PR number and the link to the PR.

Thank you Mike(s), this is useful stuff (and should be written down)!

-- 
Pedro


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Ard Biesheuvel]

This reminds me: would it be possible to keep track of who merged a
PR? (i.e., the person that set the 'push' label)

Currently, commits just appear on the branch with the original author
and the committer field set to something non-descript, e.g.,

commit 275d0a39c42ad73a6e4929822f56f5d8c16ede96 (HEAD -> master,
origin/master, origin/HEAD)
Author:     Gerd Hoffmann <kraxel@redhat.com>
AuthorDate: Fri Mar 1 08:44:00 2024 +0100
Commit:     mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
CommitDate: Fri Mar 1 18:47:27 2024 +0000

which means we cannot tell from the git history which maintainer merged this.


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

That information is in GitHub in the PR conversation.

If you follow the link from the commit to the PR, the PR conversation shows
who set the 'push' label.

Mike

> -----Original Message-----
> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, May 6, 2024 3:01 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; rfc@edk2.groups.io; Leif
> Lindholm <leif@nuviainc.com>; Andrew Fish (afish@apple.com)
> <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> This reminds me: would it be possible to keep track of who merged a
> PR? (i.e., the person that set the 'push' label)
> 
> Currently, commits just appear on the branch with the original author
> and the committer field set to something non-descript, e.g.,
> 
> commit 275d0a39c42ad73a6e4929822f56f5d8c16ede96 (HEAD -> master,
> origin/master, origin/HEAD)
> Author:     Gerd Hoffmann <kraxel@redhat.com>
> AuthorDate: Fri Mar 1 08:44:00 2024 +0100
> Commit:     mergify[bot]
> <37929162+mergify[bot]@users.noreply.github.com>
> CommitDate: Fri Mar 1 18:47:27 2024 +0000
> 
> which means we cannot tell from the git history which maintainer merged
> this.
> 
> 
> 
> 



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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Ard Biesheuvel]

On Mon, 6 May 2024 at 17:11, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> That information is in GitHub in the PR conversation.
>
> If you follow the link from the commit to the PR, the PR conversation shows
> who set the 'push' label.
>

But that is GitHub proprietary metadata, no? Is it not possible to set
the committer field in Git itself to something meaningful?


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

Hi Ard,

Thais is an attribute of Mergify.  I do not see a way to change that
behavior.

I do not know if using the GitHub merge capability or other merge services
provides different behavior here or not.

This specific request is not related to the change to GitHub PRs
for code review.  There is no intention to change the requirement
for a maintainer to set the 'push' label and no intention to change
away from Mergify at this time.

Perhaps we can open your request as an independent request that we
can find an owner to evaluate options and provide recommendations.

Thanks,

Mike



> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, May 6, 2024 8:31 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: rfc@edk2.groups.io; devel@edk2.groups.io; Pedro Falcato
> <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Andrew
> Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> Review from email to GitHub Pull Requests on 5-24-2024
> 
> On Mon, 6 May 2024 at 17:11, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > That information is in GitHub in the PR conversation.
> >
> > If you follow the link from the commit to the PR, the PR conversation
> shows
> > who set the 'push' label.
> >
> 
> But that is GitHub proprietary metadata, no? Is it not possible to set
> the committer field in Git itself to something meaningful?


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Ard Biesheuvel]

On Mon, 6 May 2024 at 17:57, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Thais is an attribute of Mergify.  I do not see a way to change that
> behavior.
>
> I do not know if using the GitHub merge capability or other merge services
> provides different behavior here or not.
>
> This specific request is not related to the change to GitHub PRs
> for code review.  There is no intention to change the requirement
> for a maintainer to set the 'push' label and no intention to change
> away from Mergify at this time.
>
> Perhaps we can open your request as an independent request that we
> can find an owner to evaluate options and provide recommendations.
>

Yep that works for me - it is not an urgent issue anyway, just
something I realized while going through the thread.


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Leara, William via groups.io]

Internal Use - Confidential
On 2024-05-01 18:43, Michael D Kinney wrote:
> Hello,
>
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
>
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.

Very welcome news!

Make it so!


William Leara
BIOS Architect
Dell | BIOS/FW Architecture
office 512-720-5122

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#851): https://urldefense.com/v3/__https://edk2.groups.io/g/rfc/message/851__;!!LpKI!kLrsCBspYuhPcpzu6_oy_nM3IrLm1PLlibITwnmUZRndVxpkUNIpWNxB7UeJUaSNexPOL8sbHixsM2L7Yh5TnNdWdBU$ [edk2[.]groups[.]io] Mute This Topic: https://urldefense.com/v3/__https://groups.io/mt/105848092/7889204__;!!LpKI!kLrsCBspYuhPcpzu6_oy_nM3IrLm1PLlibITwnmUZRndVxpkUNIpWNxB7UeJUaSNexPOL8sbHixsM2L7Yh5TxZFqh5M$ [groups[.]io] Group Owner: rfc+owner@edk2.groups.io
Unsubscribe: https://urldefense.com/v3/__https://edk2.groups.io/g/rfc/unsub__;!!LpKI!kLrsCBspYuhPcpzu6_oy_nM3IrLm1PLlibITwnmUZRndVxpkUNIpWNxB7UeJUaSNexPOL8sbHixsM2L7Yh5T3vhXcBU$ [edk2[.]groups[.]io] [william.leara@dell.com]
-=-=-=-=-=-=-=-=-=-=-=-




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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Brian J. Johnson]

On 5/3/24 12:38, Pedro Falcato wrote:
> On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro Falcato
>>> Sent: Thursday, May 2, 2024 10:51 AM
>>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew Fish
>>> (afish@apple.com) <afish@apple.com>
>>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
>>> Review from email to GitHub Pull Requests on 5-24-2024
>>>
>>> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
>>> <michael.d.kinney=intel.com@groups.io> wrote:
> <snip>
>>>> * All contributors, maintainers, and reviewers must have GitHub IDs.
>>>> * The commit message would no longer require Cc:, Reviewed-by:, Acked-
>>> by:
>>>>    or Tested-by: tags.  The only required tag would be Signed-off-by.
>>>
>>> I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
>>> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
>>> way to pull that off with github actions, but I haven't looked). It'll
>>> be a mess if I have to go through online GH PR backlogs just to find
>>> who to CC/add-to-review. It kills the decentralized bit off of git too
>>> :)
>>>
>>
>> Can you provide more details on the impact of the loss?
> 
> In my view, commits should be fairly self-describing. What changes,
> why, are obvious, but who looked at it, who reviewed it, who was cc'd
> but didn't respond, who tested are also pretty important. Git is
> supposed to be decentralized, let's not forget. If we ever migrate
> from GH, if GH ever goes down, if the links ever go down, you'll never
> be able to know who looked at it. If you're looking at an EDK2 commit
> deep into an Intel-internal fork, you won't know what "PR #478" is
> (heck, rebase-and-merge doesn't reference PRs either).
> 

Well said.  That's my concern as well:  TianoCore won't use GitHub 
forever, and any GitHub metadata (PR numbers, GitHub IDs, bug numbers, 
etc.) will become meaningless once we change.  Never mind that the code 
can be disassociated from the metadata simply by forking to a new 
repository, as Pedro said....

> Side-note: How are we supposed to find the PR for a given commit?
> Searching doesn't seem to work well. For instance, I picked a random
> non-trivial commit out of the current open PRs:
> MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg%2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> has no matches?
> 
>>
>> I am curious how other GitHub projects handle this topic. I see it
> 
> I don't think they do, sadly. But I also don't know many people with a
> positive opinion on GH PRs :)

Yeah... my opinions are decidedly mixed.  They are convenient, but have 
some serious gaps around archiving, auditing, and versioning of review 
requests.  They don't even let you review the commit messages (one of 
their most serious flaws!)

> <snip>
>>> It is sad that we're moving to PRs after I finally got a nice and
>>> sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
>>> it's better than edk2.git's half-email half-PR frankenprocess.
>>> I'd guess this change only encompasses edk2.git? How about the other
>>> repos? Any timeline for those?
>>
>> The plan is to apply this to all repos, one at a time.  Need to get the
>> revised process documented and working in one repo before applying to all.
> 
> Gotcha, thanks!
> 

-- 
Brian J. Johnson
Hewlett Packard Enterprise


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

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael D Kinney]

> -----Original Message-----
> From: Brian J. Johnson <brian.johnson@hpe.com>
> Sent: Friday, May 10, 2024 1:58 PM
> To: rfc@edk2.groups.io; pedro.falcato@gmail.com; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew Fish
> (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review
> from email to GitHub Pull Requests on 5-24-2024
> 
> On 5/3/24 12:38, Pedro Falcato wrote:
> > On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Pedro Falcato
> >>> Sent: Thursday, May 2, 2024 10:51 AM
> >>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> >>> Cc: rfc@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Andrew Fish
> >>> (afish@apple.com) <afish@apple.com>
> >>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> >>> Review from email to GitHub Pull Requests on 5-24-2024
> >>>
> >>> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> >>> <michael.d.kinney=intel.com@groups.io> wrote:
> > <snip>
> >>>> * All contributors, maintainers, and reviewers must have GitHub IDs.
> >>>> * The commit message would no longer require Cc:, Reviewed-by:, Acked-
> >>> by:
> >>>>    or Tested-by: tags.  The only required tag would be Signed-off-by.
> >>>
> >>> I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
> >>> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
> >>> way to pull that off with github actions, but I haven't looked). It'll
> >>> be a mess if I have to go through online GH PR backlogs just to find
> >>> who to CC/add-to-review. It kills the decentralized bit off of git too
> >>> :)
> >>>
> >>
> >> Can you provide more details on the impact of the loss?
> >
> > In my view, commits should be fairly self-describing. What changes,
> > why, are obvious, but who looked at it, who reviewed it, who was cc'd
> > but didn't respond, who tested are also pretty important. Git is
> > supposed to be decentralized, let's not forget. If we ever migrate
> > from GH, if GH ever goes down, if the links ever go down, you'll never
> > be able to know who looked at it. If you're looking at an EDK2 commit
> > deep into an Intel-internal fork, you won't know what "PR #478" is
> > (heck, rebase-and-merge doesn't reference PRs either).
> >
> 
> Well said.  That's my concern as well:  TianoCore won't use GitHub
> forever, and any GitHub metadata (PR numbers, GitHub IDs, bug numbers,
> etc.) will become meaningless once we change.  Never mind that the code
> can be disassociated from the metadata simply by forking to a new
> repository, as Pedro said....

There are github actions that can archive this information in a portable format such as json:

    https://github.com/marketplace/actions/github-archive-action

> 
> > Side-note: How are we supposed to find the PR for a given commit?
> > Searching doesn't seem to work well. For instance, I picked a random
> > non-trivial commit out of the current open PRs:
> > MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> >
> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg%2FBu
> s%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> > has no matches?
> >
> >>
> >> I am curious how other GitHub projects handle this topic. I see it
> >
> > I don't think they do, sadly. But I also don't know many people with a
> > positive opinion on GH PRs :)
> 
> Yeah... my opinions are decidedly mixed.  They are convenient, but have
> some serious gaps around archiving, auditing, and versioning of review
> requests.  They don't even let you review the commit messages (one of
> their most serious flaws!)

Can you propose a process to provide review comments on the commit messages
within a PR conversation?  Perhaps use of keywords/tags/links to indicate the
commit being discussed?  Could copy/paste commit message being reviewed into
PR conversation and provide feedback there.

> 
> > <snip>
> >>> It is sad that we're moving to PRs after I finally got a nice and
> >>> sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
> >>> it's better than edk2.git's half-email half-PR frankenprocess.
> >>> I'd guess this change only encompasses edk2.git? How about the other
> >>> repos? Any timeline for those?
> >>
> >> The plan is to apply this to all repos, one at a time.  Need to get the
> >> revised process documented and working in one repo before applying to all.
> >
> > Gotcha, thanks!
> >
> 
> --
> Brian J. Johnson
> Hewlett Packard Enterprise


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

Re: [edk2-devel] [edk2-rfc] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Rebecca Cran]

There's a GitHub Project for this at 
https://github.com/orgs/tianocore/projects/5 which it looks like Michael 
Kubacki created last year. I suspect it's obsolete though, since it 
looks like the tasks haven't been updated recently.


-- 

Rebecca Cran


On 5/1/24 11:43 AM, Michael D Kinney wrote:
> Hello,
>
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
>
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.
>
> Updates to the following Wiki page would be required to describe the
> required process when using GitHub Pull Requests for all code review
> related activity.
>
>      https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> A couple examples of the changes that would need to be documented are:
>
> * All contributors, maintainers, and reviewers must have GitHub IDs.
> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>    or Tested-by: tags.  The only required tag would be Signed-off-by.
> * The Pull Request submitter is required to invite the required
>    maintainers and reviewers to the pull request. This is the same
>    set of maintainers and reviewers that are required to be listed in
>    Cc: tags in today's process.
> * Maintainers are responsible for verifying that all conversations in
>    the code review are resolved and that all review approvals from the
>    required set of maintainers are present before setting the 'push' label.
>
>
> Please provide feedback
> 1) If you are not in favor of this change.
> 2) If you are not in favor of the proposed date of this change.
> 3) On the process changes you would like to see documented in the Wiki
>     pages related to using GitHub Pull Request based code reviews.
>
> There is some prototype work to automate/simplify some of the PR based
> code review process steps. Those could be added over time as resources
> are available to finish and support them.
>
> Best regards,
>
> Mike


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

Re: [edk2-devel] [edk2-rfc] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Michael Kubacki]

The transition now is simpler and more streamlined than what is 
described there. I'll update the project to reflect the current state so 
it can serve as a reference and guide related work in the future.

Thanks,
Michael

On 5/24/2024 8:20 AM, Rebecca Cran wrote:
> There's a GitHub Project for this at 
> https://github.com/orgs/tianocore/projects/5 which it looks like Michael 
> Kubacki created last year. I suspect it's obsolete though, since it 
> looks like the tasks haven't been updated recently.
> 
> 


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