Patch Triage: Difference between revisions

From Yocto Project
Jump to navigationJump to search
(Created page with "I propose that we add a new meeting to https://www.yoctoproject.org/public-virtual-meetings/, similar to the Bug Triage meeting but to review incoming patches. The patch queue...")
 
No edit summary
 
(3 intermediate revisions by the same user not shown)
Line 1: Line 1:
I propose that we add a new meeting to https://www.yoctoproject.org/public-virtual-meetings/, similar to the Bug Triage meeting but to review incoming patches. The patch queue is long and whilst we talk about how patches-on-list means anyone can perform drive-by review, there’s no way to know if someone has actually reviewed a patch and mentally said “this looks good”, or if several people have seen a patch and been sceptical but not actually posted their thoughts. This proposal aims to counter that problem.
The Yocto Project community is experimenting with a regular patchwork triage meeting; to triage, review, and assign incoming patches.


We have a patchwork instance at patchwork.yoctoproject.org which has the ability to track patches, their state, and assign them to a specific person. This is the bare minimum of a patch triage process, we just need to use it.  I admit that what I’m proposing here is inspired by the glibc project process (sourceware.org/glibc/wiki/PatchworkReviewMeetings), because I learnt about their process whilst attending the GNU Cauldron conference.
Everyone is welcome to attend, the call is held on Thursdays at 08:30 - 09:30 PST (the hour after the Bug Triage call).  Other meetings may be added to cater for other timezones or if there are more patches than can be triaged.


As we have a fairly constant stream of patches, I expect these triage meetings should be done twice weekly, to allow for rapid feedback and review cycles. Each meeting would be an hour long: the first half spent quickly reviewing as much of the patch queue as possible with any discussion postponed until the second half. This means the attendees can hopefully work through a large number of patches quickly before circling back on the more interesting patches. Any patches which result in a bigger discussion or have open questions should likely be discusseed in the Engineering Sync, where the attendence will be larger.  As patchwork is primarily a read-only system (apart from status and delegation), a wiki page will be needed to keep minutes and notes.
The focus of the meeting is to triage new patches in patchwork: https://patchwork.yoctoproject.org/project/oe-core/list/
I propose the process to be as follows:


1. Obtain last patch ID reviewed from the meeting minutes
=== Process ===


2. Iterate through the patches since the last patch reviewed, oldest first, and review. Quickly review as a group and assign a delegate and provisional status:
This process is inspired by the process the [https://sourceware.org/glibc/wiki/PatchworkReviewMeetings glibc project] use.
– Looks Good: move to Under Review status. Will be incorporated into a staging branch by the delegate so that if it fails testing the status can be changed.
– Looks Bad: move to Rejected status. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
– Needs Work (quick): move to Action Required. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
– Needs Further Review. Move to Action Required. Note this choice in the minutes, and either discuss later in the call or in the Engineering Sync. It is the responsiblity of the delegate to have this discussion.


3. Discuss any patches marked a Needs Further Review, and any other patches that the group wish to discuss.
# Obtain last patch ID reviewed from the minutes
# Iterate through the patches since the last patch reviewed, oldest first. Quickly review as a group and assign a delegate and provisional status:
** Looks Good: move to Under Review status. Will be incorporated into a staging branch by the delegate so that if it fails testing the status can be changed.
** Looks Bad: move to Rejected status. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
** Needs Work (quick): move to Action Required. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
** Needs Further Review. Move to Action Required. Note this choice in the minutes, and either discuss later in the call or in the Engineering Sync. It is the responsiblity of the delegate to have this discussion.
# Discuss any patches marked a Needs Further Review, and any other patches that the group wish to discuss.
# Review the state of all patches that have been delegated but not yet resolved.
# Note down in the minutes the last patch ID reviewed, for the next meeting.


4. Review the state of all patches that have been delegated but not yet resolved.


5. Note down in the minutes the last patch ID reviewed, for the next meeting.
=== Patch State Summary ===


As a straw-man proposal, I suggest Monday and Thursday for the calls. The hour slot immedatiately after bug triage?
{{#mermaid:flowchart TD
    Start(Start) --> New[New]
 
    New-- RFC, no further action needed -->RFC(Request for Comments)
 
    New-- Patch will not be accepted -->Rejected(Rejected)
 
    New-- Patch looks okay,<br>delegate commits to review -->UR[Under Review]
 
    UR-- Patch good,<br>can be merged -->Accepted(Accepted)
 
    UR-- Patch needs work,<br>feedback sent -->Change[Change Requested]
 
    Change-- Improved patch posted -->Superseded(Superseded)
 
    UR-- Patch needs further discussion -->New
 
    New-- Submitter has sent<br>an improved patch -->Superseded
}}
 
* New: Nobody has reviewed it yet
* Under Review: The patch is under review and testing. Change the delegate to the reviewer.
* Accepted: There is consensus for merging this patch.
* Rejected:  There is consensus for not merging this patch.
* RFC: The patch is a Request for Comments to solicit feedback and doesn't need to be tracked further.
* Change Requested: The patch has been reviewed and needs further work.
* Superseded - A newer version of the patch exists.
* Deferred - Review of this patch is waiting for another event to happen.
 
'''TODO''' There is ambiguity around the "Accepted" state. Should this indicate a patch which ''is ready to be merged'', or only for patches that ''have already been merged''? Glibc has a "committed" state to handle this.
 
'''TODO''' Now thinking Under Review until merged (accepted) or rejected (rejected).
 
=== Minutes ===
 
==== 2023-10-12 ====
 
...

Latest revision as of 10:19, 13 October 2023

The Yocto Project community is experimenting with a regular patchwork triage meeting; to triage, review, and assign incoming patches.

Everyone is welcome to attend, the call is held on Thursdays at 08:30 - 09:30 PST (the hour after the Bug Triage call). Other meetings may be added to cater for other timezones or if there are more patches than can be triaged.

The focus of the meeting is to triage new patches in patchwork: https://patchwork.yoctoproject.org/project/oe-core/list/

Process

This process is inspired by the process the glibc project use.

  1. Obtain last patch ID reviewed from the minutes
  2. Iterate through the patches since the last patch reviewed, oldest first. Quickly review as a group and assign a delegate and provisional status:
    • Looks Good: move to Under Review status. Will be incorporated into a staging branch by the delegate so that if it fails testing the status can be changed.
    • Looks Bad: move to Rejected status. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
    • Needs Work (quick): move to Action Required. Note the feedback in the minutes, the delegate is responsible for responding on the list with this feedback.
    • Needs Further Review. Move to Action Required. Note this choice in the minutes, and either discuss later in the call or in the Engineering Sync. It is the responsiblity of the delegate to have this discussion.
  1. Discuss any patches marked a Needs Further Review, and any other patches that the group wish to discuss.
  2. Review the state of all patches that have been delegated but not yet resolved.
  3. Note down in the minutes the last patch ID reviewed, for the next meeting.


Patch State Summary

  • New: Nobody has reviewed it yet
  • Under Review: The patch is under review and testing. Change the delegate to the reviewer.
  • Accepted: There is consensus for merging this patch.
  • Rejected: There is consensus for not merging this patch.
  • RFC: The patch is a Request for Comments to solicit feedback and doesn't need to be tracked further.
  • Change Requested: The patch has been reviewed and needs further work.
  • Superseded - A newer version of the patch exists.
  • Deferred - Review of this patch is waiting for another event to happen.

TODO There is ambiguity around the "Accepted" state. Should this indicate a patch which is ready to be merged, or only for patches that have already been merged? Glibc has a "committed" state to handle this.

TODO Now thinking Under Review until merged (accepted) or rejected (rejected).

Minutes

2023-10-12

...

Retrieved from "https://wiki.yoctoproject.org/wiki/index.php?title=Patch_Triage&oldid=85987"