Merge branch 'pks-update-remote-mirror-drop-remote-name' into 'master'

Remove `ref_name` parameter in `RemoteMirror`

See merge request gitlab-org/gitlab!67668

app/models/remote_mirror.rb

@@ -22,8 +22,6 @@ class RemoteMirror < ApplicationRecord
 
   validates :url, presence: true, public_url: { schemes: %w(ssh git http https), allow_blank: true, enforce_user: true }
 
-  before_save :set_new_remote_name, if: :mirror_url_changed?
-
   after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.mirror_available }
   after_update :reset_fields, if: :saved_change_to_mirror_url?
 
@@ -85,10 +83,6 @@ class RemoteMirror < ApplicationRecord
     end
   end
 
-  def remote_name
-    super || fallback_remote_name
-  end
-
   def update_failed?
     update_status == 'failed'
   end
@@ -100,7 +94,6 @@ class RemoteMirror < ApplicationRecord
   def update_repository
     Gitlab::Git::RemoteMirror.new(
       project.repository.raw,
-      remote_name,
       remote_url,
       **options_for_update
     ).update
@@ -268,12 +261,6 @@ class RemoteMirror < ApplicationRecord
     super
   end
 
-  def fallback_remote_name
-    return unless id
-
-    "remote_mirror_#{id}"
-  end
-
   def recently_scheduled?
     return false unless self.last_update_started_at
 
@@ -296,10 +283,6 @@ class RemoteMirror < ApplicationRecord
     project.update(remote_mirror_available_overridden: enabled)
   end
 
-  def set_new_remote_name
-    self.remote_name = "remote_mirror_#{SecureRandom.hex}"
-  end
-
   def mirror_url_changed?
     url_changed? || attribute_changed?(:credentials)
   end

lib/gitlab/git/remote_mirror.rb

@@ -5,11 +5,10 @@ module Gitlab
     class RemoteMirror
       include Gitlab::Git::WrapsGitalyErrors
 
-      attr_reader :repository, :ref_name, :remote_url, :only_branches_matching, :ssh_key, :known_hosts, :keep_divergent_refs
+      attr_reader :repository, :remote_url, :only_branches_matching, :ssh_key, :known_hosts, :keep_divergent_refs
 
-      def initialize(repository, ref_name, remote_url, only_branches_matching: [], ssh_key: nil, known_hosts: nil, keep_divergent_refs: false)
+      def initialize(repository, remote_url, only_branches_matching: [], ssh_key: nil, known_hosts: nil, keep_divergent_refs: false)
         @repository = repository
-        @ref_name = ref_name
         @remote_url = remote_url
         @only_branches_matching = only_branches_matching
         @ssh_key = ssh_key
@@ -20,7 +19,6 @@ module Gitlab
       def update
         wrapped_gitaly_errors do
           repository.gitaly_remote_client.update_remote_mirror(
-            ref_name,
             remote_url,
             only_branches_matching,
             ssh_key: ssh_key,

lib/gitlab/gitaly_client/remote_service.rb

@@ -37,18 +37,13 @@ module Gitlab
         encode_utf8(response.ref)
       end
 
-      def update_remote_mirror(ref_name, remote_url, only_branches_matching, ssh_key: nil, known_hosts: nil, keep_divergent_refs: false)
+      def update_remote_mirror(remote_url, only_branches_matching, ssh_key: nil, known_hosts: nil, keep_divergent_refs: false)
         req_enum = Enumerator.new do |y|
           first_request = Gitaly::UpdateRemoteMirrorRequest.new(
             repository: @gitaly_repo
           )
 
-          if remote_url
-            first_request.remote = Gitaly::UpdateRemoteMirrorRequest::Remote.new(url: remote_url)
-          else
-            first_request.ref_name = ref_name
-          end
-
+          first_request.remote = Gitaly::UpdateRemoteMirrorRequest::Remote.new(url: remote_url)
           first_request.ssh_key = ssh_key if ssh_key.present?
           first_request.known_hosts = known_hosts if known_hosts.present?
           first_request.keep_divergent_refs = keep_divergent_refs

spec/factories/projects.rb

@@ -265,7 +265,6 @@ FactoryBot.define do
 
     trait :remote_mirror do
       transient do
-        remote_name { "remote_mirror_#{SecureRandom.hex}" }
         url { "http://foo.com" }
         enabled { true }
       end

spec/lib/gitlab/git/remote_mirror_spec.rb

@@ -6,30 +6,17 @@ RSpec.describe Gitlab::Git::RemoteMirror do
   describe '#update' do
     let(:project) { create(:project, :repository) }
     let(:repository) { project.repository }
-    let(:ref_name) { 'foo' }
     let(:url) { 'https://example.com' }
     let(:options) { { only_branches_matching: ['master'], ssh_key: 'KEY', known_hosts: 'KNOWN HOSTS', keep_divergent_refs: true } }
 
-    subject(:remote_mirror) { described_class.new(repository, ref_name, url, **options) }
+    subject(:remote_mirror) { described_class.new(repository, url, **options) }
 
-    shared_examples 'an update' do
-      it 'delegates to the Gitaly client' do
-        expect(repository.gitaly_remote_client)
-          .to receive(:update_remote_mirror)
-          .with(ref_name, url, ['master'], ssh_key: 'KEY', known_hosts: 'KNOWN HOSTS', keep_divergent_refs: true)
-
-        remote_mirror.update # rubocop:disable Rails/SaveBang
-      end
-    end
-
-    context 'with url' do
-      it_behaves_like 'an update'
-    end
-
-    context 'without url' do
-      let(:url) { nil }
+    it 'delegates to the Gitaly client' do
+      expect(repository.gitaly_remote_client)
+        .to receive(:update_remote_mirror)
+        .with(url, ['master'], ssh_key: 'KEY', known_hosts: 'KNOWN HOSTS', keep_divergent_refs: true)
 
-      it_behaves_like 'an update'
+      remote_mirror.update # rubocop:disable Rails/SaveBang
     end
 
     it 'wraps gitaly errors' do

spec/lib/gitlab/gitaly_client/remote_service_spec.rb

@@ -35,34 +35,19 @@ RSpec.describe Gitlab::GitalyClient::RemoteService do
   end
 
   describe '#update_remote_mirror' do
-    let(:ref_name) { 'remote_mirror_1' }
     let(:only_branches_matching) { %w[my-branch master] }
     let(:ssh_key) { 'KEY' }
     let(:known_hosts) { 'KNOWN HOSTS' }
+    let(:url) { 'http:://git.example.com/my-repo.git' }
+    let(:expected_params) { { remote: Gitaly::UpdateRemoteMirrorRequest::Remote.new(url: url) } }
 
-    shared_examples 'an update' do
-      it 'sends an update_remote_mirror message' do
-        expect_any_instance_of(Gitaly::RemoteService::Stub)
-          .to receive(:update_remote_mirror)
-          .with(array_including(gitaly_request_with_params(expected_params)), kind_of(Hash))
-          .and_return(double(:update_remote_mirror_response))
-
-        client.update_remote_mirror(ref_name, url, only_branches_matching, ssh_key: ssh_key, known_hosts: known_hosts, keep_divergent_refs: true)
-      end
-    end
-
-    context 'with remote name' do
-      let(:url) { nil }
-      let(:expected_params) { { ref_name: ref_name } }
-
-      it_behaves_like 'an update'
-    end
-
-    context 'with remote URL' do
-      let(:url) { 'http:://git.example.com/my-repo.git' }
-      let(:expected_params) { { remote: Gitaly::UpdateRemoteMirrorRequest::Remote.new(url: url) } }
+    it 'sends an update_remote_mirror message' do
+      expect_any_instance_of(Gitaly::RemoteService::Stub)
+        .to receive(:update_remote_mirror)
+        .with(array_including(gitaly_request_with_params(expected_params)), kind_of(Hash))
+        .and_return(double(:update_remote_mirror_response))
 
-      it_behaves_like 'an update'
+      client.update_remote_mirror(url, only_branches_matching, ssh_key: ssh_key, known_hosts: known_hosts, keep_divergent_refs: true)
     end
   end
 

spec/models/remote_mirror_spec.rb

@@ -105,35 +105,6 @@ RSpec.describe RemoteMirror, :mailer do
     end
   end
 
-  describe '#remote_name' do
-    context 'when remote name is persisted in the database' do
-      it 'returns remote name with random value' do
-        allow(SecureRandom).to receive(:hex).and_return('secret')
-
-        remote_mirror = create(:remote_mirror)
-
-        expect(remote_mirror.remote_name).to eq('remote_mirror_secret')
-      end
-    end
-
-    context 'when remote name is not persisted in the database' do
-      it 'returns remote name with remote mirror id' do
-        remote_mirror = create(:remote_mirror)
-        remote_mirror.remote_name = nil
-
-        expect(remote_mirror.remote_name).to eq("remote_mirror_#{remote_mirror.id}")
-      end
-    end
-
-    context 'when remote is not persisted in the database' do
-      it 'returns nil' do
-        remote_mirror = build(:remote_mirror, remote_name: nil)
-
-        expect(remote_mirror.remote_name).to be_nil
-      end
-    end
-  end
-
   describe '#bare_url' do
     it 'returns the URL without any credentials' do
       remote_mirror = build(:remote_mirror, url: 'http://user:pass@example.com/foo')
@@ -158,7 +129,6 @@ RSpec.describe RemoteMirror, :mailer do
 
       expect(git_remote_mirror).to have_received(:new).with(
         mirror.project.repository.raw,
-        mirror.remote_name,
         mirror.url,
         keep_divergent_refs: true
       )

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -7,8 +7,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
   let_it_be(:remote_project) { create(:forked_project_with_submodules) }
   let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
 
-  let(:remote_name) { remote_mirror.remote_name }
-
   subject(:service) { described_class.new(project, project.creator) }
 
   describe '#execute' do