Looks like an all-or-nothing situation - either any commit requires re-approval or no commit requires re-approval. In this case, I would say that all commits should require re-approval since it's the safer approach. Having the issue stay in approved state after a significant change is much worse than requiring the reviewers to take a look at an insignificant commit and re-approve. The time is takes for a reviewer to go over a comment or formatting change is short and, I believe, not intrusive to their workflow.
On Tue, Dec 31, 2019 at 7:48 PM Kevin Rushforth <[email protected]> wrote: > 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 > >
