From 7fbfb1998a0757b843d63cba0b43294a53891f95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 22 Mar 2019 14:20:24 +0100 Subject: [PATCH] Force to recreate all diffs on import If for whatever reason we fail to import MR data, subsequent run will fail as we try to insert duplicate data. Instead of trying to recover, lets delete all and retry again. --- .../recreate-all-diffs-on-import.yml | 5 ++ lib/gitlab/import/merge_request_helpers.rb | 16 ++-- .../importer/pull_request_importer_spec.rb | 78 +++++++++++-------- 3 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 changelogs/unreleased/recreate-all-diffs-on-import.yml diff --git a/changelogs/unreleased/recreate-all-diffs-on-import.yml b/changelogs/unreleased/recreate-all-diffs-on-import.yml new file mode 100644 index 00000000000..fd9124372f3 --- /dev/null +++ b/changelogs/unreleased/recreate-all-diffs-on-import.yml @@ -0,0 +1,5 @@ +--- +title: Force to recreate all MR diffs on import +merge_request: 26480 +author: +type: fixed diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb index fa3ff6c3f12..b3fe1fc0685 100644 --- a/lib/gitlab/import/merge_request_helpers.rb +++ b/lib/gitlab/import/merge_request_helpers.rb @@ -38,7 +38,6 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def insert_or_replace_git_data(merge_request, source_branch_sha, target_branch_sha, already_exists = false) # These fields are set so we can create the correct merge request # diffs. @@ -47,24 +46,21 @@ module Gitlab merge_request.keep_around_commit + # We force to recreate all diffs to replace all existing data + # We use `.all` as otherwise `dependent: :nullify` (the default) + # takes an effect + merge_request.merge_request_diffs.all.delete_all if already_exists + # MR diffs normally use an "after_save" hook to pull data from Git. # All of this happens in the transaction started by calling # create/save/etc. This in turn can lead to these transactions being # held open for much longer than necessary. To work around this we # first save the diff, then populate it. - diff = - if already_exists - merge_request.merge_request_diffs.take || - merge_request.merge_request_diffs.build - else - merge_request.merge_request_diffs.build - end - + diff = merge_request.merge_request_diffs.build diff.importing = true diff.save diff.save_git_content end - # rubocop: enable CodeReuse/ActiveRecord end end end diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 680de47de2b..2e4a7c36fb8 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -11,6 +11,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi let(:source_commit) { project.repository.commit('feature') } let(:target_commit) { project.repository.commit('master') } let(:milestone) { create(:milestone, project: project) } + let(:state) { :closed } let(:pull_request) do alice = Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice') @@ -26,13 +27,13 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi source_repository_id: 400, target_repository_id: 200, source_repository_owner: 'alice', - state: :closed, + state: state, milestone_number: milestone.iid, author: alice, assignee: alice, created_at: created_at, updated_at: updated_at, - merged_at: merged_at + merged_at: state == :closed && merged_at ) end @@ -260,58 +261,63 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi end it 'does not create the source branch if merge request is merged' do - mr, exists = importer.create_merge_request - - importer.insert_git_data(mr, exists) + mr = insert_git_data expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy end - it 'creates the source branch if merge request is open' do - mr, exists = importer.create_merge_request - mr.state = 'opened' - mr.save + context 'when merge request is open' do + let(:state) { :opened } - # Ensure the project creator is creating the branches because the - # merge request author may not have access to push to this - # repository. The project owner may also be a group. - allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original + it 'creates the source branch' do + # Ensure the project creator is creating the branches because the + # merge request author may not have access to push to this + # repository. The project owner may also be a group. + allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original - importer.insert_git_data(mr, exists) + mr = insert_git_data - expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy - expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy - end + expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + end - it 'ignores Git errors when creating a branch' do - mr, exists = importer.create_merge_request - mr.state = 'opened' - mr.save + it 'is able to retry on pre-receive errors' do + expect(importer).to receive(:insert_or_replace_git_data).twice.and_call_original + expect(project.repository).to receive(:add_branch).and_raise('exception') - expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError) - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect { insert_git_data }.to raise_error('exception') - importer.insert_git_data(mr, exists) + expect(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original - expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey - expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + mr = insert_git_data + + expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + expect(mr.merge_request_diffs).to be_one + end + + it 'ignores Git command errors when creating a branch' do + expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError) + expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + + mr = insert_git_data + + expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + end end it 'creates the merge request diffs' do - mr, exists = importer.create_merge_request - - importer.insert_git_data(mr, exists) + mr = insert_git_data expect(mr.merge_request_diffs.exists?).to eq(true) end it 'creates the merge request diff commits' do - mr, exists = importer.create_merge_request + mr = insert_git_data - importer.insert_git_data(mr, exists) - - diff = mr.merge_request_diffs.take + diff = mr.merge_request_diffs.reload.first expect(diff.merge_request_diff_commits.exists?).to eq(true) end @@ -327,5 +333,11 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi expect(mr.merge_request_diffs.exists?).to eq(true) end end + + def insert_git_data + mr, exists = importer.create_merge_request + importer.insert_git_data(mr, exists) + mr + end end end