Proposal: Require re-review before integration if the PR is modified

[Jorn Vernee]

I'd like to also point out that: you are not required to push the merge 
to your PR branch in order to have github actions run for it. The github 
actions run for any branch that gets pushed to your fork (if you have 
them enabled), the Skara bots just fetch that information when the 
branch is used for a PR.

So, in other words, you could create a new local branch from your PR 
branch, do the merge on the new branch, and push that to your fork, 
creating a new branch in your fork with the merge. Github actions would 
run for the new branch, but this wouldn't modify the PR branch, so it 
wouldn't require a re-review under the proposed change.

Jorn

On 25-6-2024 11:19, erik.joelsson at oracle.com wrote:
> On 6/24/24 06:38, Thomas Stüfe wrote:
>> One question, do merges with master cause the Skara reviewer check to 
>> fail?
>>
>> We try to encourage authors to merge often, especially before 
>> pushing. Patches should be based on a reasonably fresh copy of 
>> master. I believe the contribution guidelines state that too, and it 
>> is just good practice.
>>
> Skara does not have a way to distinguish clean merges from the target 
> branch from other changes when determining if a review is stale. It 
> simple compares the hash the review was performed at with the current 
> head hash of the PR source branch.
>
> Implementing such a check may be possible in the future, but it's not 
> something we can promise to happen.
>
> /Erik
>
>> Cheers, Thomas
>>
>> On Tue, Jun 18, 2024 at 6:00 PM Iris Clark <iris.clark at oracle.com> wrote:
>>
>>     Hi.
>>
>>     Every PR must be reviewed by at least one Reviewer [0] before
>>     it's integrated
>>     into the main-line repository [1].  While we recommend that the
>>     PR contain the
>>     final version of the change at creation time, this is not always
>>     feasible,
>>     particularly for large or complex changes.  The integrated
>>     version of the PR
>>     may evolve significantly from when the review was done. Thus the
>>     final
>>     commit's "Reviewed-by" field may give false confidence that the
>>     complete set
>>     of changes has been reviewed before it was integrated into the
>>     repository.
>>     This could, in the worst case, allow an adversary to commit
>>     malicious code.
>>
>>     I propose that reviews be automatically marked as stale when the
>>     PR is
>>     updated, and re-review be required before integration. Stale
>>     reviews do not
>>     count towards the minimum number of reviews required for
>>     integration.  The
>>     final commit's "Reviewed-by" field will continue to list all
>>     reviewers,
>>     regardless of whether they evaluated an older version of the PR. 
>>     When a PR is
>>     updated, instead of simply noting that the PR applies to a
>>     particular version,
>>     the review will be noted as stale, indicating that the PR does
>>     not meet
>>     integration requirements.  This re-review requirement has been in
>>     effect for
>>     the OpenJFX repos [2] since October 2019, when OpenJFX moved to
>>     GitHub using
>>     the Skara tooling.
>>
>>     I suggest that we adopt this policy two weeks hence, on Tue 3 July.
>>
>>     Concerns?
>>
>>     [0] https://openjdk.org/guide/#fixing-a-bug (step 12)
>>     [1] https://github.com/openjdk/jdk
>>     [2] https://github.com/openjdk/jfx
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240625/79774c10/attachment-0001.htm>