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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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