96 lines
4.2 KiB
Markdown
96 lines
4.2 KiB
Markdown
# PR#75: An Approval Counts Only After the Result
|
|
|
|
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR documentation is not maintained along with new PRs; treat it as a snapshot.
|
|
|
|
## Related Links
|
|
|
|
- [PR#73](https://git.javagil.de/mi/werkrevisor/pulls/73): merged with a finding that its approver had not seen.
|
|
- [PR#74](https://git.javagil.de/mi/werkrevisor/pulls/74): the fix of that finding.
|
|
|
|
## The Problem
|
|
|
|
The user approved the head of PR#73 while werkrevisor still reviewed it.
|
|
The review then gave ORANGE for a security finding.
|
|
The approval was for that head, so the gate passed, and Gitea merged the scheduled merge at once.
|
|
The approver had not seen the finding.
|
|
|
|
## Non-Goals
|
|
|
|
- No change for GREEN, which needs no human approval, nor for RED, which blocks anyway.
|
|
- No new trigger: Werkator still starts a review only for a new commit.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a human approves what they have read
|
|
|
|
#### Scenario#75.01: an approval before the result is asked for again
|
|
|
|
So that a human approves only findings they could read.
|
|
|
|
- **Given** the gate mode `approval`
|
|
- **And** a human approved a head before its result comment
|
|
- **When** werkrevisor finishes the review of that head with YELLOW or ORANGE
|
|
- **Then** it requests a review from that human again
|
|
- **And** the result comment asks them to approve again after reading it
|
|
|
|
##### Verified by
|
|
|
|
- [test_approval_after_result.ApprovalAfterResultTest.test_an_approval_before_the_result_is_asked_for_again](../../tests/test_approval_after_result.py)
|
|
|
|
#### Scenario#75.02: an approval after the result unlocks the merge
|
|
|
|
So that the second approval merges the PR without a new review.
|
|
|
|
- **Given** a YELLOW or ORANGE result comment
|
|
- **And** a human approved the head after that comment
|
|
- **When** werkrevisor runs again for that head
|
|
- **Then** the gate names that approval
|
|
|
|
##### Verified by
|
|
|
|
- [test_approval_after_result.ApprovalAfterResultTest.test_an_approval_after_the_result_counts](../../tests/test_approval_after_result.py)
|
|
|
|
#### Scenario#75.03: an approval before the result is not carried over
|
|
|
|
So that a rebase does not turn an approval without the findings into a valid one.
|
|
|
|
- **Given** a human approved a head before its result comment
|
|
- **And** a push dismissed that approval
|
|
- **When** werkrevisor reviews the rebased head
|
|
- **Then** it does not carry that approval over
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_an_approval_before_the_result_of_its_head_is_not_carried_over](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#75.04: the other gate modes count no approval from before the review
|
|
|
|
So that `block` and `pass_with_review_request` do not unlock findings that nobody has read either.
|
|
|
|
- **Given** the gate mode `block` or `pass_with_review_request`
|
|
- **And** a human approved a head before its result comment
|
|
- **When** werkrevisor finishes the review of that head with YELLOW or ORANGE
|
|
- **Then** the approval does not unlock the gate
|
|
- **And** werkrevisor requests a review from that human again
|
|
|
|
##### Verified by
|
|
|
|
- [test_approval_after_result.OtherGateModesTest.test_block_does_not_count_an_approval_from_before_the_review](../../tests/test_approval_after_result.py)
|
|
- [test_approval_after_result.OtherGateModesTest.test_pass_with_review_request_asks_again](../../tests/test_approval_after_result.py)
|
|
- [test_approval_after_result.OtherGateModesTest.test_block_names_a_failed_request](../../tests/test_approval_after_result.py)
|
|
|
|
## The Solution
|
|
|
|
- A human approval counts only when Gitea recorded it after the result comment of its head, in all three gate modes.
|
|
With `block` and `pass_with_review_request` the result comment comes after the gate decision,
|
|
so a first review counts no approval at all.
|
|
- For an earlier approval, the bot requests a review from that human again.
|
|
A test on Gitea 1.27.3 in `mi/werkrevisor-bench` showed what follows:
|
|
the earlier approval loses its `official` flag, a merge fails with "Does not have enough approvals",
|
|
and the next approval merges the scheduled merge within about 5 seconds.
|
|
- An approval of an earlier head is carried over only when it came after the result comment of that head.
|
|
- The README explains it under "Approval mode".
|
|
- Version 0.32.2.
|