Proposal: Require re-review before integration if the PR is modified
Iris Clark
iris.clark at oracle.com
Tue Jun 18 16:00:00 UTC 2024
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
More information about the jdk-dev
mailing list