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

[Gibbons, Scott]

Just a thought - won't this essentially double the load on reviewers?  I understand the idea, but are the reviewers able to handle such an increase in workload?


Thanks,

--Scott Gibbons

Software Development Engineer, Runtime Engineering

[cid:c63a16cb-9cb6-414a-b820-25b128dfb442]  DEVELOPER SOFTWARE ENGINEERING
Ph: 1-503-456-7756

Cell: 1-469-450-8390

Intel JF1, 2111 NE 25th Ave

Hillsboro, OR 97124
Intel Corporation | www.intel.com<https://webmail.intel.com/owa/redir.aspx?SURL=WYr7qZDpIv3m1SKFmeHJuzsfCBuGN-jwkBYQUSRR6yrupkscpgzUCGgAdAB0AHAAOgAvAC8AdwB3AHcALgBpAG4AdABlAGwALgBjAG8AbQA.&URL=http%3a%2f%2fwww.intel.com>



________________________________
From: jdk-dev <jdk-dev-retn at openjdk.org> on behalf of Kevin Rushforth <kevin.rushforth at oracle.com>
Sent: Tuesday, June 18, 2024 10:31 AM
To: jdk-dev at openjdk.org <jdk-dev at openjdk.org>
Subject: Re: Proposal: Require re-review before integration if the PR is modified

Yes, it will. At least the minimum number of reviewers will need to
re-review.

-- Kevin


On 6/18/2024 10:27 AM, Daniel Fuchs wrote:
> Hi Iris,
>
> I would welcome this change. But would it block integration
> if the new changeset that was pushed is only a merge changeset,
> merging changes from the mainline into the PR for a last round
> of testing?
>
> best regards,
>
> -- daniel
>
> On 18/06/2024 17:00, Iris Clark 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/20240618/0b429c0a/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Outlook-z2fj5dpz
Type: image/jpg
Size: 1250 bytes
Desc: Outlook-z2fj5dpz
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240618/0b429c0a/Outlook-z2fj5dpz-0001.jpg>