Enforce we dismiss approvals after new commits are added to a pull-request.

[fidencio]

The current policy on Kata Containers is that once approvals are granted, the contributor can push new changes to the PR and the approvals are not dismissed.

This leads to a situation where someone could push non reviewed code to our repo, simply by adding more commits to an already approved PR. I'd like to suggest here that instead of doing that, we consider the possibility that the approvals get removed and we ensure the code is fully reviewed, and that reviewers comment will for sure be addressed before having PRs merged.

/cc @kata-containers/architecture-committee

[egernst]

This is a tradeoff between trust of folks who have commit rights and velocity, and I think we may have hashed this out in the past. Before, we had a small number of committers and reviewers, and I think this made the tradeoff worth the current policy (fewer reviewers in timezones, less committers to need to trust).

Since we're closer to 50 folks now (we should audit this over time), I am okay with adjusting the GH settings to require re-review.

Thanks for bringing this up @fidencio !

[jodh-intel]

+1. Re-reviewing is clearly safer (from both a nefarious actor who has submitted good work in the past, but also from the perspective of an honest mistake that reviewers would have caught if they'd reviewed the latest set of changes).

Yes it might slow velocity so I agree we should trial this for a few months by making the change and then seeing if PR merge velocity drops.

[sameo]

I think it makes sense, we would not be the first OSS community to follow that path. It may makes things slower to merge, so we can review that in a few weeks/months and revert if it really becomes a problem.

Thanks for opening the issue @fidencio

[bergwolf]

+1

[wainersm]

That's a great idea.

Allow me hijack this issue:

QEMU still has an email-based contribution workflow. When the developer sends a new version of a patch set, she/he puts in the cover letter a summary of the changes from the previous version, often pointing out the name of the person who suggested these changes. I like that because, as a reviewer, sometimes (often in my case) you don't remember what was the original request for change and reading the entire discussion is time-consuming. Also sometimes the new version only fix a typo or is a simple rebase, as a reviewer, I would quickly give my 1+ on those situations. So IMHO we should adopt something like that; which cannot be enforced I believe, but over time it can become a habit among kata contributors.

[c3d]

Thanks @fidencio for bringing that up. To follow up on what @wainersm said, I think we could recommend adding new commits when we address review comments, so that we don't invalidate already-reviewed commits. When you review "by file" in the PR, that will not change from today, you still see the combined result. However, if I respond to a review, then I can say "addressed in commit blah" before closing the discussion thread.

That would minimize the number of "force push" that obsolete comments already made, some of which may have not been addressed. It would also record the work being done. And since we end up with a "merge" anyway, it does not really make the history more complicated to track (although if there are too many, some squashing may prove useful).

Of course, this should be left to the appreciation of the committer, but I know that it would help me notably in large reviews (for small reviews, it may not be as useful).

[fidencio]

This has been discussed in the A/C meeting that happened Today (January 25th, 2022), and the setting has been updated on all (ci, community, kata-containers, and tests) repos, for both main and stable-2.3 branches (when the repo had a stable-2.3 branch).