Github Settings on PR approvals

[jakemas]

I've noticed recently that once a PR has been approved, one can continue to push code without the approval being automatically removed/dismissed. Example PR: #1398, Hanno approved, I pushed 2 new edits via force push, Hanno's approval remains. I think before the new PR queue mechanism was introduced to mldsa-native, any subsequent change to an approval would auto-dismiss the approval and request re-review etc. as aws-lc does.

Given we strive for single commit PRs and often force-push, we certainly lose fidelity over commits added post approval, and I could see this as a potential way for code to be potentially un-reviewed. Opening this issue so we can look into this/check/change/discuss the setting.

[mkannwischer]

I think before the new PR queue mechanism was introduced to mldsa-native, any subsequent change to an approval would auto-dismiss the approval and request re-review etc. as aws-lc does.

We've never auto-dismissed approvals on pushes. It's an independent setting that has been intentionally turned off since we created the repo. I see it as the responsibility of the maintainer not to merge PRs with non-trivial changes after approval.

Given we strive for single commit PRs and often force-push, we certainly lose fidelity over commits added post approval, and I could see this as a potential way for code to be potentially un-reviewed. Opening this issue so we can look into this/check/change/discuss the setting.

We don't strive for single-commit PRs. We strive for a clean commit history where each logical change goes into its own commit, and we prefer smaller PRs over larger ones where it makes sense.
I personally find diffing the commits before and after a force-push works well, and GitHub makes it convenient to view the diff of a force-push.

Auto-dismissing approvals would add delay for trivial updates to PRs, so I'm opposed to it. WDYT @hanno-becker?

[jakemas]

Ok thats reasonable, and good to know.