Merge branch 'pks-gitaly-fetch-remote-params' into 'master'

Convert users of `FetchRemote` to use `RemoteParams` [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57118

app/models/repository.rb

@@ -938,6 +938,8 @@ class Repository
   end
 
   def fetch_as_mirror(url, forced: false, refmap: :all_refs, remote_name: nil, prune: true)
+    return fetch_remote(remote_name, url: url, refmap: refmap, forced: forced, prune: prune) if Feature.enabled?(:fetch_remote_params, project, default_enabled: :yaml)
+
     unless remote_name
       remote_name = "tmp-#{SecureRandom.hex}"
       tmp_remote_name = true

config/feature_flags/development/fetch_remote_params.yml

+---
+name: fetch_remote_params
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325528
+milestone: '13.12'
+type: development
+group: group::gitaly
+default_enabled: false

lib/gitlab/git/repository.rb

@@ -797,15 +797,19 @@ module Gitlab
       # Fetch remote for repository
       #
       # remote - remote name
+      # url - URL of the remote to fetch. `remote` is not used in this case.
+      # refmap - if url is given, determines which references should get fetched where
       # 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?
       # check_tags_changed - should we ask gitaly to calculate whether any tags changed?
-      def fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, prune: true, check_tags_changed: false)
+      def fetch_remote(remote, url: nil, refmap: nil, ssh_auth: nil, forced: false, no_tags: false, prune: true, check_tags_changed: false)
         wrapped_gitaly_errors do
           gitaly_repository_client.fetch_remote(
             remote,
+            url: url,
+            refmap: refmap,
             ssh_auth: ssh_auth,
             forced: forced,
             no_tags: no_tags,

lib/gitlab/gitaly_client/repository_service.rb

@@ -70,13 +70,21 @@ module Gitlab
         end.join
       end
 
-      def fetch_remote(remote, ssh_auth:, forced:, no_tags:, timeout:, prune: true, check_tags_changed: false)
+      # rubocop: disable Metrics/ParameterLists
+      # The `remote` parameter is going away soonish anyway, at which point the
+      # Rubocop warning can be enabled again.
+      def fetch_remote(remote, url:, refmap:, ssh_auth:, forced:, no_tags:, timeout:, prune: true, check_tags_changed: false)
         request = Gitaly::FetchRemoteRequest.new(
           repository: @gitaly_repo, remote: remote, force: forced,
           no_tags: no_tags, timeout: timeout, no_prune: !prune,
           check_tags_changed: check_tags_changed
         )
 
+        if url
+          request.remote_params = Gitaly::Remote.new(url: url,
+                                                     mirror_refmaps: Array.wrap(refmap).map(&:to_s))
+        end
+
         if ssh_auth&.ssh_mirror_url?
           if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present?
             request.ssh_key = ssh_auth.ssh_private_key
@@ -89,6 +97,7 @@ module Gitlab
 
         GitalyClient.call(@storage, :repository_service, :fetch_remote, request, timeout: GitalyClient.long_timeout)
       end
+      # rubocop: enable Metrics/ParameterLists
 
       def create_repository
         request = Gitaly::CreateRepositoryRequest.new(repository: @gitaly_repo)

lib/gitlab/github_import/importer/pull_requests_importer.rb

@@ -36,7 +36,11 @@ module Gitlab
           # updating the timestamp.
           project.update_column(:last_repository_updated_at, Time.zone.now)
 
-          project.repository.fetch_remote('github', forced: false)
+          if Feature.enabled?(:fetch_remote_params, project, default_enabled: :yaml)
+            project.repository.fetch_remote('github', url: project.import_url, refmap: Gitlab::GithubImport.refmap, forced: false)
+          else
+            project.repository.fetch_remote('github', forced: false)
+          end
 
           pname = project.path_with_namespace
 

spec/lib/gitlab/git/repository_spec.rb

@@ -521,7 +521,9 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
         no_tags: true,
         timeout: described_class::GITLAB_PROJECTS_TIMEOUT,
         prune: false,
-        check_tags_changed: false
+        check_tags_changed: false,
+        url: nil,
+        refmap: nil
       }
 
       expect(repository.gitaly_repository_client).to receive(:fetch_remote).with('remote-name', expected_opts)

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -122,67 +122,89 @@ RSpec.describe Gitlab::GitalyClient::RepositoryService do
   end
 
   describe '#fetch_remote' do
-    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,
-        check_tags_changed: false
-      )
+    shared_examples 'a fetch' do
+      it 'sends a fetch_remote_request message' do
+        expected_remote_params = Gitaly::Remote.new(
+          url: url, http_authorization_header: "", mirror_refmaps: [])
+
+        expected_request = gitaly_request_with_params(
+          remote: remote,
+          remote_params: url ? expected_remote_params : nil,
+          ssh_key: '',
+          known_hosts: '',
+          force: false,
+          no_tags: false,
+          no_prune: false,
+          check_tags_changed: false
+        )
+
+        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, url: url, refmap: nil, ssh_auth: nil, forced: false, no_tags: false, timeout: 1, check_tags_changed: false)
+      end
 
-      expect_any_instance_of(Gitaly::RepositoryService::Stub)
-        .to receive(:fetch_remote)
-        .with(expected_request, kind_of(Hash))
-        .and_return(double(value: true))
+      context 'SSH auth' do
+        where(:ssh_mirror_url, :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
 
-      client.fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, timeout: 1, check_tags_changed: false)
+        with_them do
+          let(:ssh_auth) do
+            double(
+              :ssh_auth,
+              ssh_mirror_url?: ssh_mirror_url,
+              ssh_key_auth?: ssh_key_auth,
+              ssh_private_key: ssh_private_key,
+              ssh_known_hosts: ssh_known_hosts
+            )
+          end
+
+          it do
+            expected_remote_params = Gitaly::Remote.new(
+              url: url, http_authorization_header: "", mirror_refmaps: [])
+
+            expected_request = gitaly_request_with_params({
+              remote: remote,
+              remote_params: url ? expected_remote_params : nil,
+              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, url: url, refmap: nil, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 1)
+          end
+        end
+      end
     end
 
-    context 'SSH auth' do
-      where(:ssh_mirror_url, :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  | ''    | ''            | {}
+    context 'with remote' do
+      it_behaves_like 'a fetch' do
+        let(:remote) { 'remote-name' }
+        let(:url) { nil }
       end
+    end
 
-      with_them do
-        let(:ssh_auth) do
-          double(
-            :ssh_auth,
-            ssh_mirror_url?: ssh_mirror_url,
-            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
+    context 'with URL' do
+      it_behaves_like 'a fetch' do
+        let(:remote) { "" }
+        let(:url) { 'https://example.com/git/repo.git' }
       end
     end
   end

spec/lib/gitlab/github_import/importer/pull_requests_importer_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::GithubImport::Importer::PullRequestsImporter do
-  let(:project) { create(:project, import_source: 'foo/bar') }
+  let(:url) { 'https://github.com/foo/bar.git' }
+  let(:project) { create(:project, import_source: 'foo/bar', import_url: url) }
   let(:client) { double(:client) }
 
   let(:pull_request) do
@@ -147,14 +148,10 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsImporter do
     end
   end
 
-  describe '#update_repository' do
+  shared_examples '#update_repository' do
     it 'updates the repository' do
       importer = described_class.new(project, client)
 
-      expect(project.repository)
-        .to receive(:fetch_remote)
-        .with('github', forced: false)
-
       expect_next_instance_of(Gitlab::Import::Logger) do |logger|
         expect(logger)
           .to receive(:info)
@@ -173,6 +170,28 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsImporter do
     end
   end
 
+  describe '#update_repository with :fetch_remote_params enabled' do
+    before do
+      stub_feature_flags(fetch_remote_params: true)
+      expect(project.repository)
+        .to receive(:fetch_remote)
+        .with('github', forced: false, url: url, refmap: Gitlab::GithubImport.refmap)
+    end
+
+    it_behaves_like '#update_repository'
+  end
+
+  describe '#update_repository with :fetch_remote_params disabled' do
+    before do
+      stub_feature_flags(fetch_remote_params: false)
+      expect(project.repository)
+        .to receive(:fetch_remote)
+        .with('github', forced: false)
+    end
+
+    it_behaves_like '#update_repository'
+  end
+
   describe '#update_repository?' do
     let(:importer) { described_class.new(project, client) }
 

spec/models/repository_spec.rb

@@ -1123,6 +1123,70 @@ RSpec.describe Repository do
     end
   end
 
+  describe '#fetch_as_mirror' do
+    let(:url) { "http://example.com" }
+
+    context 'when :fetch_remote_params is enabled' do
+      let(:remote_name) { "remote-name" }
+
+      before do
+        stub_feature_flags(fetch_remote_params: true)
+      end
+
+      it 'fetches the URL without creating a remote' do
+        expect(repository).not_to receive(:add_remote)
+        expect(repository)
+          .to receive(:fetch_remote)
+          .with(remote_name, url: url, forced: false, prune: true, refmap: :all_refs)
+          .and_return(nil)
+
+        repository.fetch_as_mirror(url, remote_name: remote_name)
+      end
+    end
+
+    context 'when :fetch_remote_params is disabled' do
+      before do
+        stub_feature_flags(fetch_remote_params: false)
+      end
+
+      shared_examples 'a fetch' do
+        it 'adds and fetches a remote' do
+          expect(repository)
+            .to receive(:add_remote)
+            .with(expected_remote, url, mirror_refmap: :all_refs)
+            .and_return(nil)
+          expect(repository)
+            .to receive(:fetch_remote)
+            .with(expected_remote, forced: false, prune: true)
+            .and_return(nil)
+
+          repository.fetch_as_mirror(url, remote_name: remote_name)
+        end
+      end
+
+      context 'with temporary remote' do
+        let(:remote_name) { nil }
+        let(:expected_remote_suffix) { "123456" }
+        let(:expected_remote) { "tmp-#{expected_remote_suffix}" }
+
+        before do
+          expect(repository)
+            .to receive(:async_remove_remote).with(expected_remote).and_return(nil)
+          allow(SecureRandom).to receive(:hex).and_return(expected_remote_suffix)
+        end
+
+        it_behaves_like 'a fetch'
+      end
+
+      context 'with remote name' do
+        let(:remote_name) { "foo" }
+        let(:expected_remote) { "foo" }
+
+        it_behaves_like 'a fetch'
+      end
+    end
+  end
+
   describe '#fetch_ref' do
     let(:broken_repository) { create(:project, :broken_storage).repository }