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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-