Process/people to merge PRs
directionless opened this issue · 8 comments
Some discussion about what kind of process we want to merge PRs.
Some things I've heard:
- Give active devs commit bits
- use CODEOWNERS
- Require N accepts to merge
My personal bias is to CODEOWNERS for subject matter experts
This is a nitpick but I suggest tracking code and feature owners in CONTRIBUTING.
I also suggest PRs require at least 1 person to accept before merging. This prevents me from mistakenly merging my own PR.
I also suggest tracking those with Write access to be labeled in CONTRIBUTING. This makes the list transparent and allows for discussion of add/remove in the accompanying PR.
The criteria for “active dev” can remain subjective with a vibe of “within the last 2yrs” and “has made significant code contributions”. I see significant as 3-5+ changes that are beyond bug fixes.
I'm +1 on relying on CODEOWNERS to help with commit/review rights.
I'd like to see N+2 for reviews before merge, where you get a LGTM from a CODEOWNER and a maintainer. The reason I'm suggesting this specific scheme is that I'd like to see some way to notify users with domain knowledge about specific areas. For example, I'd like to be pinged for changes to tables that affect macOS table implementation.
I like the idea of tracking various things, but I'm a little leery of trying to maintain user lists in multiple places. On the other hand, CODEOWNERS supports using the github groups. So it might end up being pretty simple.
I like N+2
Since there are not a lot of osquery developers compared to users right now I think N==1 is a good starting point. Once this cadence is established (e.g., people follow up and accept quickly) then N can be increased to 2.
From the meeting today, it sounds like:
- there's general support for CODEOWNERS
- We can't, initially, break things down into areas-of-expertise
- N=1 is probably fine to start
I personally advocate we use github groups, not explicit enumeration in CODEOWNERS
I created https://github.com/orgs/osquery/teams/osquery-committers and osquery/osquery#5603
With those changes (and some more button pushing) I believe this provides a simple place to add new committers. Frustratingly, it does not solve getting them a good process to be added to that team.
I have merged in that CODEOWNERS, and I have checked the various boxes in branch protections. Now we see how it works!
I like tracking people in CONTRIBUTING, but I also worry about divergence from the github group. We'll figure it out.