Don't require code owner re-approval if changes don't affect approved files
Problem description
When iterating on a feature in an MR that changes a file requiring code owner approval, every new push to the source branch invalidates code owner approvals that were already given. This is the case even for pushes that don't change the files which require code owner approval. This causes some inefficiency in the code review process and confuses users that already approved the changes.
Proposal
- Add new setting to
Project settings
>Merge request approvals
>Approval settings
-
🎨 Please check attached design for setting location and UI text
Code owner approvals should be specific to changed files, even if the MR is updated with new changes. Re-approval should only be necessary if the files requiring approval are changed.
Example
In a repo, each file can have a different code owner. If a merge request
- changes multiple files
- all changes are approved once by all required code owners
- and a file is changed again, then only the code > owners of the changed files need to approve.
Expected behavior:
- File A: Code owner A
- File B: Code owner B
Steps: In a merge request.
- File A and B are changed
- Code owners A and B have given their consent
- File A is changed
In this case
- Code owner A must approve because the last approval is outdated.
- Code owner B does not have to approve because the last approval is still valid.
Implementation Plan
-
Add preserve_codeowner_approvals_on_unchanged_f... (#369549) -
Wire up to the frontend -
Update ee/app/services/merge_requests/reset_approvals_service.rb
?
merge_request.approvals
merge_request.approval_rules (rule_type: code_owner)
merge_request.approval_rules[x].approvers
merge_request.approval_state.wrapped_approval_rules[x].approvers
merge_request.approval_state.wrapped_approval_rules[x].approved_approvers
We need to figure out:
- which (if any) of the codeowner rules were previously approved?
- of those rules, have any of the associated files been changed?
- are those approvers responsible for any other rules which do need to be reset?
Edited by Lee Tickett