Merge branch 'unindirect-fetch-remote' into 'master'

Stop Repository#fetch_remote from using Gitlab::Shell

See merge request gitlab-org/gitlab-ce!22635

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
 

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

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.

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)

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

 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
 

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' }