Merge branch 'rs-partial-mirror-update' into 'master'

Handle a partially-updated RemoteMirror

See merge request gitlab-org/gitlab!29082

Gemfile

@@ -455,7 +455,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 12.9.0.pre.rc4'
+gem 'gitaly', '~> 13.0.0.pre.rc1'
 
 gem 'grpc', '~> 1.24.0'
 

Gemfile.lock

@@ -378,7 +378,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     git (1.5.0)
-    gitaly (12.9.0.pre.rc4)
+    gitaly (13.0.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
@@ -1236,7 +1236,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 12.9.0.pre.rc4)
+  gitaly (~> 13.0.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-labkit (= 0.12.0)

app/models/remote_mirror.rb

@@ -106,7 +106,23 @@ class RemoteMirror < ApplicationRecord
     update_status == 'started'
   end
 
-  def update_repository(options)
+  def update_repository
+    Gitlab::Git::RemoteMirror.new(
+      project.repository.raw,
+      remote_name,
+      **options_for_update
+    ).update
+  end
+
+  def options_for_update
+    options = {
+      keep_divergent_refs: keep_divergent_refs?
+    }
+
+    if only_protected_branches?
+      options[:only_branches_matching] = project.protected_branches.pluck(:name)
+    end
+
     if ssh_mirror_url?
       if ssh_key_auth? && ssh_private_key.present?
         options[:ssh_key] = ssh_private_key
@@ -117,13 +133,7 @@ class RemoteMirror < ApplicationRecord
       end
     end
 
-    options[:keep_divergent_refs] = keep_divergent_refs?
-
-    Gitlab::Git::RemoteMirror.new(
-      project.repository.raw,
-      remote_name,
-      **options
-    ).update
+    options
   end
 
   def sync?

app/services/projects/update_remote_mirror_service.rb

@@ -29,15 +29,17 @@ module Projects
       remote_mirror.ensure_remote!
       repository.fetch_remote(remote_mirror.remote_name, ssh_auth: remote_mirror, no_tags: true)
 
-      opts = {}
-      if remote_mirror.only_protected_branches?
-        opts[:only_branches_matching] = project.protected_branches.select(:name).map(&:name)
-      end
+      response = remote_mirror.update_repository
 
-      remote_mirror.update_repository(opts)
+      if response.divergent_refs.any?
+        message = "Some refs have diverged and have not been updated on the remote:"
+        message += "\n\n#{response.divergent_refs.join("\n")}"
 
+        remote_mirror.mark_as_failed!(message)
+      else
         remote_mirror.update_finish!
       end
+    end
 
     def retry_or_fail(mirror, message, tries)
       if tries < MAX_TRIES

spec/factories/remote_mirrors.rb

@@ -4,5 +4,10 @@ FactoryBot.define do
   factory :remote_mirror, class: 'RemoteMirror' do
     association :project, :repository
     url { "http://foo:bar@test.com" }
+
+    trait :ssh do
+      url { 'ssh://git@test.com:foo/bar.git' }
+      auth_method { 'ssh_public_key' }
+    end
   end
 end

spec/models/remote_mirror_spec.rb

@@ -143,22 +143,54 @@ describe RemoteMirror, :mailer do
   end
 
   describe '#update_repository' do
-    let(:git_remote_mirror) { spy }
+    it 'performs update including options' do
+      git_remote_mirror = stub_const('Gitlab::Git::RemoteMirror', spy)
+      mirror = build(:remote_mirror)
 
-    before do
-      stub_const('Gitlab::Git::RemoteMirror', git_remote_mirror)
+      expect(mirror).to receive(:options_for_update).and_return(options: true)
+      mirror.update_repository
+
+      expect(git_remote_mirror).to have_received(:new).with(
+        mirror.project.repository.raw,
+        mirror.remote_name,
+        options: true
+      )
+      expect(git_remote_mirror).to have_received(:update)
+    end
   end
 
-    it 'includes the `keep_divergent_refs` setting' do
+  describe '#options_for_update' do
+    it 'includes the `keep_divergent_refs` option' do
       mirror = build_stubbed(:remote_mirror, keep_divergent_refs: true)
 
-      mirror.update_repository({})
+      options = mirror.options_for_update
 
-      expect(git_remote_mirror).to have_received(:new).with(
-        anything,
-        mirror.remote_name,
-        hash_including(keep_divergent_refs: true)
-      )
+      expect(options).to include(keep_divergent_refs: true)
+    end
+
+    it 'includes the `only_branches_matching` option' do
+      branch = create(:protected_branch)
+      mirror = build_stubbed(:remote_mirror, project: branch.project, only_protected_branches: true)
+
+      options = mirror.options_for_update
+
+      expect(options).to include(only_branches_matching: [branch.name])
+    end
+
+    it 'includes the `ssh_key` option' do
+      mirror = build(:remote_mirror, :ssh, ssh_private_key: 'private-key')
+
+      options = mirror.options_for_update
+
+      expect(options).to include(ssh_key: 'private-key')
+    end
+
+    it 'includes the `known_hosts` option' do
+      mirror = build(:remote_mirror, :ssh, ssh_known_hosts: 'known-hosts')
+
+      options = mirror.options_for_update
+
+      expect(options).to include(known_hosts: 'known-hosts')
     end
   end
 

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe Projects::UpdateRemoteMirrorService do
   let(:project) { create(:project, :repository) }
   let(:remote_project) { create(:forked_project_with_submodules) }
-  let(:remote_mirror) { project.remote_mirrors.create!(url: remote_project.http_url_to_repo, enabled: true, only_protected_branches: false) }
+  let(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
   let(:remote_name) { remote_mirror.remote_name }
 
   subject(:service) { described_class.new(project, project.creator) }
@@ -16,7 +16,9 @@ describe Projects::UpdateRemoteMirrorService do
     before do
       project.repository.add_branch(project.owner, 'existing-branch', 'master')
 
-      allow(remote_mirror).to receive(:update_repository).and_return(true)
+      allow(remote_mirror)
+        .to receive(:update_repository)
+        .and_return(double(divergent_refs: []))
     end
 
     it 'ensures the remote exists' do
@@ -53,7 +55,7 @@ describe Projects::UpdateRemoteMirrorService do
     it 'marks the mirror as failed and raises the error when an unexpected error occurs' do
       allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken')
 
-      expect { execute! }.to raise_error /Badly broken/
+      expect { execute! }.to raise_error(/Badly broken/)
 
       expect(remote_mirror).to be_failed
       expect(remote_mirror.last_error).to include('Badly broken')
@@ -83,32 +85,21 @@ describe Projects::UpdateRemoteMirrorService do
       end
     end
 
-    context 'when syncing all branches' do
-      it 'push all the branches the first time' do
+    context 'when there are divergent refs' do
+      before do
         stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror)
-
-        expect(remote_mirror).to receive(:update_repository).with({})
-
-        execute!
-      end
       end
 
-    context 'when only syncing protected branches' do
-      it 'sync updated protected branches' do
-        stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror)
-        protected_branch = create_protected_branch(project)
-        remote_mirror.only_protected_branches = true
-
-        expect(remote_mirror)
-          .to receive(:update_repository)
-          .with(only_branches_matching: [protected_branch.name])
+      it 'marks the mirror as failed and sets an error message' do
+        response = double(divergent_refs: %w[refs/heads/master refs/heads/develop])
+        expect(remote_mirror).to receive(:update_repository).and_return(response)
 
         execute!
-      end
 
-      def create_protected_branch(project)
-        branch_name = project.repository.branch_names.find { |n| n != 'existing-branch' }
-        create(:protected_branch, project: project, name: branch_name)
+        expect(remote_mirror).to be_failed
+        expect(remote_mirror.last_error).to include("Some refs have diverged")
+        expect(remote_mirror.last_error).to include("refs/heads/master\n")
+        expect(remote_mirror.last_error).to include("refs/heads/develop")
       end
     end
   end