Large organizations with many projects and large projects need to enforce review policies so that they can ensure the correct teams and individuals review changes that impact them. GitLab should allow approvals from Code Owners to be required before a merge request can be merged.
Target audience
Further details
Building on the integration of Code Owners into the merge request approval process, we should make it possible to require Code Owner approval through the merge request workflow. To fully enforce code owners on a specific branch the Protected Branch rule to prohibit anyone from pushing directly to the branch to avoid a manual merge bypassing approval requirements.
Proposal
Add a merge request option to project settings to require approval by code owners in merge requests:
Require approval by code owners
Only allow code owners to change files that match CODEOWNER rules. Commits that change files owned by other users or groups will require approval before the merge request may be merged. Owners may change any file.
Until all matching code owners rules are satisfied by one approval (or more), the merge request cannot be merged.
Add a new ~"usage ping" field for the count of projects where code owner approval is required.
Designs
Project settings
MR create/edit form
MR Widget
The project settings for MR approvals has a checkbox to enforce codeowner approval.
The MR create/edit form shows a message that codeowner approval is required and displays the codeowner rules that match for the given MR.
The MR widget shows the current status for each of the matching codeowner rules
The default number of approvals required per codeowner rule will be 1. We will offer customizability in a follow-up issue.
What does success look like, and how can we measure that?
We will monitor adoption of required code owner approvals with a new usage ping field
If the MR has approval from all code owners, we can skip the protected branch/push rule check when merging it. We can even check that when pushing a manual MR merge, as long as the manual merge didn't make any new changes.
Very very large install base of Gerrit (15,000 - 18,000) users. While not yet defined as a requirement in moving to GitLab, given their Gerrit installation is massive this could be a very nice offering to them as they start to shift their current users as well as GitLab CE and EES instances over to a central IT group managing their git use.
This is the opportunity for this customer. https://gitlab.my.salesforce.com/0066100000Jvyzn
Very large Financial Services company. They are trying GitLab now but have mentioned how our competitors have this feature and it would be needed to move forward with GitLab. They mentioned the ability to allow for collaboration at the code level but approval at the MR Level. Looking for default models as they have to scale this to accommodate for the entire company (8-10,000 developers).
Over at @nasa-jsc-robotics we really require this feature due to cross-product projects (repos) being shared and edited by each product's developers. As far as I can see this is the best implementation of specific required approvers for changes...
@jramsay It's not clear to me whether the goal here is to allow code owners to be enforced on MRs, or on (protected) branches.
The title suggests it's the former, but then the description states "it is also necessary to add controls to prevent changes directly to important branches without approval. We should add the ability to enforce code ownership requirements on branches" which suggests the latter. "Use CODEOWNERS file to determine code owner of the branch and add a merge request option to require approval by code owners." suggests we're adding an MR option, but then "If a user that is not an owner of a file tries to push a commit that changes the file to a protected branch" again refers to a protected branch
If the goal is to enforce code owners on protected branches, I'm not sure why we'd need a checkbox on the MR, because any MR to a protected target branch would automatically be affected, and there would/should be no way to disable it. I also assume that in that case, we would want to look at all of the MR's approvals to determine whether to accept the merge into master, rather than just the user who actually pushed the merge commit.
If the goal is to enforce code owners on MRs, I'm not sure how "If a user that is not an owner of a file tries to push a commit that changes the file to a protected branch" is relevant, and per the description, "Code owners will be automatically added to related Merge Requests (separate feature)".
@DouweM the title was accurate, but the content of the description was a mismatch of various ideas from the early proposal. I've clarified the scope and removed references to protected branches. This iteration should be linked to merge requests. If approvals need to be enforced on a protected branch this can be done by preventing anyone from pushing to the branch, and only allowing a merge through the GitLab interface.
@lulalala How would this fit into our new Multiple Approval Rules setup? Would it be a matter of making the automatically generated codeowners approval rule(s) required to be met?
I've clarified the scope and removed references to protected branches.
There is still one reference to 'protected branch' in the "Further Details" description section. So is that part of the scope?
If approvals need to be enforced on a protected branch this can be done by preventing anyone from pushing to the branch, and only allowing a merge through the GitLab interface.
The "if author is code owner, then CO rule can be considered fulfilled" thing is an interesting exception which just falls outside of the current rule system. So it won't be a simple manipulating approvals required count. Some kind of rule subclassing is needed.
If a CODEOWNER-rule defines multiple users, how many of those users need to Approve the merge request? The other approval rules allow specifying the number of approvers, should we do the same here?
To keep the iteration smaller, I'd maybe push that to a second iteration and default to 1 per CODEOWNER-rule. WDYT?
One MereRequestApproval rule per applicable line in the CODEOWNERS file
The name is set to the pattern in the codeowners file
RequiredApprovals is set to 0 in the first iteration: This will add all the CODEOWNERS as possible approvers, but none of them required and allow us to display these rules on the merge request edit form and the widget.
Add the checkbox to project settings
When this is checked, all MergeRequestApprovalRules will have their required approvals set to 1. We should probably delegate to avoid updating all the rows.
The pseudo code that came up for this was, but needs investigation:
defapprovals_requiredsuperunlesscode_ower?merge_request.project.code_owner_approvals_required# hardcoded to 1end
To keep the iteration smaller, I'd maybe push that to a second iteration and default to 1 per CODEOWNER-rule.
@reprazent I agree. We can go with a default 1 approval per rule for this iteration. We can allow customizability later. I designed the screens (shared above) with this in mind.
RequiredApprovals is set to 0 in the first iteration: This will add all the CODEOWNERS as possible approvers, but none of them required and allow us to display these rules on the merge request edit form and the widget.
This means that when Require approval from codeowners is not enabled, we'd still add the codeoweners as optional approvers.
The default state of a project should be that the Require approval from codeowners checkbox is not selected and any/all codeowners are optional approvers. If the checkbox is selected, then we can go with a default 1 approval per codeowner rule for this iteration.
Maybe we need some copy for the MR create/edit form and MR Widget when codeowner approval is optional?
@reprazent In the design proposal for https://gitlab.com/gitlab-org/gitlab-ee/issues/1979, I have included a small note below the approver rules list in the MR create/edit form. Also, in the MR widget, codeowners are shown as optional approvers. Is this what you meant?
@jramsay Do I understand code owner approvals are still possible in GitLab Starter, as they are now.
So the different rules would still be displayed on the MR edit page, and in the MR widget. But the approvals would just be optional. In GitLab Premium we show the checkbox to enforce code owner approval.
Sorry for pinging a closed issue, but I just wanted some clarification. I'm not sure if I should create a new issue for them.
I implemented CODEOWNERS with GitLab before it was implemented as a feature in GitLab. I used webhooks and protected branches. The project is called Bulwark.
I'd like to see if there was a way I can get rid of this project. Here are my companies requirements (which I built Bulwark for).
For each file changed in the MR (deleted/removed/edited), any matching users (regardless of the number of CODEOWNERS lines/entries) will HAVE to approve the merge request.
This is conflicting a little what what I'm seeing mentioned here:
One MereRequestApproval rule per applicable line in the CODEOWNERS file
That means the following CODEOWNERS file will only require 1 user to approve that changes.
* pknopf mreents
My companies requirements are that both pknopf and mreents MUST approve the changes, not "at least 1" of them.
Given what I've read in this issue, it seems that in order to require both pknopf and mreents to approve, I'd need to have a CODEOWNERS file like this:
* pknopf* mreents
I don't think that is a good idea. I think if a CODEOWNERS file globs a user, regardless of the number of lines they appear in, they MUST approve the changes. The structure of the CODEOWNERS file shouldn't dictate how the approvals work. If a path/user is globbed via CODEOWNERS (as a whole/unit), they are added as a required approver.
If a user is added/removed to the CODEOWNERS file, they must approve the changes.
This is to ensure that people can't stealthily make changes to files they shouldn't by hijacking the CODEOWNERS file.
Require approval from the author of the MR, regardless of CODEOWNERS.
This ensures that if a random user submits a MR, other users can't make changes to the MR without the authors approval.
Does anyone here know if it's possible to increase the default number of codeowner approvals for a rule where the codeowner is a group from the default of 1 approver to any other number? I know it's possible to set a separate rule in Settings > General > MR Approvals, but these rules don't seem to allow specifying that the approvals must come from codeowners.
For people stumbling upon this issue and not finding the answer to the last 3 questions: Specifying the minimum number of required approvals for code owners is supported when using sections: