Implement ExternalMergeRequestPushService

Implement dependent ExternalMergeRequestHooksService
and wire them with the pipeline creation logic
to distinguish whether an external merge request (such as
GitHub pull request) is the cause of a new push to the
mirror repository.

11 files changed, 243 insertions, 23 deletions

diff --git a/app/models/ci/pipeline_enums.rb b/app/models/ci/pipeline_enums.rb
index 571c4271475..5fdb0187492 100644
--- a/app/models/ci/pipeline_enums.rb
+++ b/app/models/ci/pipeline_enums.rb
@@ -23,7 +23,8 @@ module Ci
         api: 5,
         external: 6,
         chat: 8,
-        merge_request_event: 10
+        merge_request_event: 10,
+        external_merge_request: 11
       }
     end
 
diff --git a/app/models/service.rb b/app/models/service.rb
index 752467622f2..1ea32c6e797 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -46,6 +46,7 @@ class Service < ApplicationRecord
   scope :issue_hooks, -> { where(issues_events: true, active: true) }
   scope :confidential_issue_hooks, -> { where(confidential_issues_events: true, active: true) }
   scope :merge_request_hooks, -> { where(merge_requests_events: true, active: true) }
+  scope :external_merge_request_hooks, -> { push_hooks }
   scope :note_hooks, -> { where(note_events: true, active: true) }
   scope :confidential_note_hooks, -> { where(confidential_note_events: true, active: true) }
   scope :job_hooks, -> { where(job_events: true, active: true) }
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index d30df34e54b..1a7801c4242 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -57,7 +57,11 @@ module Git
 
       Ci::CreatePipelineService
         .new(project, current_user, push_data)
-        .execute(:push, pipeline_options)
+        .execute(pipeline_source, pipeline_options)
+    end
+
+    def pipeline_source
+      :push
     end
 
     def execute_project_hooks
diff --git a/app/services/git/external_merge_request_hooks_service.rb b/app/services/git/external_merge_request_hooks_service.rb
index b6f065b9362..6fd1defb9a0 100644
--- a/app/services/git/external_merge_request_hooks_service.rb
+++ b/app/services/git/external_merge_request_hooks_service.rb
@@ -1,27 +1,27 @@
 # frozen_string_literal: true
 
 module Git
-  class ExternalMergeRequestHooksService  < ::Git::BaseHooksService
+  class ExternalMergeRequestHooksService < ::Git::BaseHooksService
     include Gitlab::Utils::StrongMemoize
 
-    def create_pipelines
-      return unless params.fetch(:create_pipelines, true)
+    private
 
-      Ci::CreatePipelineService
-        .new(project, current_user, push_data)
-        .execute(:external_merge_request, pipeline_options)
+    def pipeline_source
+      :external_merge_request
     end
 
-    private
-
     def hook_name
       :external_merge_request_hooks
     end
 
     def commits
-      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
+      # TODO: test this works
+      project.repository.commits(params[:ref], limit: 10)
     end
 
+    # TODO: remove this to use the implementation from the parent class.
+    # we need to first ensure that projet.has_remote_mirror? is not true for
+    # mirror projects
     def update_remote_mirrors
       return unless project.has_remote_mirror?
 
diff --git a/app/services/git/external_merge_request_push_service.rb b/app/services/git/external_merge_request_push_service.rb
index 09e25a3535d..aeec18c6d4e 100644
--- a/app/services/git/external_merge_request_push_service.rb
+++ b/app/services/git/external_merge_request_push_service.rb
@@ -3,9 +3,9 @@
 module Git
   class ExternalMergeRequestPushService < ::BaseService
     def execute
-      return unless Gitlab::Git.tag_ref?(params[:ref])
+      return unless Gitlab::Git.external_merge_request_ref?(params[:ref])
 
-      TagHooksService.new(project, current_user, params).execute
+      ExternalMergeRequestHooksService.new(project, current_user, params).execute
 
       true
     end
diff --git a/lib/gitlab/ci/build/policy/refs.rb b/lib/gitlab/ci/build/policy/refs.rb
index 55aaa36f835..81f96bdbb67 100644
--- a/lib/gitlab/ci/build/policy/refs.rb
+++ b/lib/gitlab/ci/build/policy/refs.rb
@@ -30,7 +30,8 @@ module Gitlab
             matches_tags_keyword?(pattern, pipeline) ||
               matches_branches_keyword?(pattern, pipeline) ||
               matches_pipeline_source?(pattern, pipeline) ||
-              matches_single_ref?(pattern, pipeline)
+              matches_single_ref?(pattern, pipeline) ||
+              matches_external_merge_request_source?(pattern, pipeline)
           end
 
           def matches_tags_keyword?(pattern, pipeline)
@@ -58,6 +59,13 @@ module Gitlab
             end
           end
 
+          # TODO: best would be to support 'only/except: external_merge_requests'
+          # but for now we reuse 'merge_requests'
+          def matches_external_merge_request_source?(pattern, pipeline)
+            sanitized_source_name(pipeline) == 'external_merge_request' &&
+              pattern == 'merge_requests'
+          end
+
           def sanitized_source_name(pipeline)
             # TODO Memoizing this doesn't seem to make sense with
             #   pipelines being passed in to #satsified_by? as a param.
diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb
index afad391e8e0..7fadb9c239e 100644
--- a/lib/gitlab/ci/pipeline/chain/command.rb
+++ b/lib/gitlab/ci/pipeline/chain/command.rb
@@ -45,6 +45,13 @@ module Gitlab
             end
           end
 
+          def external_merge_request_ref_exists?
+            strong_memoize(:external_merge_request_ref_exists) do
+              Gitlab::Git.external_merge_request_ref?(origin_ref) &&
+              project.repository.ref_exists?(origin_ref)
+            end
+          end
+
           def ref
             strong_memoize(:ref) do
               Gitlab::Git.ref_name(origin_ref)
diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
index 8f5445850d7..38beac3758c 100644
--- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb
+++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
@@ -9,7 +9,7 @@ module Gitlab
             include Chain::Helpers
 
             def perform!
-              unless @command.branch_exists? || @command.tag_exists? || @command.merge_request_ref_exists?
+              unless ref_exists?
                 return error('Reference not found')
               end
 
@@ -25,6 +25,13 @@ module Gitlab
             def break?
               @pipeline.errors.any?
             end
+
+            def ref_exists?
+              @command.branch_exists? ||
+                @command.tag_exists? ||
+                @command.merge_request_ref_exists? ||
+                @command.external_merge_request_ref_exists?
+            end
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/build/policy/refs_spec.rb b/spec/lib/gitlab/ci/build/policy/refs_spec.rb
index abc801f1eb5..799d2e45622 100644
--- a/spec/lib/gitlab/ci/build/policy/refs_spec.rb
+++ b/spec/lib/gitlab/ci/build/policy/refs_spec.rb
@@ -91,14 +91,7 @@ describe Gitlab::Ci::Build::Policy::Refs do
             .to be_satisfied_by(pipeline)
         end
 
-        # external_merge_request is not (yet?) a configuration option. We aren't
-        # distinguishing between Gitlab and external MR commits in configuration.
-        it 'is not_satisfied with only: merge_request' do
-          expect(described_class.new(%w[external_merge_requests]))
-            .to be_satisfied_by(pipeline)
-        end
-
-        it 'is not satisfied with only: merge_request_event' do
+        it 'is not satisfied with only: merge_request_events' do
           expect(described_class.new(%w[merge_request_events]))
             .not_to be_satisfied_by(pipeline)
         end
diff --git a/spec/services/git/external_merge_request_hooks_service_spec.rb b/spec/services/git/external_merge_request_hooks_service_spec.rb
new file mode 100644
index 00000000000..9e25e5b6abb
--- /dev/null
+++ b/spec/services/git/external_merge_request_hooks_service_spec.rb
@@ -0,0 +1,150 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Git::ExternalMergeRequestHooksService, :service do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :repository) }
+
+  let(:oldrev) { Gitlab::Git::BLANK_SHA }
+  let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" }
+  let(:ref) { 'refs/pull/123' }
+
+  let(:commits) { project.repository.commits(ref) }
+
+  let(:service) do
+    described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+  end
+
+  it 'update remote mirrors' do
+    expect(service).to receive(:update_remote_mirrors).and_call_original
+
+    service.execute
+  end
+
+  describe 'System hooks' do
+    it 'Executes system hooks' do
+      push_data = service.execute
+
+      expect_next_instance_of(SystemHooksService) do |system_hooks_service|
+        expect(system_hooks_service)
+          .to receive(:execute_hooks)
+          .with(push_data, :tag_push_hooks)
+      end
+
+      service.execute
+    end
+  end
+
+  describe "Webhooks" do
+    it "executes hooks on the project" do
+      expect(project).to receive(:execute_hooks)
+
+      service.execute
+    end
+  end
+
+  describe "Pipelines" do
+    before do
+      stub_ci_pipeline_to_return_yaml_file
+      project.add_developer(user)
+    end
+
+    it "creates a new pipeline" do
+      expect { service.execute }.to change { Ci::Pipeline.count }
+
+      expect(Ci::Pipeline.last).to be_external_merge_request?
+    end
+  end
+
+  describe 'Push data' do
+    shared_examples_for 'tag push data expectations' do
+      subject(:push_data) { service.execute }
+      it 'has expected push data attributes' do
+        is_expected.to match a_hash_including(
+          object_kind: 'tag_push',
+          ref: ref,
+          before: oldrev,
+          after: newrev,
+          message: tag.message,
+          user_id: user.id,
+          user_name: user.name,
+          project_id: project.id
+        )
+      end
+
+      context "with repository data" do
+        subject { push_data[:repository] }
+
+        it 'has expected repository attributes' do
+          is_expected.to match a_hash_including(
+            name: project.name,
+            url: project.url_to_repo,
+            description: project.description,
+            homepage: project.web_url
+          )
+        end
+      end
+
+      context "with commits" do
+        subject { push_data[:commits] }
+
+        it { is_expected.to be_an(Array) }
+
+        it 'has 1 element' do
+          expect(subject.size).to eq(1)
+        end
+
+        context "the commit" do
+          subject { push_data[:commits].first }
+
+          it { is_expected.to include(timestamp: commit.date.xmlschema) }
+
+          it 'has expected commit attributes' do
+            is_expected.to match a_hash_including(
+              id: commit.id,
+              message: commit.safe_message,
+              url: [
+               Gitlab.config.gitlab.url,
+               project.namespace.to_param,
+               project.to_param,
+               'commit',
+               commit.id
+              ].join('/')
+            )
+          end
+
+          context "with an author" do
+            subject { push_data[:commits].first[:author] }
+
+            it 'has expected author attributes' do
+              is_expected.to match a_hash_including(
+                name: commit.author_name,
+                email: commit.author_email
+              )
+            end
+          end
+        end
+      end
+    end
+
+    context 'annotated tag' do
+      include_examples 'tag push data expectations'
+    end
+
+    context 'lightweight tag' do
+      let(:tag_name) { 'light-tag' }
+      let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' }
+
+      before do
+        # Create the lightweight tag
+        rugged_repo(project.repository).tags.create(tag_name, newrev)
+
+        # Clear tag list cache
+        project.repository.expire_tags_cache
+      end
+
+      include_examples 'tag push data expectations'
+    end
+  end
+end
diff --git a/spec/services/git/external_merge_request_push_service_spec.rb b/spec/services/git/external_merge_request_push_service_spec.rb
new file mode 100644
index 00000000000..a94fe986b4d
--- /dev/null
+++ b/spec/services/git/external_merge_request_push_service_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Git::ExternalMergeRequestPushService do
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+  let(:oldrev) { Gitlab::Git::BLANK_SHA }
+  let(:newrev) { '8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b' }
+  let(:ref) { 'ref/pull/123' }
+
+  let(:service) { described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
+
+  describe 'Hooks' do
+    context 'run on an external merge request like GitHub pull request' do
+      it 'delegates to Git::ExternalMergeRequestHooksService' do
+        expect_next_instance_of(::Git::ExternalMergeRequestHooksService) do |hooks_service|
+          expect(hooks_service.project).to eq(service.project)
+          expect(hooks_service.current_user).to eq(service.current_user)
+          expect(hooks_service.params).to eq(service.params)
+
+          expect(hooks_service).to receive(:execute)
+        end
+
+        service.execute
+      end
+    end
+
+    context 'run on a branch' do
+      let(:ref) { 'refs/heads/master' }
+
+      it 'does nothing' do
+        expect(::Git::ExternalMergeRequestHooksService).not_to receive(:new)
+
+        service.execute
+      end
+    end
+
+    context 'run on a tag' do
+      let(:ref) { 'refs/tags/v1.1.0' }
+
+      it 'does nothing' do
+        expect(::Git::ExternalMergeRequestHooksService).not_to receive(:new)
+
+        service.execute
+      end
+    end
+  end
+end