Merge branch 'feature/migrate-repository-add-tag-to-gitaly' into 'master'

Migrate Gitlab::Git::Repository#add_tag to Gitaly

Closes gitaly#601

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

app/services/tags/create_service.rb

@@ -11,7 +11,7 @@ module Tags
 
       begin
         new_tag = repository.add_tag(current_user, tag_name, target, message)
-      rescue Rugged::TagError
+      rescue Gitlab::Git::Repository::TagExistsError
         return error("Tag #{tag_name} already exists")
       rescue Gitlab::Git::HooksService::PreReceiveError => ex
         return error(ex.message)

lib/gitlab/git/repository.rb

@@ -20,6 +20,7 @@ module Gitlab
       GitError = Class.new(StandardError)
       DeleteBranchError = Class.new(StandardError)
       CreateTreeError = Class.new(StandardError)
+      TagExistsError = Class.new(StandardError)
 
       class << self
         # Unlike `new`, `create` takes the storage path, not the storage name
@@ -646,24 +647,13 @@ module Gitlab
       end
 
       def add_tag(tag_name, user:, target:, message: nil)
-        target_object = Ref.dereference_object(lookup(target))
-        raise InvalidRef.new("target not found: #{target}") unless target_object
-
-        user = Gitlab::Git::User.from_gitlab(user) unless user.respond_to?(:gl_id)
-
-        options = nil # Use nil, not the empty hash. Rugged cares about this.
-        if message
-          options = {
-            message: message,
-            tagger: Gitlab::Git.committer_hash(email: user.email, name: user.name)
-          }
+        gitaly_migrate(:operation_user_add_tag) do |is_enabled|
+          if is_enabled
+            gitaly_add_tag(tag_name, user: user, target: target, message: message)
+          else
+            rugged_add_tag(tag_name, user: user, target: target, message: message)
+          end
         end
-
-        OperationService.new(user, self).add_tag(tag_name, target_object.oid, options)
-
-        find_tag(tag_name)
-      rescue Rugged::ReferenceError => ex
-        raise InvalidRef, ex
       end
 
       def rm_branch(branch_name, user:)
@@ -1392,6 +1382,33 @@ module Gitlab
         false
       end
 
+      def gitaly_add_tag(tag_name, user:, target:, message: nil)
+        gitaly_operations_client.add_tag(tag_name, user, target, message)
+      end
+
+      def rugged_add_tag(tag_name, user:, target:, message: nil)
+        target_object = Ref.dereference_object(lookup(target))
+        raise InvalidRef.new("target not found: #{target}") unless target_object
+
+        user = Gitlab::Git::User.from_gitlab(user) unless user.respond_to?(:gl_id)
+
+        options = nil # Use nil, not the empty hash. Rugged cares about this.
+        if message
+          options = {
+            message: message,
+            tagger: Gitlab::Git.committer_hash(email: user.email, name: user.name)
+          }
+        end
+
+        Gitlab::Git::OperationService.new(user, self).add_tag(tag_name, target_object.oid, options)
+
+        find_tag(tag_name)
+      rescue Rugged::ReferenceError => ex
+        raise InvalidRef, ex
+      rescue Rugged::TagError
+        raise TagExistsError
+      end
+
       def rugged_create_branch(ref, start_point)
         rugged_ref = rugged.branches.create(ref, start_point)
         target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)

lib/gitlab/gitaly_client/operation_service.rb

@@ -19,6 +19,27 @@ module Gitlab
           raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
         end
       end
+
+      def add_tag(tag_name, user, target, message)
+        request = Gitaly::UserCreateTagRequest.new(
+          repository: @gitaly_repo,
+          user: Util.gitaly_user(user),
+          tag_name: GitalyClient.encode(tag_name),
+          target_revision: GitalyClient.encode(target),
+          message: GitalyClient.encode(message.to_s)
+        )
+
+        response = GitalyClient.call(@repository.storage, :operation_service, :user_create_tag, request)
+        if pre_receive_error = response.pre_receive_error.presence
+          raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+        elsif response.exists
+          raise Gitlab::Git::Repository::TagExistsError
+        end
+
+        Util.gitlab_tag_from_gitaly_tag(@repository, response.tag)
+      rescue GRPC::FailedPrecondition => e
+        raise Gitlab::Git::Repository::InvalidRef, e
+      end
     end
   end
 end

lib/gitlab/gitaly_client/ref_service.rb

@@ -155,19 +155,7 @@ module Gitlab
 
       def consume_tags_response(response)
         response.flat_map do |message|
-          message.tags.map do |gitaly_tag|
-            if gitaly_tag.target_commit.present?
-              gitaly_commit = Gitlab::Git::Commit.decorate(@repository, gitaly_tag.target_commit)
-            end
-
-            Gitlab::Git::Tag.new(
-              @repository,
-              encode!(gitaly_tag.name.dup),
-              gitaly_tag.id,
-              gitaly_commit,
-              encode!(gitaly_tag.message.chomp)
-            )
-          end
+          message.tags.map { |gitaly_tag| Util.gitlab_tag_from_gitaly_tag(@repository, gitaly_tag) }
         end
       end
 

lib/gitlab/gitaly_client/util.rb

@@ -20,6 +20,20 @@ module Gitlab
             email: GitalyClient.encode(gitlab_user.email)
           )
         end
+
+        def gitlab_tag_from_gitaly_tag(repository, gitaly_tag)
+          if gitaly_tag.target_commit.present?
+            commit = Gitlab::Git::Commit.decorate(repository, gitaly_tag.target_commit)
+          end
+
+          Gitlab::Git::Tag.new(
+            repository,
+            Gitlab::EncodingHelper.encode!(gitaly_tag.name.dup),
+            gitaly_tag.id,
+            commit,
+            Gitlab::EncodingHelper.encode!(gitaly_tag.message.chomp)
+          )
+        end
       end
     end
   end

spec/models/repository_spec.rb

@@ -1617,27 +1617,41 @@ describe Repository do
   end
 
   describe '#add_tag' do
-    context 'with a valid target' do
-      let(:user) { build_stubbed(:user) }
+    let(:user) { build_stubbed(:user) }
 
-      it 'creates the tag using rugged' do
-        expect(repository.rugged.tags).to receive(:create)
-          .with('8.5', repository.commit('master').id,
-            hash_including(message: 'foo',
-                           tagger: hash_including(name: user.name, email: user.email)))
-          .and_call_original
+    shared_examples 'adding tag' do
+      context 'with a valid target' do
+        it 'creates the tag' do
+          repository.add_tag(user, '8.5', 'master', 'foo')
 
-        repository.add_tag(user, '8.5', 'master', 'foo')
-      end
+          tag = repository.find_tag('8.5')
+          expect(tag).to be_present
+          expect(tag.message).to eq('foo')
+          expect(tag.dereferenced_target.id).to eq(repository.commit('master').id)
+        end
 
-      it 'returns a Gitlab::Git::Tag object' do
-        tag = repository.add_tag(user, '8.5', 'master', 'foo')
+        it 'returns a Gitlab::Git::Tag object' do
+          tag = repository.add_tag(user, '8.5', 'master', 'foo')
+
+          expect(tag).to be_a(Gitlab::Git::Tag)
+        end
+      end
 
-        expect(tag).to be_a(Gitlab::Git::Tag)
+      context 'with an invalid target' do
+        it 'returns false' do
+          expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false
+        end
       end
+    end
 
-      it 'passes commit SHA to pre-receive and update hooks,\
-        and tag SHA to post-receive hook' do
+    context 'when Gitaly operation_user_add_tag feature is enabled' do
+      it_behaves_like 'adding tag'
+    end
+
+    context 'when Gitaly operation_user_add_tag feature is disabled', skip_gitaly_mock: true do
+      it_behaves_like 'adding tag'
+
+      it 'passes commit SHA to pre-receive and update hooks and tag SHA to post-receive hook' do
         pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', project)
         update_hook = Gitlab::Git::Hook.new('update', project)
         post_receive_hook = Gitlab::Git::Hook.new('post-receive', project)
@@ -1662,12 +1676,6 @@ describe Repository do
           .with(anything, anything, tag_sha, anything)
       end
     end
-
-    context 'with an invalid target' do
-      it 'returns false' do
-        expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false
-      end
-    end
   end
 
   describe '#rm_branch' do

spec/services/tags/create_service_spec.rb

@@ -28,7 +28,7 @@ describe Tags::CreateService do
       it 'returns an error' do
         expect(repository).to receive(:add_tag)
           .with(user, 'v1.1.0', 'master', 'Foo')
-          .and_raise(Rugged::TagError)
+          .and_raise(Gitlab::Git::Repository::TagExistsError)
 
         response = service.execute('v1.1.0', 'master', 'Foo')