Skip to content
Snippets Groups Projects
Commit 6276d562 authored by Sean McGivern's avatar Sean McGivern :red_circle:
Browse files

Merge branch 'bvl-require-code-owner-approval' into 'master'

Require code owner approval

Closes #4418

See merge request gitlab-org/gitlab-ee!9656
parents 50029c15 ba3ed8a1
No related branches found
No related tags found
1 merge request!9656Require code owner approval
Pipeline #50353073 failed
Showing
with 375 additions and 26 deletions
......@@ -2433,6 +2433,7 @@
t.bigint "pool_repository_id"
t.string "runners_token_encrypted"
t.string "bfg_object_map"
t.boolean "merge_requests_require_code_owner_approval"
t.index ["ci_id"], name: "index_projects_on_ci_id", using: :btree
t.index ["created_at"], name: "index_projects_on_created_at", using: :btree
t.index ["creator_id"], name: "index_projects_on_creator_id", using: :btree
......
......@@ -155,6 +155,23 @@ are other conditions that may block it, such as merge conflicts,
[pending discussions](../../discussions/index.md#l#only-allow-merge-requests-to-be-merged-if-all-discussions-are-resolved)
or a [failed CI/CD pipeline](merge_when_pipeline_succeeds.md).
## Code Owners approvals **[PREMIUM]**
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/4418) in [GitLab Premium](https://about.gitlab.com/pricing/) 11.9.
It is possible to require at least one approval for each entry in the
[`CODEOWNERS` file](../code_owners.md) that matches a file changed in
the merge request. To enable this feature:
1. Navigate to your project's **Settings > General** and expand
**Merge request approvals**.
1. Tick the **Require approval from code owners** checkbox
checkbox.
1. Click **Save changes**.
When this feature is enabled, all merge requests will need approval
from one code owner per matched rule before it can be merged.
## Overriding the merge request approvals default settings
> Introduced in GitLab Enterprise Edition 9.4.
......
<script>
import _ from 'underscore';
import { sprintf, __ } from '~/locale';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import { RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants';
import ApprovedIcon from './approved_icon.vue';
export default {
......@@ -14,6 +16,24 @@ export default {
required: true,
},
},
computed: {
sections() {
return [
{
id: _.uniqueId(),
title: '',
rules: this.approvalRules.filter(rule => rule.rule_type !== RULE_TYPE_CODE_OWNER),
},
{
id: _.uniqueId(),
title: __('Code Owners'),
rules: this.approvalRules
.filter(rule => rule.rule_type === RULE_TYPE_CODE_OWNER)
.map(rule => ({ ...rule, nameClass: 'monospace' })),
},
].filter(x => x.rules.length);
},
},
methods: {
pendingApprovalsText(rule) {
if (!rule.approvals_required) {
......@@ -58,11 +78,17 @@ export default {
<th>{{ s__('MRApprovals|Approved by') }}</th>
</tr>
</thead>
<tbody>
<tr v-for="rule in approvalRules" :key="rule.id">
<tbody v-for="{ id, title, rules } in sections" :key="id" class="border-top-0">
<tr v-if="title" class="js-section-title">
<td class="w-0"></td>
<td colspan="99">
<strong>{{ title }}</strong>
</td>
</tr>
<tr v-for="rule in rules" :key="rule.id">
<td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
<td :colspan="rule.fallback ? 2 : 1">
<div class="d-none d-sm-block js-name">{{ rule.name }}</div>
<div class="d-none d-sm-block js-name" :class="rule.nameClass">{{ rule.name }}</div>
<div class="d-flex d-sm-none flex-column js-summary">
<span>{{ summaryText(rule) }}</span>
<user-avatar-list
......
import _ from 'underscore';
import { __ } from '~/locale';
import { RULE_TYPE_REGULAR, RULE_TYPE_FALLBACK } from 'ee/approvals/constants';
import {
RULE_TYPE_REGULAR,
RULE_TYPE_FALLBACK,
RULE_TYPE_CODE_OWNER,
} from 'ee/approvals/constants';
function mapApprovalRule(rule, settings) {
if (rule.rule_type === RULE_TYPE_FALLBACK) {
......@@ -20,6 +25,23 @@ function mapApprovalRule(rule, settings) {
return rule;
}
function getApprovalRuleNamesLeft(data) {
if (!data.multiple_approval_rules_available) {
return [];
}
const rulesLeft = _.groupBy(data.approval_rules_left, x => x.rule_type);
// Filter out empty names (fallback rule has no name) because the empties would look weird.
const regularRules = (rulesLeft[RULE_TYPE_REGULAR] || []).map(x => x.name).filter(x => x);
// If there are code owners that need to approve, only mention that once.
// As the names of code owner rules are patterns that don't mean much out of context.
const codeOwnerRules = rulesLeft[RULE_TYPE_CODE_OWNER] ? [__('Code Owners')] : [];
return [...regularRules, ...codeOwnerRules];
}
/**
* Map the approval rules response for use by the MR widget
*/
......@@ -33,10 +55,6 @@ export function mapApprovalRulesResponse(rules, settings) {
export function mapApprovalsResponse(data) {
return {
...data,
// Filter out empty names (fallback rule has no name) because
// the empties would look weird.
approvalRuleNamesLeft: data.multiple_approval_rules_available
? data.approval_rules_left.map(x => x.name).filter(x => x)
: [],
approvalRuleNamesLeft: getApprovalRuleNamesLeft(data),
};
}
......@@ -42,6 +42,7 @@ def project_params_ee
use_custom_template
packages_enabled
merge_requests_author_approval
merge_requests_require_code_owner_approval
group_with_project_templates_id
]
......
# frozen_string_literal: true
class ApprovalMergeRequestRule < ApplicationRecord
include Gitlab::Utils::StrongMemoize
include ApprovalRuleLike
DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
......@@ -43,13 +44,18 @@ def approval_project_rule_id=(approval_project_rule_id)
# enabled on project settings.
# @return [Array<User>]
def approvers
scope = super
strong_memoize(:approvers) do
scope_or_array = super
if merge_request.author && !project.merge_requests_author_approval?
scope = scope.where.not(id: merge_request.author)
end
next scope_or_array unless merge_request.author
next scope_or_array if project.merge_requests_author_approval?
scope
if scope_or_array.respond_to?(:where)
scope_or_array.where.not(id: merge_request.author)
else
scope_or_array - [merge_request.author]
end
end
end
def sync_approved_approvers
......
......@@ -5,10 +5,14 @@ class ApprovalWrappedRule
extend Forwardable
include Gitlab::Utils::StrongMemoize
REQUIRED_APPROVALS_PER_CODE_OWNER_RULE = 1
attr_reader :merge_request
attr_reader :approval_rule
def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule, :rule_type
def_delegators(:@approval_rule,
:id, :name, :users, :groups, :code_owner, :code_owner?, :source_rule,
:rule_type)
def initialize(merge_request, approval_rule)
@merge_request = merge_request
......@@ -72,4 +76,22 @@ def approvals_left
def unactioned_approvers
approvers - approved_approvers
end
def approvals_required
if code_owner?
code_owner_approvals_required
else
approval_rule.approvals_required
end
end
private
def code_owner_approvals_required
strong_memoize(:code_owner_approvals_required) do
next 0 unless project.merge_requests_require_code_owner_approval?
approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0
end
end
end
......@@ -13,12 +13,18 @@ module ApprovalRuleLike
validates :name, presence: true
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 }
scope :with_users, -> { preload(:users, :group_users) }
end
# Users who are eligible to approve, including specified group members.
# @return [Array<User>]
def approvers
@approvers ||= User.from_union([users, group_users])
@approvers ||= if users.loaded? && group_users.loaded?
users | group_users
else
User.from_union([users, group_users])
end
end
def add_member(member)
......
......@@ -229,6 +229,10 @@ def multiple_approval_rules_available?
feature_available?(:multiple_approval_rules)
end
def code_owner_approval_required_available?
feature_available?(:code_owner_approval_required)
end
def service_desk_enabled
::EE::Gitlab::ServiceDesk.enabled?(project: self) && super
end
......@@ -338,6 +342,10 @@ def approver_group_ids=(value)
end
end
def merge_requests_require_code_owner_approval?
super && code_owner_approval_required_available?
end
def find_path_lock(path, exact_match: false, downstream: false)
path_lock_finder = strong_memoize(:path_lock_finder) do
::Gitlab::PathLocksFinder.new(self)
......
......@@ -70,7 +70,7 @@ class License < ActiveRecord::Base
protected_environments
custom_project_templates
packages
code_owner_as_approver_suggestion
code_owner_approval_required
feature_flags
batch_comments
issues_analytics
......
......@@ -39,6 +39,10 @@ def target_project
merge_request.target_project.present(current_user: current_user)
end
def code_owner_rules_with_users
@code_owner_rules ||= merge_request.approval_rules.code_owner.with_users.to_a
end
def approver_groups
::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
end
......
......@@ -8,7 +8,7 @@ def initialize(merge_request, params = {})
end
def execute
if ::Feature.enabled?(:multiple_code_owner_rules)
if ::Feature.enabled?(:multiple_code_owner_rules, default_enabled: true)
sync_rules
else
merge_request.sync_code_owners_with_approvers
......
......@@ -5,23 +5,31 @@
- else
= render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project
- if project.code_owner_approval_required_available?
.form-group.require-code-owner-approval
.form-check
= form.check_box(:merge_requests_require_code_owner_approval, class: 'form-check-input')
= form.label :merge_requests_require_code_owner_approval, class: 'form-check-label' do
%strong= _('Require approval from code owners')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'editing-approvals-premium'), target: '_blank'
.form-group
.form-check
= form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true)
= form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
%strong= _("Can override approvers and approvals required per merge request")
= link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank'
%strong= _('Can override approvers and approvals required per merge request')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank'
.form-group.reset-approvals-on-push
.form-check
= form.check_box :reset_approvals_on_push, class: 'form-check-input'
= form.label :reset_approvals_on_push, class: 'form-check-label' do
%strong= _("Remove all approvals in a merge request when new commits are pushed to its source branch")
%strong= _('Remove all approvals in a merge request when new commits are pushed to its source branch')
.form-group.self-approval
.form-check
= form.check_box :merge_requests_author_approval, class: 'form-check-input'
= form.label :merge_requests_author_approval, class: 'form-check-label' do
%strong= _("Enable self approval of merge requests")
= link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals",
%strong= _('Enable self approval of merge requests')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals',
anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
- return unless @project.merge_requests_require_code_owner_approval?
- code_owner_rules = merge_request.code_owner_rules_with_users
- return unless code_owner_rules.any?
.prepend-top-20
%strong= _('Code owner approval is required')
%p
= _('At least one approval from a code owner is required to change files matching the respective CODEOWNER rules.')
= link_to(_('Read more'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'code-owners-approvals-premium'))
.border-bottom
%table.table.m-0
%thead.thead-white.text-nowrap
%tr.d-none.d-sm-table-row
%th.w-25= s_('CodeOwner|Pattern')
%th= _('Members')
%tbody
- code_owner_rules.each do |code_owner_approval_rule|
%tr
%td.monospace= code_owner_approval_rule.name
%td.d-none.d-sm-table-cell
- code_owner_approval_rule.approvers.each do |approver|
= user_avatar(user: approver)
......@@ -26,3 +26,4 @@
Tip: add a
= link_to 'CODEOWNERS', help_page_path('user/project/code_owners'), target: '_blank', tabindex: -1
to automatically add approvers based on file paths and file types.
= render 'projects/merge_requests/code_owner_approval_rules', merge_request: @mr_presenter
---
title: Enforce merge request approvals from code owners
merge_request: 9656
author:
type: added
# frozen_string_literal: true
class AddMergeRequestsRequireCodeownerApprovalToProjects < ActiveRecord::Migration[5.0]
DOWNTIME = false
def change
add_column :projects, :merge_requests_require_code_owner_approval, :boolean
end
end
require 'rails_helper'
describe 'Merge request > User sees approval widget', :js do
let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) }
let(:project) { create(:project, :public, :repository) }
let(:user) { project.creator }
let(:merge_request) { create(:merge_request, source_project: project) }
before do
stub_feature_flags(approval_rules: false)
sign_in(user)
end
context 'when merge when discussions resolved is active' do
......@@ -17,8 +19,6 @@
end
before do
sign_in(user)
visit project_merge_request_path(project, merge_request)
end
......@@ -27,4 +27,111 @@
expect(find('.js-mr-approvals')).to have_selector('.approvals-body')
end
end
context 'when rules are enabled' do
before do
stub_feature_flags(approval_rules: true)
end
context 'merge request approvers enabled' do
let(:project) { create(:project, :public, :repository, approvals_before_merge: 3) }
before do
stub_licensed_features(merge_request_approvers: true)
visit project_merge_request_path(project, merge_request)
end
it 'the renders the number of required approvals' do
wait_for_requests
expect(page).to have_content('Requires 3 more approvals.')
end
end
context 'multiple approval rules enabled' do
let(:members) { create_list(:user, 2) }
let!(:rule) do
create(:approval_merge_request_rule,
merge_request: merge_request,
users: members,
approvals_required: 1)
end
before do
stub_licensed_features(multiple_approval_rules: true)
members.each { |user| project.add_developer(user) }
end
it 'shows the approval rule' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires approval from #{rule.name}")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table') do
expect(page).to have_content(rule.name)
end
end
context 'for code owner rules' do
let(:code_owners) { create_list(:user, 2) }
let!(:code_owner_rule) do
create(:code_owner_rule,
merge_request: merge_request,
users: code_owners,
name: '*.js')
end
before do
code_owners.each { |user| project.add_developer(user) }
end
it 'shows the code owner rule as optional' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires approval from #{rule.name}.")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table .monospace') do
code_owner_row = find(:xpath, "//tr[td[contains(.,'#{code_owner_rule.name}')]]")
expect(code_owner_row).to have_content('Optional')
end
end
context 'when code owner approval is required' do
before do
stub_licensed_features(code_owner_approval_required: true, multiple_approval_rules: true)
project.update!(merge_requests_require_code_owner_approval: true)
end
it 'shows the code owner rule as required' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires 2 more approvals from #{rule.name} and Code Owners")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table .monospace') do
code_owner_row = find(:xpath, "//tr[td[contains(.,'#{code_owner_rule.name}')]]")
expect(code_owner_row).to have_content('0 of 1')
end
end
end
end
end
end
end
require 'spec_helper'
describe 'Projects > Merge Requests > User edits a merge request' do
let(:user) { create(:user) }
before do
stub_licensed_features(licensed_features)
project.add_maintainer(user)
sign_in(user)
end
context 'when the merge request has matching code owners', :js do
let(:licensed_features) do
{ code_owners: true, code_owner_approval_required: true }
end
let(:project) do
create(:project, :custom_repo,
merge_requests_require_code_owner_approval: true,
files: { 'docs/CODEOWNERS' => "*.rb @ruby-owner\n*.js @js-owner" })
end
let(:merge_request) do
create(:merge_request,
source_project: project,
target_project: project,
target_branch: 'master',
source_branch: 'feature')
end
let(:ruby_owner) { create(:user, username: 'ruby-owner') }
before do
project.add_developer(ruby_owner)
project.repository.create_file(user, 'ruby.rb', '# a ruby file',
message: 'Add a ruby file',
branch_name: 'feature')
# To make sure the rules are created for the merge request, the services
# that do that aren't triggered from factories
MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
end
it 'shows the matching code owner rules' do
visit(edit_project_merge_request_path(project, merge_request))
expect(page).to have_content('*.rb')
expect(page).to have_link(href: user_path(ruby_owner))
end
end
end
require 'spec_helper'
describe 'EE > Projects > Settings > User manages approval rule settings' do
let(:project) { create(:project) }
let(:user) { project.owner }
before do
sign_in(user)
stub_licensed_features(licensed_features)
visit edit_project_path(project)
end
context 'when `code_owner_approval_required` is available' do
let(:licensed_features) { { code_owner_approval_required: true } }
it 'allows the user to enforce code owner approval' do
within('.require-code-owner-approval') do
check('Require approval from code owners')
end
within('.merge-request-approval-settings-form') do
click_on('Save changes')
end
expect(project.reload.merge_requests_require_code_owner_approval?).to be_truthy
end
end
context 'when `code_owner_approval_required` is not available' do
let(:licensed_features) { { code_owner_approval_required: false } }
it 'does not allow the user to require code owner approval' do
expect(page).not_to have_content('Require approval from code owners')
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment