Skip to content
Snippets Groups Projects
Commit e002aea0 authored by Douwe Maan's avatar Douwe Maan Committed by GitLab Release Tools Bot
Browse files

Merge branch '1979-1-2-multi-block-approval-data-migration' into 'master'

[Data migration] Sync Approver/ApproverGroup with ApprovalRule/ApprovalRuleMember

Closes #1979

See merge request gitlab-org/gitlab-ee!8669

(cherry picked from commit b25add07)

c89c4d82 Sync approvals_required count setting to rules
8cabf62b Finalize approval state when merged
6f6f91d1 Sync code owner changes to approval rules
2714d05a Add background task to migrate projects and MR
840cb644 Add migration progress checker
375bf7d9 Set MR rule to point to project MR
d3458463 Check if specific type of background migration are done
fc73f551 Update progress checker
3487761d Assign code owners in data migration
b855cb6b Remove finalize MR approval rule in data migration
489c337b Remove further unused code
d79b7d3f Bulk migrate multiple project's MR
b5f388cc Refactor code around transaction
18bbba99 Allow git access
1adde758 Avoid pushing too many jobs in one go
0c3092e9 Add spec to handle destroyed targe...
parent e497e43f
No related branches found
No related tags found
1 merge request!9154Prepare 11.7 RC6 EE release
Pipeline #43340768 failed
Showing
with 405 additions and 1 deletion
......@@ -66,3 +66,5 @@ def set_projects!
end
end
end
MergeRequests::CreateService.include(EE::MergeRequests::CreateService)
......@@ -52,3 +52,5 @@ def create_event(merge_request)
end
end
end
MergeRequests::PostMergeService.prepend(EE::MergeRequests::PostMergeService)
......@@ -3,6 +3,8 @@
class ApprovalMergeRequestRule < ApplicationRecord
include ApprovalRuleLike
DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
scope :regular, -> { where(code_owner: false) }
scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes
......
......@@ -4,6 +4,8 @@ class Approver < ActiveRecord::Base
belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :user
include ApproverMigrateHook
validates :user, presence: true
def find_by_user_id(user_id)
......
......@@ -4,6 +4,8 @@ class ApproverGroup < ActiveRecord::Base
belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :group
include ApproverMigrateHook
validates :group, presence: true
delegate :users, to: :group
......
......@@ -8,7 +8,7 @@ module ApprovalRuleLike
included do
has_and_belongs_to_many :users
has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
has_many :group_users, through: :groups, source: :users
has_many :group_users, -> { distinct }, through: :groups, source: :users
validates :name, presence: true
end
......
# frozen_string_literal: true
#
# Sync up old models (Approver and ApproverGroup)
# to new models (ApprovalMergeRequestRule and ApprovalProjectRule)
#
# TODO: remove once #1979 is closed
module ApproverMigrateHook
extend ActiveSupport::Concern
included do
after_commit :schedule_approval_migration, on: [:create, :destroy]
end
def schedule_approval_migration
# After merge, approval information is frozen
return if target.is_a?(MergeRequest) && target.merged?
Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform(target.class.name, target.id, sync_code_owner_rule: false)
end
end
......@@ -81,5 +81,22 @@ def compare_license_management_reports
compare_reports(::Ci::CompareLicenseManagementReportsService)
end
def sync_code_owners_with_approvers
return if merged?
owners = code_owners
if owners.present?
ActiveRecord::Base.transaction do
rule = approval_rules.code_owner.first
rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
rule.users = code_owners.uniq
end
else
approval_rules.code_owner.delete_all
end
end
end
end
# frozen_string_literal: true
module EE
module MergeRequests
module CreateService
extend ::Gitlab::Utils::Override
override :after_create
def after_create(issuable)
super
issuable.sync_code_owners_with_approvers
end
end
end
end
# frozen_string_literal: true
module EE
module MergeRequests
module PostMergeService
extend ::Gitlab::Utils::Override
override :execute
def execute(merge_request)
super
ApprovalRules::FinalizeService.new(merge_request).execute
end
end
end
end
......@@ -53,6 +53,8 @@ def update_approvers
new_code_owners = merge_request.code_owners - previous_code_owners
create_approvers(merge_request, new_code_owners)
merge_request.sync_code_owners_with_approvers
end
results
......
......@@ -25,6 +25,8 @@ def execute(merge_request)
cleanup_approvers(merge_request, reload: true)
end
sync_approval_rules(merge_request)
merge_request
end
......@@ -42,6 +44,14 @@ def reset_approvals(merge_request)
merge_request.approvals.delete_all if target_project.reset_approvals_on_push
end
# TODO remove after #1979 is closed
def sync_approval_rules(merge_request)
return if merge_request.merged?
return unless merge_request.previous_changes.include?(:approvals_before_merge)
merge_request.approval_rules.regular.update_all(approvals_required: merge_request.approvals_before_merge)
end
end
end
end
......@@ -37,6 +37,8 @@ def execute
sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled?
project.import_state.force_import_job! if params[:mirror].present? && project.mirror?
sync_approval_rules
end
result
......@@ -67,6 +69,13 @@ def log_audit_events
def sync_wiki_on_enable
::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
end
# TODO remove after #1979 is closed
def sync_approval_rules
return unless project.previous_changes.include?(:approvals_before_merge)
project.approval_rules.update_all(approvals_required: project.approvals_before_merge)
end
end
end
end
# frozen_string_literal: true
class MigrateProjectApprovers < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
BATCH_SIZE = 3000
class MergeRequest < ActiveRecord::Base
include ::EachBatch
self.table_name = 'merge_requests'
end
class Approver < ActiveRecord::Base
self.table_name = 'approvers'
end
class ApproverGroup < ActiveRecord::Base
self.table_name = 'approver_groups'
end
def up
get_project_ids.each do |project_id|
Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform('Project', project_id)
end
bulk_queue_background_migration_jobs_by_range(MergeRequest, 'MigrateApproverToApprovalRulesInBatch', batch_size: BATCH_SIZE)
check_time = Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress::RESCHEDULE_DELAY
BackgroundMigrationWorker.bulk_perform_in(check_time, [['MigrateApproverToApprovalRulesCheckProgress']])
end
def down
end
private
def get_project_ids
results = ActiveRecord::Base.connection.exec_query <<~SQL
SELECT DISTINCT target_id FROM (
SELECT target_id FROM approvers WHERE target_type='Project'
UNION
SELECT target_id FROM approver_groups WHERE target_type='Project'
) t
SQL
results.rows.flatten
end
end
......@@ -77,6 +77,8 @@ def handle_merge_request_errors!(errors)
requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
end
put 'approvers' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/8883')
merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
......
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
# A Project/MergeRequest level migration, aiming to convert existing data
# (from approvers, approver_groups tables)
# to new rule based schema.
class MigrateApproverToApprovalRules
include Gitlab::Utils::StrongMemoize
class Approver < ActiveRecord::Base
self.table_name = 'approvers'
end
class ApproverGroup < ActiveRecord::Base
self.table_name = 'approver_groups'
end
class ApprovalMergeRequestRule < ActiveRecord::Base
self.table_name = 'approval_merge_request_rules'
belongs_to :merge_request
scope :code_owner, -> { where(code_owner: true) }
scope :regular, -> { where(code_owner: false) } # Non code owner rule
has_and_belongs_to_many :users
has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_merge_request_rules_groups
has_one :approval_merge_request_rule_source
has_one :approval_project_rule, through: :approval_merge_request_rule_source
def project
merge_request.target_project
end
end
class ApprovalMergeRequestRuleSource < ActiveRecord::Base
self.table_name = 'approval_merge_request_rule_sources'
belongs_to :approval_merge_request_rule
belongs_to :approval_project_rule
end
class ApprovalProjectRule < ActiveRecord::Base
self.table_name = 'approval_project_rules'
belongs_to :project
has_and_belongs_to_many :users
has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_project_rules_groups
scope :regular, -> { all }
end
class MergeRequest < ActiveRecord::Base
self.table_name = 'merge_requests'
belongs_to :target_project, class_name: "Project"
has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
def approver_ids
@approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck('distinct user_id')
end
def approver_group_ids
@approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck('distinct group_id')
end
def sync_code_owners_with_approvers
return if state == 'merged' || state == 'closed'
Gitlab::GitalyClient.allow_n_plus_1_calls do
::MergeRequest.find(id).sync_code_owners_with_approvers
end
end
end
class Project < ActiveRecord::Base
self.table_name = 'projects'
has_many :approval_rules, class_name: 'ApprovalProjectRule'
def approver_ids
@approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck('distinct user_id')
end
def approver_group_ids
@approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck('distinct group_id')
end
end
class User < ActiveRecord::Base
self.table_name = 'users'
end
ALLOWED_TARGET_TYPES = %w{MergeRequest Project}.freeze
# @param target_type [String] class of target, either 'MergeRequest' or 'Project'
# @param target_id [Integer] id of target
def perform(target_type, target_id, sync_code_owner_rule: true)
@target_type = target_type
@target_id = target_id
@sync_code_owner_rule = sync_code_owner_rule
raise "Incorrect target_type #{target_type}" unless ALLOWED_TARGET_TYPES.include?(@target_type)
ActiveRecord::Base.transaction do
case target
when MergeRequest
handle_merge_request
when Project
handle_project
end
end
end
private
def handle_merge_request
if rule = sync_rule
rule.approval_project_rule = target.target_project.approval_rules.regular.first
end
target.sync_code_owners_with_approvers if @sync_code_owner_rule
end
def handle_project
sync_rule
end
def sync_rule
unless approvers_exists?
target.approval_rules.regular.delete_all
return
end
rule = first_or_initialize
rule.update(user_ids: target.approver_ids, group_ids: target.approver_group_ids)
rule
end
def target
strong_memoize(:target) do
case @target_type
when 'MergeRequest'
MergeRequest.find_by(id: @target_id)
when 'Project'
Project.find_by(id: @target_id)
end
end
end
def first_or_initialize
rule = target.approval_rules.regular.first_or_initialize
unless rule.persisted?
rule.name ||= ApprovalRuleLike::DEFAULT_NAME
rule.approvals_required = target.approvals_before_merge || 0
rule.save!
end
rule
end
def approvers_exists?
target.approver_ids.any? || target.approver_group_ids.any?
end
end
end
end
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
class MigrateApproverToApprovalRulesCheckProgress
RESCHEDULE_DELAY = 1.day
def perform
if remaining?
BackgroundMigrationWorker.perform_in(RESCHEDULE_DELAY, self.class.name)
else
Feature.enable(:approval_rule)
end
end
private
def remaining?
Gitlab::BackgroundMigration.exists?('MigrateApproverToApprovalRulesInBatch')
end
end
end
end
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
class MigrateApproverToApprovalRulesInBatch
class MergeRequest < ActiveRecord::Base
self.table_name = 'merge_requests'
end
def perform(start_id, end_id)
merge_request_ids = MergeRequest.where('id >= ? AND id <= ?', start_id, end_id).pluck(:id)
merge_request_ids.each do |merge_request_id|
MigrateApproverToApprovalRules.new.perform('MergeRequest', merge_request_id)
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress do
context 'when there is MigrateApproverToApprovalRulesInBatch jobs' do
it 'reschedules check' do
allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(true)
expect(BackgroundMigrationWorker).to receive(:perform_in).with(described_class::RESCHEDULE_DELAY, described_class.name)
described_class.new.perform
end
end
context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do
it 'enables feature' do
allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
expect(Feature).to receive(:enable).with(:approval_rule)
described_class.new.perform
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesInBatch do
context 'when there is no more MigrateApproverToApprovalRules jobs' do
let(:job) { double(:job) }
let(:project) { create(:project) }
it 'migrates individual target' do
allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(job)
merge_requests = create_list(:merge_request, 3)
expect(job).to receive(:perform).exactly(3).times
described_class.new.perform(merge_requests.first.id, merge_requests.last.id)
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