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

[erik.joelsson at oracle.com]

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/e540b962/attachment.htm>