Commit e48a3b62 authored by Kerri Miller's avatar Kerri Miller Committed by James Lopez

Migration to move MRRCOA from project to branch

I've set this up as a post-deploy background migration, taking advantage
of `each_batch` to set up jobs for Sidekiq workers to copy the project's
`merge_requests_require_code_owner_approval` to each of its protected
branch's `code_owner_approval_required` attribute. I've added some basic
tests here, as well.

I initially added a pair of #after_commit hooks to make sure projects
get updated while we were asynchronously chewing through the projects
and branches. However, I abandoned this, as a database reviewer supplied
a faster approach that shouldn't be an issue in terms of the total time
required to process the entities.
parent 2f4be6da
......@@ -40,6 +40,11 @@ class ProtectedBranch < ApplicationRecord
def self.protected_refs(project)
project.protected_branches.select(:name)
end
def self.branch_requires_code_owner_approval?(project, branch_name)
# NOOP
#
end
end
ProtectedBranch.prepend_if_ee('EE::ProtectedBranch')
# frozen_string_literal: true
class MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
BATCH_SIZE = 200
class Project < ActiveRecord::Base
include EachBatch
self.table_name = 'projects'
self.inheritance_column = :_type_disabled
has_many :protected_branches
end
class ProtectedBranch < ActiveRecord::Base
include EachBatch
self.table_name = 'protected_branches'
self.inheritance_column = :_type_disabled
belongs_to :project
end
def up
add_concurrent_index :projects, :id, name: "temp_active_projects_with_prot_branches", where: 'archived = false and pending_delete = false and merge_requests_require_code_owner_approval = true'
ProtectedBranch
.joins(:project)
.where(projects: { archived: false, pending_delete: false, merge_requests_require_code_owner_approval: true })
.each_batch(of: BATCH_SIZE) do |batch|
batch.update_all(code_owner_approval_required: true)
end
remove_concurrent_index_by_name(:projects, "temp_active_projects_with_prot_branches")
end
def down
# noop
#
end
end
......@@ -92,16 +92,15 @@ class ApprovalWrappedRule
def code_owner_approvals_required
strong_memoize(:code_owner_approvals_required) do
next 0 unless merge_requests_require_code_owner_approval?
next 0 unless branch_requires_code_owner_approval?
approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0
end
end
def merge_requests_require_code_owner_approval?
def branch_requires_code_owner_approval?
return false unless project.code_owner_approval_required_available?
project.merge_requests_require_code_owner_approval? ||
project.branch_requires_code_owner_approval?(merge_request.target_branch)
ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch)
end
end
......@@ -8,6 +8,14 @@ module EE
protected_ref_access_levels :unprotect
end
class_methods do
def branch_requires_code_owner_approval?(project, branch_name)
return false unless project.code_owner_approval_required_available?
project.protected_branches.requiring_code_owner_approval.matching(branch_name).any?
end
end
def code_owner_approval_required
super && project.code_owner_approval_required_available?
end
......
......@@ -102,7 +102,7 @@ module EE
scope :verification_failed_wikis, -> { joins(:repository_state).merge(ProjectRepositoryState.verification_failed_wikis) }
scope :for_plan_name, -> (name) { joins(namespace: :plan).where(plans: { name: name }) }
scope :requiring_code_owner_approval,
-> { where(merge_requests_require_code_owner_approval: true) }
-> { joins(:protected_branches).where(protected_branches: { code_owner_approval_required: true }) }
scope :with_security_reports_stored, -> { where('EXISTS (?)', ::Vulnerabilities::Occurrence.scoped_project.select(1)) }
scope :with_security_reports, -> { where('EXISTS (?)', ::Ci::JobArtifact.security_reports.scoped_project.select(1)) }
......@@ -377,13 +377,14 @@ module EE
end
def merge_requests_require_code_owner_approval?
super && code_owner_approval_required_available?
code_owner_approval_required_available? &&
protected_branches.requiring_code_owner_approval.any?
end
def branch_requires_code_owner_approval?(branch_name)
return false unless code_owner_approval_required_available?
protected_branches.requiring_code_owner_approval.matching(branch_name).any?
::ProtectedBranch.branch_requires_code_owner_approval?(self, branch_name)
end
def require_password_to_approve
......
---
title: Move Require code owner approval setting from Project to ProtectedBranches
merge_request: 16187
author:
type: changed
......@@ -55,9 +55,5 @@ FactoryBot.modify do
trait :random_last_repository_updated_at do
last_repository_updated_at { rand(1.year).seconds.ago }
end
trait :requiring_code_owner_approval do
merge_requests_require_code_owner_approval true
end
end
end
......@@ -107,7 +107,11 @@ describe 'Merge request > User sees approval widget', :js do
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)
allow(ProtectedBranch)
.to receive(:branch_requires_code_owner_approval?).and_return(true)
end
it 'shows the code owner rule as required' do
......
......@@ -16,7 +16,6 @@ describe 'Projects > Merge Requests > User edits a merge request' do
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
......@@ -36,6 +35,10 @@ describe 'Projects > Merge Requests > User edits a merge request' do
message: 'Add a ruby file',
branch_name: 'feature')
create(:protected_branch,
code_owner_approval_required: true,
project: project)
# 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
......
......@@ -35,6 +35,12 @@ describe 'EE > Projects > Settings > User manages approval rule settings' do
context 'when `code_owner_approval_required` is available' do
let(:licensed_features) { { code_owner_approval_required: true } }
before do
create(:protected_branch,
code_owner_approval_required: true,
project: project)
end
it_behaves_like 'dirty submit form', [{ form: '#js-merge-request-approval-settings', input: '#project_merge_requests_author_approval' }]
it 'allows the user to enforce code owner approval' do
......
......@@ -195,9 +195,15 @@ describe Gitlab::UsageData do
describe 'code owner approval required' do
before do
create(:project, :archived, :requiring_code_owner_approval)
create(:project, :requiring_code_owner_approval, pending_delete: true)
create(:project, :requiring_code_owner_approval)
create(:protected_branch, code_owner_approval_required: true)
create(:protected_branch,
code_owner_approval_required: true,
project: create(:project, :archived))
create(:protected_branch,
code_owner_approval_required: true,
project: create(:project, pending_delete: true))
end
it 'counts the projects actively requiring code owner approval' do
......
......@@ -196,23 +196,12 @@ describe ApprovalWrappedRule do
.to receive(:code_owner_approval_required_available?).and_return(true)
end
context "when no protected_branches require code owner approval" do
it 'returns the correct number of approvals' do
allow(subject.project)
.to receive(:merge_requests_require_code_owner_approval?).and_return(feature_enabled)
allow(subject.project)
.to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(false)
expect(subject.approvals_required).to eq(expected_required_approvals)
end
end
context "when the project doesn't require code owner approval on all MRs" do
it 'returns the expected number of approvals for protected_branches that do require approval' do
allow(subject.project)
.to receive(:merge_requests_require_code_owner_approval?).and_return(false)
allow(subject.project)
.to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(feature_enabled)
allow(ProtectedBranch)
.to receive(:branch_requires_code_owner_approval?).with(subject.project, branch.name).and_return(feature_enabled)
expect(subject.approvals_required).to eq(expected_required_approvals)
end
......
......@@ -45,11 +45,15 @@ describe Project do
context 'scopes' do
describe '.requiring_code_owner_approval' do
let!(:project) { create(:project) }
let!(:expected_project) { protected_branch_needing_approval.project }
let!(:protected_branch_needing_approval) { create(:protected_branch, code_owner_approval_required: true) }
it 'only includes the right projects' do
create(:project)
expected_project = create(:project, merge_requests_require_code_owner_approval: true)
scoped_query_result = described_class.requiring_code_owner_approval
expect(described_class.requiring_code_owner_approval).to contain_exactly(expected_project)
expect(described_class.count).to eq(2)
expect(scoped_query_result).to contain_exactly(expected_project)
end
end
......@@ -906,13 +910,17 @@ describe Project do
true | true | true
false | true | false
true | false | false
true | nil | false
end
with_them do
before do
stub_licensed_features(code_owner_approval_required: feature_available)
project.merge_requests_require_code_owner_approval = feature_enabled
if feature_enabled
create(:protected_branch,
project: project,
code_owner_approval_required: true)
end
end
it 'requires code owner approval when needed' do
......
# frozen_string_literal: true
require 'spec_helper'
require Rails.root.join('db', 'post_migrate', '20190827102026_migrate_code_owner_approval_status_to_protected_branches_in_batches.rb')
describe MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches, :migration do
let(:namespaces) { table(:namespaces) }
let(:projects) { table(:projects) }
let(:protected_branches) { table(:protected_branches) }
let(:namespace) do
namespaces.create!(
path: 'gitlab-instance-administrators',
name: 'GitLab Instance Administrators'
)
end
let(:project) do
projects.create!(
namespace_id: namespace.id,
name: 'GitLab Instance Administration'
)
end
let!(:protected_branch_1) do
protected_branches.create!(
name: "branch name",
project_id: project.id
)
end
describe '#up' do
context "when there's no projects needing approval" do
it "doesn't change any protected branch records" do
expect { migrate! }
.not_to change { ProtectedBranch.where(code_owner_approval_required: true).count }
end
end
context "when there's a project needing approval" do
let!(:project_needing_approval) do
projects.create!(
namespace_id: namespace.id,
name: 'GitLab Instance Administration',
merge_requests_require_code_owner_approval: true
)
end
let!(:protected_branch_2) do
protected_branches.create!(
name: "branch name",
project_id: project_needing_approval.id
)
end
it "changes N protected branch records" do
expect { migrate! }
.to change { ProtectedBranch.where(code_owner_approval_required: true).count }
.by(1)
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment