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

[Kevin Rushforth]

> Thanks, Jorn. Yeah, okay, but this seems rather cumbersome. And it 
> leaves the reviewers guessing. As a Reviewer, I want to see a PR 
> branch that is kept in sync with master, especially for longer-running 
> PRs.

An enhancement was filed to not require re-review for a clean merge:

https://bugs.openjdk.org/browse/SKARA-2312

It is being worked on, although there is no timeline for when it might 
be implemented.

> Another thought, would it be possible for Skara to disregard changes 
> that just fix the copyrights?

It might be possible, since Skara does this when comparing whether a 
backport is clean. I'm not sure how likely it is.

-- Kevin


On 7/4/2024 3:43 AM, Thomas Stüfe wrote:
> Thanks, Jorn. Yeah, okay, but this seems rather cumbersome. And it 
> leaves the reviewers guessing. As a Reviewer, I want to see a PR 
> branch that is kept in sync with master, especially for longer-running 
> PRs.
>
> Another thought, would it be possible for Skara to disregard changes 
> that just fix the copyrights? I have many reviews ending with "LGTM, 
> please fix copyrights before pushing". It would be nice not to need 
> another review for those.
>
> Thanks, Thomas
>
> On Tue, Jun 25, 2024 at 1:14 PM Jorn Vernee <jorn.vernee at oracle.com> 
> wrote:
>
>     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/20240708/622d4133/attachment-0001.htm>