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

[Thomas Stüfe]

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/20240704/170fcc1b/attachment-0001.htm>