[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Should Skara tooling invalidate approvals when new commits are pushed? [was: RFR: 8130738: Add tabSi
From:       Kevin Rushforth <kevin.rushforth () oracle ! com>
Date:       2019-12-31 17:45:16
Message-ID: c8c7a4fe-faca-1afc-d8ff-a59dda0a5497 () oracle ! com
[Download RAW message or body]

Since this isn't directly related to the PR in question, I'm starting a 
new thread.

On 12/20/2019 7:22 PM, Philip Race wrote:
> On 12/20/19, 7:04 PM, Scott Palmer wrote:
>> I'm not sure if I'me supposed to try to integrate now that I've made 
>> that 10 ->  0 change, or if the new change resets the need for review...
>
> It shows ready, which surprises me.
> Still learning skara .. I'd expect any change to reset as how can it 
> know if it is
> a  good or insignificant change or not no matter how the commenter 
> characterised the issue ?

Pushing a new commit doesn't automatically invalidate existing 
approvals, although it is highlighted in the "Approvers" section that 
the approval was for a prior commit:

     Approvers
         Phil Race (prr - Reviewer) Note! Review applies to f846ad6

I remember bringing this up with the Skara team during my initial 
testing, since I also had wondered whether pushing a new commit should 
invalidate approvals. The current policy allows for doing trivial fixes 
brought up when approving a review (e.g., a change in a comment or 
formatting) without requiring all reviewers to re-approve. It matches 
current practice for email-based reviews, so I think the current 
behavior is a reasonable default.

They did say that they could implement an optional feature (i.e., a 
project would need to opt-in to this behavior, probably via a 
.jcheck/conf property) to invalidate all approvals upon pushing a new 
commit, but I don't think an RFE was filed to track it.

This seems like it could be a valuable feature, although it does come 
with a slight cost in terms of timing and flexibility. What do others think?

-- Kevin

[prev in list] [next in list] [prev in thread] [next in thread]