<div dir="ltr">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.<div><br></div><div>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.</div><div><br></div><div>Thanks, Thomas</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 25, 2024 at 1:14 PM Jorn Vernee <<a href="mailto:jorn.vernee@oracle.com">jorn.vernee@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><u></u>
<div>
<p>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.<br>
<br>
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.<br>
</p>
<p>Jorn<br>
</p>
<div>On 25-6-2024 11:19,
<a href="mailto:erik.joelsson@oracle.com" target="_blank">erik.joelsson@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite">
<div>On 6/24/24 06:38, Thomas Stüfe wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>One question, do merges with master cause the Skara
reviewer check to fail? </div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
</div>
</blockquote>
<p>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.</p>
<p>Implementing such a check may be possible in the future, but
it's not something we can promise to happen.<br>
</p>
<p>/Erik<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>Cheers, Thomas</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Jun 18, 2024 at
6:00 PM Iris Clark <<a href="mailto:iris.clark@oracle.com" target="_blank">iris.clark@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi.<br>
<br>
Every PR must be reviewed by at least one Reviewer [0]
before it's integrated<br>
into the main-line repository [1]. While we recommend that
the PR contain the<br>
final version of the change at creation time, this is not
always feasible,<br>
particularly for large or complex changes. The integrated
version of the PR<br>
may evolve significantly from when the review was done.
Thus the final<br>
commit's "Reviewed-by" field may give false confidence that
the complete set<br>
of changes has been reviewed before it was integrated into
the repository.<br>
This could, in the worst case, allow an adversary to commit
malicious code.<br>
<br>
I propose that reviews be automatically marked as stale when
the PR is<br>
updated, and re-review be required before integration.
Stale reviews do not<br>
count towards the minimum number of reviews required for
integration. The<br>
final commit's "Reviewed-by" field will continue to list all
reviewers,<br>
regardless of whether they evaluated an older version of the
PR. When a PR is<br>
updated, instead of simply noting that the PR applies to a
particular version,<br>
the review will be noted as stale, indicating that the PR
does not meet<br>
integration requirements. This re-review requirement has
been in effect for<br>
the OpenJFX repos [2] since October 2019, when OpenJFX moved
to GitHub using<br>
the Skara tooling.<br>
<br>
I suggest that we adopt this policy two weeks hence, on Tue
3 July.<br>
<br>
Concerns?<br>
<br>
[0] <a href="https://openjdk.org/guide/#fixing-a-bug" rel="noreferrer" target="_blank">https://openjdk.org/guide/#fixing-a-bug</a>
(step 12)<br>
[1] <a href="https://github.com/openjdk/jdk" rel="noreferrer" target="_blank">https://github.com/openjdk/jdk</a><br>
[2] <a href="https://github.com/openjdk/jfx" rel="noreferrer" target="_blank">https://github.com/openjdk/jfx</a><br>
</blockquote>
</div>
</blockquote>
</blockquote>
</div>
</blockquote></div>