Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
> -Original Message- > From: Brian J. Johnson > Sent: Friday, May 10, 2024 1:58 PM > To: r...@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > Cc: devel@edk2.groups.io; Leif Lindholm ; Andrew Fish > (af...@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 > > wrote: > >> > >> > >> > >>> -Original Message- > >>> From: r...@edk2.groups.io On Behalf Of Pedro Falcato > >>> Sent: Thursday, May 2, 2024 10:51 AM > >>> To: devel@edk2.groups.io; Kinney, Michael D > >>> Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish > >>> (af...@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 > >>> wrote: > > > * 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. > > > > >>> 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 wrote: -Original Message- From: r...@edk2.groups.io On Behalf Of Pedro Falcato Sent: Thursday, May 2, 2024 10:51 AM To: devel@edk2.groups.io; Kinney, Michael D Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish (af...@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 wrote: * 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!) 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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+ow...@edk2.groups.io Unsubscribe: https://urldefense.com/v3/__https://edk2.groups.io/g/rfc/unsub__;!!LpKI!kLrsCBspYuhPcpzu6_oy_nM3IrLm1PLlibITwnmUZRndVxpkUNIpWNxB7UeJUaSNexPOL8sbHixsM2L7Yh5T3vhXcBU$ [edk2[.]groups[.]io] [william.le...@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/21656 Group Owner: develꌉ@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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:57, Kinney, Michael D 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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 > Sent: Monday, May 6, 2024 8:31 AM > To: Kinney, Michael D > Cc: r...@edk2.groups.io; devel@edk2.groups.io; Pedro Falcato > ; Leif Lindholm ; Andrew > Fish (af...@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 > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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: r...@edk2.groups.io On Behalf Of Ard > Biesheuvel > Sent: Monday, May 6, 2024 3:01 AM > To: devel@edk2.groups.io; Kinney, Michael D > Cc: Pedro Falcato ; r...@edk2.groups.io; Leif > Lindholm ; Andrew Fish (af...@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 > 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 + > > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 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 + 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On Sat, May 4, 2024 at 1:57 AM Michael Kubacki wrote: > > On 5/3/2024 4:38 PM, Michael D Kinney wrote: > > > > > >> -Original Message- > >> From: Kinney, Michael D > >> Sent: Friday, May 3, 2024 1:13 PM > >> To: Pedro Falcato > >> Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm > >> ; Andrew Fish (af...@apple.com) ; > >> Kinney, Michael D > >> 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 > >>> Sent: Friday, May 3, 2024 10:39 AM > >>> To: Kinney, Michael D > >>> Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm > >>> ; Andrew Fish (af...@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 > >>> wrote: > > > > > -Original Message- > > From: r...@edk2.groups.io On Behalf Of Pedro > >>> Falcato > > Sent: Thursday, May 2, 2024 10:51 AM > > To: devel@edk2.groups.io; Kinney, Michael D > >>> > > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew > >>> Fish > > (af...@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 > > wrote: > >>> > >> * 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 > >> > >> 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 BRANCHCREATED 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.
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On 5/3/2024 4:38 PM, Michael D Kinney wrote: -Original Message- From: Kinney, Michael D Sent: Friday, May 3, 2024 1:13 PM To: Pedro Falcato Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm ; Andrew Fish (af...@apple.com) ; Kinney, Michael D 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 Sent: Friday, May 3, 2024 10:39 AM To: Kinney, Michael D Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm ; Andrew Fish (af...@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 wrote: -Original Message- From: r...@edk2.groups.io On Behalf Of Pedro Falcato Sent: Thursday, May 2, 2024 10:51 AM To: devel@edk2.groups.io; Kinney, Michael D Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish (af...@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 wrote: * 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 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 BRANCHCREATED 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
> -Original Message- > From: Kinney, Michael D > Sent: Friday, May 3, 2024 1:13 PM > To: Pedro Falcato > Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm > ; Andrew Fish (af...@apple.com) ; > Kinney, Michael D > 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 > > Sent: Friday, May 3, 2024 10:39 AM > > To: Kinney, Michael D > > Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm > > ; Andrew Fish (af...@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 > > wrote: > > > > > > > > > > > > > -Original Message- > > > > From: r...@edk2.groups.io On Behalf Of Pedro > > Falcato > > > > Sent: Thursday, May 2, 2024 10:51 AM > > > > To: devel@edk2.groups.io; Kinney, Michael D > > > > > > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew > > Fish > > > > (af...@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 > > > > wrote: > > > > > > > * 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 > > 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 BRANCHCREATED 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 :) > > > > > > > > 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
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 > Sent: Friday, May 3, 2024 10:39 AM > To: Kinney, Michael D > Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm > ; Andrew Fish (af...@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 > wrote: > > > > > > > > > -Original Message- > > > From: r...@edk2.groups.io On Behalf Of Pedro > Falcato > > > Sent: Thursday, May 2, 2024 10:51 AM > > > To: devel@edk2.groups.io; Kinney, Michael D > > > > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew > Fish > > > (af...@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 > > > wrote: > > > > > * 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 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 :) > > > > > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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: r...@edk2.groups.io On Behalf Of Michael > Kubacki > Sent: Friday, May 3, 2024 10:22 AM > To: devel@edk2.groups.io; Kinney, Michael D > ; quic_llind...@quicinc.com; > marcin.juszkiew...@linaro.org; r...@edk2.groups.io > Cc: Leif Lindholm ; Andrew Fish (af...@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:
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 wrote: > > > > > -Original Message- > > From: r...@edk2.groups.io On Behalf Of Pedro Falcato > > Sent: Thursday, May 2, 2024 10:51 AM > > To: devel@edk2.groups.io; Kinney, Michael D > > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish > > (af...@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 > > wrote: > > > * 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 :) > > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [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 Sent: Thursday, May 2, 2024 8:21 AM To: devel@edk2.groups.io; quic_llind...@quicinc.com; marcin.juszkiew...@linaro.org; Kinney, Michael D ; r...@edk2.groups.io Cc: Leif Lindholm ; Andrew Fish (af...@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
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On 5/2/2024 12:37 PM, Kinney, Michael D wrote: -Original Message- From: Leif Lindholm Sent: Thursday, May 2, 2024 3:57 AM To: devel@edk2.groups.io; mikub...@linux.microsoft.com; Kinney, Michael D ; r...@edk2.groups.io Cc: Andrew Fish (af...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
> -Original Message- > From: r...@edk2.groups.io On Behalf Of Pedro Falcato > Sent: Thursday, May 2, 2024 10:51 AM > To: devel@edk2.groups.io; Kinney, Michael D > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish > (af...@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 > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [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 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
> -Original Message- > From: Leif Lindholm > Sent: Thursday, May 2, 2024 3:57 AM > To: devel@edk2.groups.io; mikub...@linux.microsoft.com; Kinney, Michael > D ; r...@edk2.groups.io > Cc: Andrew Fish (af...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
> -Original Message- > From: r...@edk2.groups.io On Behalf Of Brian J. > Johnson > Sent: Thursday, May 2, 2024 8:59 AM > To: devel@edk2.groups.io; dionnagl...@google.com; > quic_llind...@quicinc.com > Cc: Kinney, Michael D ; r...@edk2.groups.io; > Andrew Fish (af...@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 > > 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
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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 > Sent: Thursday, May 2, 2024 8:21 AM > To: devel@edk2.groups.io; quic_llind...@quicinc.com; > marcin.juszkiew...@linaro.org; Kinney, Michael D > ; r...@edk2.groups.io > Cc: Leif Lindholm ; Andrew Fish (af...@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 >
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On Thu, May 2, 2024 at 8:59 AM Brian J. Johnson 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 > > 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
Re: [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 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 (#118523): https://edk2.groups.io/g/devel/message/118523 Mute This Topic: https://groups.io/mt/105847510/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On 5/2/2024 5:37 AM, Ard Biesheuvel wrote: On Wed, 1 May 2024 at 19:44, 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. +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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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). 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On Wed, 1 May 2024 at 19:44, 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. > +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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
On Wed, May 1, 2024 at 11:12 AM Leif Lindholm via 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
