Move blessed mergable PRs into new column, fix blessed PRs with partial approvals #1046
Conversation
|
|
||
| const blessedColumnNames = [ | ||
| "Waiting for Code Reviews (Blessed)", | ||
| "Waiting for Author to Merge (Blessed)", |
There was a problem hiding this comment.
does the weekly maintainer have to know which (Blessed) column to move to?
There was a problem hiding this comment.
No, they should only ever put something into the first one, since I removed the "straight to mergable" thing.
|
The PR's not yet complete; working on a fix for the blessed PRs overriding any other reviews thing. |
| const approvedBy = getApprovedBy(); | ||
| const pendingCriticalPackages = getPendingCriticalPackages(); | ||
| const approverKind = getApproverKind(); |
There was a problem hiding this comment.
Dropped some of these to make them less of a hazard for code below; a lot of code was incorrectly reading out reviews for one package and applying them to the whole.
jakebailey
changed the title
The use is just: If there's a PR in "needs maintainer review" and you think it's not doing anything harmful such that an owner can make the final decision, move the PR to "Waiting for Code Reviews (blessed)". If all authors approve, it will eventually move to "Waiting for Merge (Blessed)" and be eligible for a self merge. If someone at any point complains, it leaves the "blessed" part of the flow, dropping back into "Needs Author Action", or eventually back into the maintainer queue. |
packages/mergebot/src/basic.ts
Outdated
| @@ -53,6 +67,12 @@ export const labelNames = [ | |||
|
|
|||
| export type ApproverKind = "maintainer" | "owner" | "other"; | |||
There was a problem hiding this comment.
stupid idea (because the code is already good enough): why not export type ApproverKind = (typeof approverKindOrder)[number] ?
but also: numeric enums give you all the features you need with enum ApproverKind { None, Other, Owner, Maintainer } to simplify getMaxApproverKind.
There was a problem hiding this comment.
I'll do the former one, but I was avoiding the latter because the ApproverKind is a part of baselines and will end up showing up in the dumped JSON as regular numbers.
The new test reveals that blessing also bypasses needing all owners of modified packages to approve. Working on figuring that one out.