From 668e5d63fa186112b7ebde2464d026b2a19aa834 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 26 Oct 2018 23:30:24 +0100 Subject: [PATCH] Stop Repository#fetch_remote from using Gitlab::Shell This indirection doesn't provide any value, so remove it --- app/models/repository.rb | 4 - lib/gitlab/git/repository.rb | 20 +++ lib/gitlab/shell.rb | 17 --- spec/lib/gitlab/git/repository_spec.rb | 21 +++ .../gitaly_client/repository_service_spec.rb | 61 ++++++++- spec/lib/gitlab/shell_spec.rb | 120 ------------------ 6 files changed, 98 insertions(+), 145 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 37a1dd64052..ee5579329a8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -912,10 +912,6 @@ class Repository async_remove_remote(remote_name) if tmp_remote_name end - def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false, prune: true) - gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune) - end - def async_remove_remote(remote_name) return unless remote_name diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index fcc92341c40..20cd257bb98 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -720,6 +720,26 @@ module Gitlab end end + # Fetch remote for repository + # + # remote - remote name + # ssh_auth - SSH known_hosts data and a private key to use for public-key authentication + # forced - should we use --force flag? + # no_tags - should we use --no-tags flag? + # prune - should we use --prune flag? + def fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, prune: true) + wrapped_gitaly_errors do + gitaly_repository_client.fetch_remote( + remote, + ssh_auth: ssh_auth, + forced: forced, + no_tags: no_tags, + prune: prune, + timeout: GITLAB_PROJECTS_TIMEOUT + ) + end + end + def blob_at(sha, path) Gitlab::Git::Blob.find(self, sha, path) unless Gitlab::Git.blank_ref?(sha) end diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 16c1edb2f11..c6a6fb9b5ce 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -108,23 +108,6 @@ module Gitlab success end - # Fetch remote for repository - # - # repository - an instance of Git::Repository - # remote - remote name - # ssh_auth - SSH known_hosts data and a private key to use for public-key authentication - # forced - should we use --force flag? - # no_tags - should we use --no-tags flag? - # - # Ex. - # fetch_remote(my_repo, "upstream") - # - def fetch_remote(repository, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true) - wrapped_gitaly_errors do - repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout, prune: prune) - end - end - # Move repository reroutes to mv_directory which is an alias for # mv_namespace. Given the underlying implementation is a move action, # indescriminate of what the folders might be. diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 9a443fa7f20..54291e847d8 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -476,6 +476,27 @@ describe Gitlab::Git::Repository, :seed_helper do end end + describe '#fetch_remote' do + it 'delegates to the gitaly RepositoryService' do + ssh_auth = double(:ssh_auth) + expected_opts = { + ssh_auth: ssh_auth, + forced: true, + no_tags: true, + timeout: described_class::GITLAB_PROJECTS_TIMEOUT, + prune: false + } + + expect(repository.gitaly_repository_client).to receive(:fetch_remote).with('remote-name', expected_opts) + + repository.fetch_remote('remote-name', ssh_auth: ssh_auth, forced: true, no_tags: true, prune: false) + end + + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RepositoryService, :fetch_remote do + subject { repository.fetch_remote('remote-name') } + end + end + describe '#find_remote_root_ref' do it 'gets the remote root ref from GitalyClient' do expect_any_instance_of(Gitlab::GitalyClient::RemoteService) diff --git a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb index 1547d447197..d605fcbafee 100644 --- a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Gitlab::GitalyClient::RepositoryService do + using RSpec::Parameterized::TableSyntax + let(:project) { create(:project) } let(:storage_name) { project.repository_storage } let(:relative_path) { project.disk_path + '.git' } @@ -107,16 +109,67 @@ describe Gitlab::GitalyClient::RepositoryService do end describe '#fetch_remote' do - let(:ssh_auth) { double(:ssh_auth, ssh_import?: true, ssh_key_auth?: false, ssh_known_hosts: nil) } - let(:import_url) { 'ssh://example.com' } + let(:remote) { 'remote-name' } it 'sends a fetch_remote_request message' do + expected_request = gitaly_request_with_params( + remote: remote, + ssh_key: '', + known_hosts: '', + force: false, + no_tags: false, + no_prune: false + ) + expect_any_instance_of(Gitaly::RepositoryService::Stub) .to receive(:fetch_remote) - .with(gitaly_request_with_params(no_prune: false), kind_of(Hash)) + .with(expected_request, kind_of(Hash)) .and_return(double(value: true)) - client.fetch_remote(import_url, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 60) + client.fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, timeout: 1) + end + + context 'SSH auth' do + where(:ssh_import, :ssh_key_auth, :ssh_private_key, :ssh_known_hosts, :expected_params) do + false | false | 'key' | 'known_hosts' | {} + false | true | 'key' | 'known_hosts' | {} + true | false | 'key' | 'known_hosts' | { known_hosts: 'known_hosts' } + true | true | 'key' | 'known_hosts' | { ssh_key: 'key', known_hosts: 'known_hosts' } + true | true | 'key' | nil | { ssh_key: 'key' } + true | true | nil | 'known_hosts' | { known_hosts: 'known_hosts' } + true | true | nil | nil | {} + true | true | '' | '' | {} + end + + with_them do + let(:ssh_auth) do + double( + :ssh_auth, + ssh_import?: ssh_import, + ssh_key_auth?: ssh_key_auth, + ssh_private_key: ssh_private_key, + ssh_known_hosts: ssh_known_hosts + ) + end + + it do + expected_request = gitaly_request_with_params({ + remote: remote, + ssh_key: '', + known_hosts: '', + force: false, + no_tags: false, + no_prune: false + }.update(expected_params)) + + expect_any_instance_of(Gitaly::RepositoryService::Stub) + .to receive(:fetch_remote) + .with(expected_request, kind_of(Hash)) + .and_return(double(value: true)) + + client.fetch_remote(remote, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 1) + end + end end end diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index b1b7c427313..6ce9d515a0f 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/spec/lib/gitlab/shell_spec.rb @@ -498,126 +498,6 @@ describe Gitlab::Shell do end end - describe '#fetch_remote' do - def fetch_remote(ssh_auth = nil, prune = true) - gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', ssh_auth: ssh_auth, prune: prune) - end - - def expect_call(fail, options = {}) - receive_fetch_remote = - if fail - receive(:fetch_remote).and_raise(GRPC::NotFound) - else - receive(:fetch_remote).and_return(true) - end - - expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive_fetch_remote - end - - def build_ssh_auth(opts = {}) - defaults = { - ssh_import?: true, - ssh_key_auth?: false, - ssh_known_hosts: nil, - ssh_private_key: nil - } - - double(:ssh_auth, defaults.merge(opts)) - end - - it 'returns true when the command succeeds' do - expect_call(false, force: false, tags: true, prune: true) - - expect(fetch_remote).to be_truthy - end - - it 'returns true when the command succeeds' do - expect_call(false, force: false, tags: true, prune: false) - - expect(fetch_remote(nil, false)).to be_truthy - end - - it 'raises an exception when the command fails' do - expect_call(true, force: false, tags: true, prune: true) - - expect { fetch_remote }.to raise_error(Gitlab::Shell::Error) - end - - it 'allows forced and no_tags to be changed' do - expect_call(false, force: true, tags: false, prune: true) - - result = gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', forced: true, no_tags: true, prune: true) - expect(result).to be_truthy - end - - context 'SSH auth' do - it 'passes the SSH key if specified' do - expect_call(false, force: false, tags: true, prune: true, ssh_key: 'foo') - - ssh_auth = build_ssh_auth(ssh_key_auth?: true, ssh_private_key: 'foo') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - - it 'does not pass an empty SSH key' do - expect_call(false, force: false, tags: true, prune: true) - - ssh_auth = build_ssh_auth(ssh_key_auth: true, ssh_private_key: '') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - - it 'does not pass the key unless SSH key auth is to be used' do - expect_call(false, force: false, tags: true, prune: true) - - ssh_auth = build_ssh_auth(ssh_key_auth: false, ssh_private_key: 'foo') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - - it 'passes the known_hosts data if specified' do - expect_call(false, force: false, tags: true, prune: true, known_hosts: 'foo') - - ssh_auth = build_ssh_auth(ssh_known_hosts: 'foo') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - - it 'does not pass empty known_hosts data' do - expect_call(false, force: false, tags: true, prune: true) - - ssh_auth = build_ssh_auth(ssh_known_hosts: '') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - - it 'does not pass known_hosts data unless SSH is to be used' do - expect_call(false, force: false, tags: true, prune: true) - - ssh_auth = build_ssh_auth(ssh_import?: false, ssh_known_hosts: 'foo') - - expect(fetch_remote(ssh_auth)).to be_truthy - end - end - - context 'gitaly call' do - let(:remote_name) { 'remote-name' } - let(:ssh_auth) { double(:ssh_auth) } - - subject do - gitlab_shell.fetch_remote(repository.raw_repository, remote_name, - forced: true, no_tags: true, ssh_auth: ssh_auth) - end - - it 'passes the correct params to the gitaly service' do - expect(repository.gitaly_repository_client).to receive(:fetch_remote) - .with(remote_name, ssh_auth: ssh_auth, forced: true, no_tags: true, prune: true, timeout: timeout) - - subject - end - end - end - describe '#import_repository' do let(:import_url) { 'https://gitlab.com/gitlab-org/gitlab-ce.git' }