Move "Require code owner approval" setting from project API to protected branches API
Problem to solve
The CODEOWNERS feature provides greater flexibility than approval rules that are defined for all merge requests. For teams that practice very high velocity development, allowing low risk changes to enter master
very quickly is necessary to achieve this velocity. It is important that high risk changes have a greater level of scrutiny.
GitLab only permits push permissions to protected branches to be controlled at the branch level, but for teams trying maximize velocity, it needs to be possible to divert high risk changes to a merge request workflow, while allowing low risk changes to be merged based on the file paths changed.
Further details
For very large repositories, merge request workflows become a source of contention that can slow velocity. Merge trains in combination with merge when pipelines helps, but different companies have developed their own tooling and processes like allowing changes to be pushed directly to master, and for regressions to be reverted when detected.
The customer requesting this has a one of the largest Git repositories and has invested in such processes above and does not use merge requests extensively. Providing a feature that diverts the workflow towards a merge request based on file path solves a risk problem, and will increase merge request adoption.
This also generally useful to teams with a high degree of trust and desire for maximum velocity, by allowing developers to make changes freely as much as possible.
Proposal
Move the existing Require code owner approval setting from the Project API, to be available per protected branch.
If this option is enabled, it will:
- block pushes that contain changes to files matched by the
CODEOWNERS
file, requiring a merge request to be used - block merges where code owners have not approved
Benefits:
- moving the options from the project level to the branch level is consistent with direction of moving access controls towards branch and file path level for large repositories (greater flexibility)
- prevents excessive merge request restrictions when merging between unprotected feature branches (more permissive)
Challenges:
- requires a migration of the existing setting to the new finer grained setting
- if existing option unchecked, no migration needed
- if existing option checked
- if protected branches exist: new protected branch level setting will be enabled for all protected branches
- if protected branches do not exist: TBD
Links / references
Original request
We've been looking at how the Protected branch flow and Code Owners interact and found that it isn't suitable for our use-case.
We have a big monorepo where users can push commits themselves without going through a MR, but for certain files we'd like to enforce MRs, and were hoping to do so with the code owners feature.
However, as covered in the documentation the CODEOWNERS file only enforces something when the branch is protected, as it piggy-backs on MRs being accepted.
This is conceptually similar to ba3ed8a1 ("Enforce code owner approval", 2019-02-21). Pseudocode for how this might work:
diff --git a/lib/gitlab/checks/branch_check.rb b/lib/gitlab/checks/branch_check.rb
index ad926739752..465d8cb1c7a 100644
--- a/lib/gitlab/checks/branch_check.rb
+++ b/lib/gitlab/checks/branch_check.rb
@@ -38,6 +38,15 @@ module Gitlab
private
def protected_branch_checks
+ if !ProtectedBranch.protected?(project, branch_name) and
+ CodeOwners.in_code_owners?(project, branch_name)
+ if matching_merge_request? && merge_request_approved_by_code_owners?
+ return 1
+ else
+ return 0
+ end
+ end
+
logger.log_timed(LOG_MESSAGES[:protected_branch_checks]) do
return unless ProtectedBranch.protected?(project, branch_name) # rubocop:disable Cop/AvoidReturnFromBlocks
I.e. the protected_branch_checks
now always run, but abort early unless the branch is protected.
At that point we could check if the files being changed are in CODEOWNERS
, and then only accept the push if there's a merge request that's approved bringing us to that SHA-1, assuming of course that there's some MR mechanism to also ensure that you can't just create a MR to approve that SHA-1 unless one of the relevant CODEOWNERS
has approved it, if applicable.
This is assuming that CODEOWNERS
is only used in this mode of mandatory approval, perhaps for this purpose users who use it only for notification would need to somehow be split up from this use-case, or not.
This issue is somewhat conceptually similar to https://gitlab.com/gitlab-org/gitlab-ee/issues/9203