Re: [edk2-rfc] [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: 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]
-=-=-=-=-=-=-=-=-=-=-=-