Commit 15aadc66 authored by Jacob Vosmaer (GitLab)'s avatar Jacob Vosmaer (GitLab) Committed by Douwe Maan

Make OperationService RPC's mandatory

parent 7e84f353
This diff is collapsed.
...@@ -64,6 +64,8 @@ module Gitlab ...@@ -64,6 +64,8 @@ module Gitlab
target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit) target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit)
Gitlab::Git::Branch.new(@repository, branch.name, target_commit.id, target_commit) Gitlab::Git::Branch.new(@repository, branch.name, target_commit.id, target_commit)
rescue GRPC::FailedPrecondition => ex
raise Gitlab::Git::Repository::InvalidRef, ex
end end
def user_delete_branch(branch_name, user) def user_delete_branch(branch_name, user)
...@@ -133,6 +135,8 @@ module Gitlab ...@@ -133,6 +135,8 @@ module Gitlab
request request
).branch_update ).branch_update
Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update) Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
rescue GRPC::FailedPrecondition => e
raise Gitlab::Git::CommitError, e
end end
def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
......
...@@ -35,30 +35,15 @@ feature 'Master deletes tag' do ...@@ -35,30 +35,15 @@ feature 'Master deletes tag' do
end end
context 'when pre-receive hook fails', :js do context 'when pre-receive hook fails', :js do
context 'when Gitaly operation_user_delete_tag feature is enabled' do before do
before do allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag)
allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag) .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
.and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
end
scenario 'shows the error message' do
delete_first_tag
expect(page).to have_content('Do not delete tags')
end
end end
context 'when Gitaly operation_user_delete_tag feature is disabled', :skip_gitaly_mock do scenario 'shows the error message' do
before do delete_first_tag
allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
.and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
end
scenario 'shows the error message' do
delete_first_tag
expect(page).to have_content('Do not delete tags') expect(page).to have_content('Do not delete tags')
end
end end
end end
......
...@@ -1971,21 +1971,15 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -1971,21 +1971,15 @@ describe Gitlab::Git::Repository, seed_helper: true do
end end
end end
context 'with gitaly' do it "calls Gitaly's OperationService" do
it "calls Gitaly's OperationService" do expect_any_instance_of(Gitlab::GitalyClient::OperationService)
expect_any_instance_of(Gitlab::GitalyClient::OperationService) .to receive(:user_ff_branch).with(user, source_sha, target_branch)
.to receive(:user_ff_branch).with(user, source_sha, target_branch) .and_return(nil)
.and_return(nil)
subject subject
end
it_behaves_like '#ff_merge'
end end
context 'without gitaly', :skip_gitaly_mock do it_behaves_like '#ff_merge'
it_behaves_like '#ff_merge'
end
end end
describe '#delete_all_refs_except' do describe '#delete_all_refs_except' do
...@@ -2308,92 +2302,95 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -2308,92 +2302,95 @@ describe Gitlab::Git::Repository, seed_helper: true do
expect { subject }.to raise_error(Gitlab::Git::CommandError, 'error') expect { subject }.to raise_error(Gitlab::Git::CommandError, 'error')
end end
end end
end
describe '#squash' do describe '#squash' do
let(:squash_id) { '1' } let(:squash_id) { '1' }
let(:branch_name) { 'fix' } let(:branch_name) { 'fix' }
let(:start_sha) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' } let(:start_sha) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
let(:end_sha) { '12d65c8dd2b2676fa3ac47d955accc085a37a9c1' } let(:end_sha) { '12d65c8dd2b2676fa3ac47d955accc085a37a9c1' }
subject do subject do
opts = { opts = {
branch: branch_name, branch: branch_name,
start_sha: start_sha, start_sha: start_sha,
end_sha: end_sha, end_sha: end_sha,
author: user, author: user,
message: 'Squash commit message' message: 'Squash commit message'
} }
repository.squash(user, squash_id, opts) repository.squash(user, squash_id, opts)
end
# Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
skip 'sparse checkout' do
let(:expected_files) { %w(files files/js files/js/application.js) }
it 'checks out only the files in the diff' do
allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
m.call(*args) do
worktree_path = args[0]
files_pattern = File.join(worktree_path, '**', '*')
expected = expected_files.map do |path|
File.expand_path(path, worktree_path)
end
expect(Dir[files_pattern]).to eq(expected)
end
end
subject
end end
context 'sparse checkout', :skip_gitaly_mock do context 'when the diff contains a rename' do
let(:expected_files) { %w(files files/js files/js/application.js) } let(:repo) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged }
let(:end_sha) { new_commit_move_file(repo).oid }
it 'checks out only the files in the diff' do after do
# Erase our commits so other tests get the original repo
repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
repo.references.update('refs/heads/master', SeedRepo::LastCommit::ID)
end
it 'does not include the renamed file in the sparse checkout' do
allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args| allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
m.call(*args) do m.call(*args) do
worktree_path = args[0] worktree_path = args[0]
files_pattern = File.join(worktree_path, '**', '*') files_pattern = File.join(worktree_path, '**', '*')
expected = expected_files.map do |path|
File.expand_path(path, worktree_path)
end
expect(Dir[files_pattern]).to eq(expected) expect(Dir[files_pattern]).not_to include('CHANGELOG')
expect(Dir[files_pattern]).not_to include('encoding/CHANGELOG')
end end
end end
subject subject
end end
context 'when the diff contains a rename' do
let(:repo) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged }
let(:end_sha) { new_commit_move_file(repo).oid }
after do
# Erase our commits so other tests get the original repo
repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
repo.references.update('refs/heads/master', SeedRepo::LastCommit::ID)
end
it 'does not include the renamed file in the sparse checkout' do
allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
m.call(*args) do
worktree_path = args[0]
files_pattern = File.join(worktree_path, '**', '*')
expect(Dir[files_pattern]).not_to include('CHANGELOG')
expect(Dir[files_pattern]).not_to include('encoding/CHANGELOG')
end
end
subject
end
end
end end
end
context 'with an ASCII-8BIT diff', :skip_gitaly_mock do # Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+✓ testme\n ======\n \n Sample repo for testing gitlab features\n" } skip 'with an ASCII-8BIT diff' do
let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+✓ testme\n ======\n \n Sample repo for testing gitlab features\n" }
it 'applies a ASCII-8BIT diff' do it 'applies a ASCII-8BIT diff' do
allow(repository).to receive(:run_git!).and_call_original allow(repository).to receive(:run_git!).and_call_original
allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT')) allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
expect(subject).to match(/\h{40}/) expect(subject).to match(/\h{40}/)
end
end end
end
context 'with trailing whitespace in an invalid patch', :skip_gitaly_mock do # Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+ \n ====== \n \n Sample repo for testing gitlab features\n" } skip 'with trailing whitespace in an invalid patch' do
let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+ \n ====== \n \n Sample repo for testing gitlab features\n" }
it 'does not include whitespace warnings in the error' do it 'does not include whitespace warnings in the error' do
allow(repository).to receive(:run_git!).and_call_original allow(repository).to receive(:run_git!).and_call_original
allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT')) allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
expect { subject }.to raise_error do |error| expect { subject }.to raise_error do |error|
expect(error).to be_a(described_class::GitError) expect(error).to be_a(described_class::GitError)
expect(error.message).not_to include('trailing whitespace') expect(error.message).not_to include('trailing whitespace')
end
end end
end end
end end
......
...@@ -1861,155 +1861,61 @@ describe Repository do ...@@ -1861,155 +1861,61 @@ describe Repository do
describe '#add_tag' do describe '#add_tag' do
let(:user) { build_stubbed(:user) } let(:user) { build_stubbed(:user) }
shared_examples 'adding tag' do context 'with a valid target' do
context 'with a valid target' do it 'creates the tag' do
it 'creates the tag' do repository.add_tag(user, '8.5', 'master', 'foo')
repository.add_tag(user, '8.5', 'master', 'foo')
tag = repository.find_tag('8.5')
expect(tag).to be_present
expect(tag.message).to eq('foo')
expect(tag.dereferenced_target.id).to eq(repository.commit('master').id)
end
it 'returns a Gitlab::Git::Tag object' do
tag = repository.add_tag(user, '8.5', 'master', 'foo')
expect(tag).to be_a(Gitlab::Git::Tag)
end
end
context 'with an invalid target' do tag = repository.find_tag('8.5')
it 'returns false' do expect(tag).to be_present
expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false expect(tag.message).to eq('foo')
end expect(tag.dereferenced_target.id).to eq(repository.commit('master').id)
end end
end
context 'when Gitaly operation_user_add_tag feature is enabled' do
it_behaves_like 'adding tag'
end
context 'when Gitaly operation_user_add_tag feature is disabled', :disable_gitaly do
it_behaves_like 'adding tag'
it 'passes commit SHA to pre-receive and update hooks and tag SHA to post-receive hook' do
pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', project)
update_hook = Gitlab::Git::Hook.new('update', project)
post_receive_hook = Gitlab::Git::Hook.new('post-receive', project)
allow(Gitlab::Git::Hook).to receive(:new)
.and_return(pre_receive_hook, update_hook, post_receive_hook)
allow(pre_receive_hook).to receive(:trigger).and_call_original
allow(update_hook).to receive(:trigger).and_call_original
allow(post_receive_hook).to receive(:trigger).and_call_original
it 'returns a Gitlab::Git::Tag object' do
tag = repository.add_tag(user, '8.5', 'master', 'foo') tag = repository.add_tag(user, '8.5', 'master', 'foo')
commit_sha = repository.commit('master').id expect(tag).to be_a(Gitlab::Git::Tag)
tag_sha = tag.target end
end
expect(pre_receive_hook).to have_received(:trigger) context 'with an invalid target' do
.with(anything, anything, anything, commit_sha, anything) it 'returns false' do
expect(update_hook).to have_received(:trigger) expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false
.with(anything, anything, anything, commit_sha, anything)
expect(post_receive_hook).to have_received(:trigger)
.with(anything, anything, anything, tag_sha, anything)
end end
end end
end end
describe '#rm_branch' do describe '#rm_branch' do
shared_examples "user deleting a branch" do it 'removes a branch' do
it 'removes a branch' do expect(repository).to receive(:before_remove_branch)
expect(repository).to receive(:before_remove_branch) expect(repository).to receive(:after_remove_branch)
expect(repository).to receive(:after_remove_branch)
repository.rm_branch(user, 'feature') repository.rm_branch(user, 'feature')
end
end end
context 'with gitaly enabled' do context 'when pre hooks failed' do
it_behaves_like "user deleting a branch" before do
allow_any_instance_of(Gitlab::GitalyClient::OperationService)
context 'when pre hooks failed' do .to receive(:user_delete_branch).and_raise(Gitlab::Git::PreReceiveError)
before do
allow_any_instance_of(Gitlab::GitalyClient::OperationService)
.to receive(:user_delete_branch).and_raise(Gitlab::Git::PreReceiveError)
end
it 'gets an error and does not delete the branch' do
expect do
repository.rm_branch(user, 'feature')
end.to raise_error(Gitlab::Git::PreReceiveError)
expect(repository.find_branch('feature')).not_to be_nil
end
end
end
context 'with gitaly disabled', :disable_gitaly do
it_behaves_like "user deleting a branch"
let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
let(:blank_sha) { '0000000000000000000000000000000000000000' }
context 'when pre hooks were successful' do
it 'runs without errors' do
expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
.with(git_user, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
expect { repository.rm_branch(user, 'feature') }.not_to raise_error
end
it 'deletes the branch' do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
expect { repository.rm_branch(user, 'feature') }.not_to raise_error
expect(repository.find_branch('feature')).to be_nil
end
end end
context 'when pre hooks failed' do it 'gets an error and does not delete the branch' do
it 'gets an error' do expect do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) repository.rm_branch(user, 'feature')
end.to raise_error(Gitlab::Git::PreReceiveError)
expect do
repository.rm_branch(user, 'feature')
end.to raise_error(Gitlab::Git::PreReceiveError)
end
it 'does not delete the branch' do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
expect do expect(repository.find_branch('feature')).not_to be_nil
repository.rm_branch(user, 'feature')
end.to raise_error(Gitlab::Git::PreReceiveError)
expect(repository.find_branch('feature')).not_to be_nil
end
end end
end end
end end
describe '#rm_tag' do describe '#rm_tag' do
shared_examples 'removing tag' do it 'removes a tag' do
it 'removes a tag' do expect(repository).to receive(:before_remove_tag)
expect(repository).to receive(:before_remove_tag)
repository.rm_tag(build_stubbed(:user), 'v1.1.0') repository.rm_tag(build_stubbed(:user), 'v1.1.0')
expect(repository.find_tag('v1.1.0')).to be_nil
end
end
context 'when Gitaly operation_user_delete_tag feature is enabled' do
it_behaves_like 'removing tag'
end
context 'when Gitaly operation_user_delete_tag feature is disabled', :skip_gitaly_mock do expect(repository.find_tag('v1.1.0')).to be_nil
it_behaves_like 'removing tag'
end end
end end
......
...@@ -71,17 +71,5 @@ describe Files::UpdateService do ...@@ -71,17 +71,5 @@ describe Files::UpdateService do
expect(results.data).to eq(new_contents) expect(results.data).to eq(new_contents)
end end
end end
context 'with gitaly disabled', :skip_gitaly_mock do
context 'when target branch is different than source branch' do
let(:branch_name) { "#{project.default_branch}-new" }
it 'fires hooks only once' do
expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original
subject.execute
end
end
end
end end
end end
...@@ -36,9 +36,9 @@ describe MergeRequests::RebaseService do ...@@ -36,9 +36,9 @@ describe MergeRequests::RebaseService do
end end
end end
context 'when unexpected error occurs', :disable_gitaly do context 'when unexpected error occurs' do
before do before do
allow(repository).to receive(:run_git!).and_raise('Something went wrong') allow(repository).to receive(:gitaly_operation_client).and_raise('Something went wrong')
end end
it 'saves a generic error message' do it 'saves a generic error message' do
...@@ -53,9 +53,9 @@ describe MergeRequests::RebaseService do ...@@ -53,9 +53,9 @@ describe MergeRequests::RebaseService do
end end
end end
context 'with git command failure', :disable_gitaly do context 'with git command failure' do
before do before do
allow(repository).to receive(:run_git!).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong') allow(repository).to receive(:gitaly_operation_client).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong')
end end
it 'saves a generic error message' do it 'saves a generic error message' do
...@@ -71,7 +71,7 @@ describe MergeRequests::RebaseService do ...@@ -71,7 +71,7 @@ describe MergeRequests::RebaseService do
end end
context 'valid params' do context 'valid params' do
shared_examples 'successful rebase' do describe 'successful rebase' do
before do before do
service.execute(merge_request) service.execute(merge_request)
end end
...@@ -97,26 +97,8 @@ describe MergeRequests::RebaseService do ...@@ -97,26 +97,8 @@ describe MergeRequests::RebaseService do
end end
end end
context 'when Gitaly rebase feature is enabled' do
it_behaves_like 'successful rebase'
end
context 'when Gitaly rebase feature is disabled', :disable_gitaly do
it_behaves_like 'successful rebase'
end
context 'git commands', :disable_gitaly do
it 'sets GL_REPOSITORY env variable when calling git commands' do
expect(repository).to receive(:popen).exactly(3)
.with(anything, anything, hash_including('GL_REPOSITORY'), anything)
.and_return(['', 0])
service.execute(merge_request)
end
end
context 'fork' do context 'fork' do
shared_examples 'successful fork rebase' do describe 'successful fork rebase' do
let(:forked_project) do let(:forked_project) do
fork_project(project, user, repository: true) fork_project(project, user, repository: true)
end end
...@@ -140,14 +122,6 @@ describe MergeRequests::RebaseService do ...@@ -140,14 +122,6 @@ describe MergeRequests::RebaseService do
expect(parent_sha).to eq(target_branch_sha) expect(parent_sha).to eq(target_branch_sha)
end end
end end
context 'when Gitaly rebase feature is enabled' do
it_behaves_like 'successful fork rebase'
end
context 'when Gitaly rebase feature is disabled', :disable_gitaly do
it_behaves_like 'successful fork rebase'
end
end end
end end
end end
......
...@@ -124,51 +124,6 @@ describe MergeRequests::SquashService do ...@@ -124,51 +124,6 @@ describe MergeRequests::SquashService do
message: a_string_including('squash')) message: a_string_including('squash'))
end end
end end
context 'with Gitaly disabled', :skip_gitaly_mock do
stages = {
'add worktree for squash' => 'worktree',
'configure sparse checkout' => 'config',
'get files in diff' => 'diff --name-only',
'check out target branch' => 'checkout',
'apply patch' => 'diff --binary',
'commit squashed changes' => 'commit',
'get SHA of squashed commit' => 'rev-parse'
}
stages.each do |stage, command|
context "when the #{stage} stage fails" do
before do
git_command = a_collection_containing_exactly(
a_string_starting_with("#{Gitlab.config.git.bin_path} #{command}")
).or(
a_collection_starting_with([Gitlab.config.git.bin_path] + command.split)
)
allow(repository).to receive(:popen).and_return(['', 0])
allow(repository).to receive(:popen).with(git_command, anything, anything, anything).and_return([error, 1])
end
it 'logs the stage and output' do
expect(service).to receive(:log_error).with(log_error)
expect(service).to receive(:log_error).with(error)
service.execute(merge_request)
end
it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error,
message: a_string_including('squash'))
end
it 'cleans up the temporary directory' do
expect(File.exist?(squash_dir_path)).to be(false)
service.execute(merge_request)
end
end
end
end
end end
context 'when any other exception is thrown' do context 'when any other exception is thrown' do
......
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