CI: Enforce Squid Project review process voting

[yadij]

Implement the Squid Project merge procedure voting criteria
documented at https://wiki.squid-cache.org/MergeProcedure#votes
as a github action that can be applied as a merge requirement.

This will make the Squid merge procedure voting apply to prevent
Anubis bot attempting to start merge checks before approval limits,
as well as disabling github UI buttons.

Also, currently we rely on Anubis bot to enforce a 48hr hold. This
hold is added to the voting criteria checked.

[rousskov]

At an implementation level, I am against introducing a hard-coded 48-hour delay for unanimously approved PRs. At a design level, we should avoid splitting complex PR merge management tasks between Anubis and GitHub Actions.

As always, I am happy to collaborate on changing what invariants Anubis should enforce.

PR description: Currently we rely on Anubis bot to enforce the review hold. The
bot has not been completely reliable at this task - at times it has merged new code within 48hr of commits to edited PRs.

This statement is false or misleading -- Anubis has reliably enforced the "review hold" that it was tasked with enforcing. We can discuss changes to how the corresponding hold should be defined, but we should not accuse Anubis of being unreliable in this context.

And there has been no application of the limit to github UI buttons (allowing manual commit at any time).

PRs that still require approval are reflected in GitHub UI via non-green "PR approval" check managed by Anubis. Adding another non-green check would not change much in this context as long as both checks are in sync. And adding a new PR approval check that is not in sync with the existing "PR approval" check is a bad idea for other obvious reasons. Again, if desired, we can change the definition of "approved PR", of course. Please propose a new definition if you think these changes are warranted; we can discuss how to implement the new definition once we agree on what that new definition should be.

P.S. If the goal is to block core developers from merging PRs prematurely, then we can also enhance Anubis to block otherwise-all-green PRs until their staging checks are complete. We can enlarge the existing "PR approval" check to cover more than just PR approvals OR we can add another check dedicated to staging tests. IMO, it is better to trust core developers not to merge manually instead, but I do not insist on that simple/cheap strategy. I am happy to collaborate on related improvements.

[yadij]

At an implementation level, I am against introducing a hard-coded 48-hour delay for unanimously approved PRs.

Ah, good catch. Now added overrides for that and for reviewer vetos.
Right now this check is failing because you have a "request changes".

We can further test it works correctly once you clear your review to approved, and if kinkie also approves within 48hrs of now we can trace the override for unanimous approval.

At a design level, we should avoid splitting complex PR merge management tasks between Anubis and GitHub Actions.

As always, I am happy to collaborate on changing what invariants Anubis should enforce.

PR description: Currently we rely on Anubis bot to enforce the review hold. The
bot has not been completely reliable at this task - at times it has merged new code within 48hr of commits to edited PRs.

This statement is false or misleading -- Anubis has reliably enforced the "review hold" that it was tasked with enforcing. We can discuss changes to how the corresponding hold should be defined, but we should not accuse Anubis of being unreliable in this context.

Like I wrote. There have been times when it merged PRs early and dropped last minute changes the author had made.

FTR; The specific case seems to be:

1. commit X is pushed to the PR
2. reviewers immediately approve changes in commits A..X
3. Anubis begins merge procedure using "auto" branch
4. additional commits Y..Z arrive on PR branch
5. Anubis completes merge procedure for commit A..X
6. Anubis closes the PR.

These cases were only spotted when I went back to clean up my local branches found that github was holding references to closed PR work. Luckily these Y..Z commits tend to be docs or style polish right at the end and did not matter. But the fact that they can occur at all is a worry.

P.S. If the goal is to block core developers from merging PRs prematurely, then we can also enhance Anubis to block otherwise-all-green PRs until their staging checks are complete. We can enlarge the existing "PR approval" check to cover more than just PR approvals OR we can add another check dedicated to staging tests. IMO, it is better to trust core developers not to merge manually instead, but I do not insist on that simple/cheap strategy. I am happy to collaborate on related improvements.

The goal is to make github UI perform more inline with what the Squid Project procedure requires. That should be matching what Anubis does, though not always - as I have mentioned there are edge cases where Anubis does what you wanted to implement, not what the Project procedure documents as requirements.

[rousskov]

There have been times when it merged PRs early and dropped last minute changes the author had made.

FTR; The specific case seems to be:

1. commit X is pushed to the PR 
2. reviewers immediately approve changes in commits A..X 
3. Anubis begins merge procedure using "auto" branch 
4. additional commits Y..Z arrive on PR branch 
5. Anubis completes merge procedure for commit A..X 
6. Anubis closes the PR.

Anubis is supposed to (and has code to) abort merging process after event 4. I believe I have seen Anubis doing that. If you have a specific PR where that abort did not happen, please share that PR number with me. If confirmed, we will need to investigate and fix that bug! That failure may be too old to investigate now; please alert me if you spot this behavior again.

---

The goal is to make github UI perform more inline with what the Squid Project procedure requires. That should be matching what Anubis does, though not always - as I have mentioned there are edge cases where Anubis does what you wanted to implement, not what the Project procedure documents as requirements.

I disagree that Anubis goes against Project requirements. We should discuss any specific cases and reach an agreement before proceeding with this or similar PRs (or related Anubis changes)!

[kinkie]

I like this. It's clear and each individual step is documented separately.

[yadij]

@rousskov AFAICS the issues you brought up about this PR change in earlier reviews have all been resolved. The remaining discussion is all about Anubis, which is not something this PR can alter. Please re-review the proposed change for technical issues (adherence to the documented voting procedure).

@kinkie some updates have changed the code. please re-review.

[kinkie]

Small but meaningful change requests

[kinkie]

Why not more often? Say every 4 hours or so?

[yadij]

The criteria calculations have a 1-day resolution. So it is not worth doing more than once per day after the initial check.

Issues resolved

[yadij]

Clearing this up for merge since issues were all resolved months ago with no further feedback.

[rousskov]

yadij yadij dismissed stale reviews from kinkie and rousskov 9 hours ago

@yadij, you do not have the right to dismiss negative votes of other core developers. Doing so violates fundamental Project protections, hurting the Project on multiple levels. I have asked you to stop doing that on multiple occasions. I have reverted the corresponding master branch commit 58b2109.

Let's discuss how to move forward from here. I am opening a thread on the Board mailing list.

[yadij]

yadij yadij dismissed stale reviews from kinkie and rousskov 9 hours ago

@yadij, you do not have the right to dismiss negative votes of other core developers.

1. See CI: Enforce Squid Project review process voting #2067 (comment). Per https://wiki.squid-cache.org/MergeProcedure#votes item 1, only blocks until resolved.

2. After waiting 3 months for any type of response, I applied https://wiki.squid-cache.org/MergeProcedure#exceptions item 1 to clear the merge queue item.

3. You applied https://wiki.squid-cache.org/MergeProcedure#exceptions item 2 to revert.

[yadij]

Oh, and Anubis did not wait the required 48hr hold period between my change c4ce4ee and starting the merge checks with auto branch.
Per Squid Project policy you are supposed to have 48hrs to re-review and/or clear my merge label.

[rousskov]

yadij yadij dismissed stale reviews from kinkie and rousskov 9 hours ago

@yadij, you do not have the right to dismiss negative votes of other core developers.

Per https://wiki.squid-cache.org/MergeProcedure#votes item 1, only blocks until resolved.

It has not been resolved. Only the blocking person can determine whether their blocking concerns have been resolved. That has not happened here. You do not have the right to make that determination for other core developers.

Everything that happened after that fundamental principle was violated is secondary. It is not worth discussing until that principle is upheld -- that principle is the necessary foundation for any such discussion.

We should continue this discussion on the Board mailing list though; there is a dedicated email thread for that.