Merge branch 'gitaly-redis-refactor' into 'master'

Support new /internal/post-receive API endpoint

Closes gitaly#522

See merge request !163

CHANGELOG

+v5.9.0
+  - Support new /internal/post-receive API endpoint for post-receive operations
+
 v5.8.1
   - Support old versions of ruby without monotonic clock
 

lib/gitlab_net.rb

@@ -11,6 +11,7 @@ require_relative 'httpunix'
 
 class GitlabNet
   class ApiUnreachableError < StandardError; end
+  class NotFound < StandardError; end
 
   CHECK_TIMEOUT = 5
   READ_TIMEOUT = 300
@@ -112,6 +113,19 @@ class GitlabNet
     false
   end
 
+  def post_receive(gl_repository, identifier, changes)
+    params = {
+      gl_repository: gl_repository,
+      identifier: identifier,
+      changes: changes
+    }
+    resp = post("#{host}/post_receive", params)
+
+    raise NotFound if resp.code == '404'
+
+    JSON.parse(resp.body) if resp.code == '200'
+  end
+
   def redis_client
     redis_config = config.redis
     database = redis_config['database'] || 0

lib/gitlab_post_receive.rb

@@ -20,29 +20,20 @@ class GitlabPostReceive
   end
 
   def exec
-    result = update_redis
+    response = GitlabMetrics.measure("post-receive") do
+      api.post_receive(gl_repository, @actor, changes)
+    end
 
-    begin
-      broadcast_message = GitlabMetrics.measure("broadcast-message") do
-        api.broadcast_message
-      end
+    return false unless response
 
-      if broadcast_message.has_key?("message")
-        puts
-        print_broadcast_message(broadcast_message["message"])
-      end
+    print_broadcast_message(response['broadcast_message']) if response['broadcast_message']
+    print_merge_request_links(response['merge_request_urls']) if response['merge_request_urls']
 
-      merge_request_urls = GitlabMetrics.measure("merge-request-urls") do
-        api.merge_request_urls(@gl_repository, @repo_path, @changes)
-      end
-      print_merge_request_links(merge_request_urls)
-
-      api.notify_post_receive(gl_repository, repo_path)
-    rescue GitlabNet::ApiUnreachableError
-      nil
-    end
-
-    result && GitlabReferenceCounter.new(repo_path).decrease
+    response['reference_counter_decreased']
+  rescue GitlabNet::ApiUnreachableError
+    false
+  rescue GitlabNet::NotFound
+    fallback_post_receive
   end
 
   protected
@@ -89,6 +80,7 @@ class GitlabPostReceive
     # message.scan returns a nested array of capture groups, so flatten.
     lines = message.scan(/(.{,#{text_width}})(?:\s|$)/)[0...-1].flatten
 
+    puts
     puts "=" * total_width
     puts
 
@@ -127,4 +119,31 @@ class GitlabPostReceive
       false
     end
   end
+
+  private
+
+  def fallback_post_receive
+    result = update_redis
+
+    begin
+      broadcast_message = GitlabMetrics.measure("broadcast-message") do
+        api.broadcast_message
+      end
+
+      if broadcast_message.has_key?("message")
+        print_broadcast_message(broadcast_message["message"])
+      end
+
+      merge_request_urls = GitlabMetrics.measure("merge-request-urls") do
+        api.merge_request_urls(@gl_repository, @repo_path, @changes)
+      end
+      print_merge_request_links(merge_request_urls)
+
+      api.notify_post_receive(gl_repository, repo_path)
+    rescue GitlabNet::ApiUnreachableError
+      nil
+    end
+
+    result && GitlabReferenceCounter.new(repo_path).decrease
+  end
 end

spec/gitlab_net_spec.rb

@@ -126,6 +126,46 @@ describe GitlabNet, vcr: true do
     end
   end
 
+  describe :post_receive do
+    let(:gl_repository) { "project-1" }
+    let(:changes) { "123456 789012 refs/heads/test\n654321 210987 refs/tags/tag" }
+    let(:params) do
+      { gl_repository: gl_repository, identifier: key, changes: changes }
+    end
+    let(:merge_request_urls) do
+      [{
+        "branch_name" => "test",
+        "url" => "http://localhost:3000/gitlab-org/gitlab-test/merge_requests/7",
+        "new_merge_request" => false
+      }]
+    end
+
+    subject { gitlab_net.post_receive(gl_repository, key, changes) }
+
+    it 'sends the correct parameters' do
+      Net::HTTP::Post.any_instance.should_receive(:set_form_data).with(hash_including(params))
+
+
+      VCR.use_cassette("post-receive") do
+        subject
+      end
+    end
+
+    it 'calls /internal/post-receive' do
+      VCR.use_cassette("post-receive") do
+        expect(subject['merge_request_urls']).to eq(merge_request_urls)
+        expect(subject['broadcast_message']).to eq('Message')
+        expect(subject['reference_counter_decreased']).to eq(true)
+      end
+    end
+
+    it 'throws a NotFound error when post-receive is not available' do
+      VCR.use_cassette("post-receive-not-found") do
+        expect { subject }.to raise_error(GitlabNet::NotFound)
+      end
+    end
+  end
+
   describe :authorized_key do
     let (:ssh_key) { "rsa-key" }
 

spec/gitlab_post_receive_spec.rb

@@ -4,173 +4,164 @@ require 'gitlab_post_receive'
 
 describe GitlabPostReceive do
   let(:repository_path) { "/home/git/repositories" }
-  let(:repo_name)   { 'dzaporozhets/gitlab-ci' }
-  let(:actor)   { 'key-123' }
+  let(:repo_name) { 'dzaporozhets/gitlab-ci' }
+  let(:actor) { 'key-123' }
   let(:changes) { "123456 789012 refs/heads/tést\n654321 210987 refs/tags/tag" }
   let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
   let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
-  let(:repo_path)  { File.join(repository_path, repo_name) + ".git" }
+  let(:repo_path) { File.join(repository_path, repo_name) + ".git" }
   let(:gl_repository) { "project-1" }
   let(:gitlab_post_receive) { GitlabPostReceive.new(gl_repository, repo_path, actor, wrongly_encoded_changes) }
-  let(:message) { "test " * 10 + "message " * 10 }
+  let(:broadcast_message) { "test " * 10 + "message " * 10 }
   let(:redis_client) { double('redis_client') }
   let(:enqueued_at) { Time.new(2016, 6, 23, 6, 59) }
+  let(:new_merge_request_urls) do
+    [{
+      'branch_name' => 'new_branch',
+      'url' => 'http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch',
+      'new_merge_request' => true
+    }]
+  end
+  let(:existing_merge_request_urls) do
+    [{
+      'branch_name' => 'feature_branch',
+      'url' => 'http://localhost/dzaporozhets/gitlab-ci/merge_requests/1',
+      'new_merge_request' => false
+    }]
+  end
 
   before do
+    $logger = double('logger').as_null_object # Global vars are bad
     GitlabConfig.any_instance.stub(repos_path: repository_path)
-    GitlabNet.any_instance.stub(broadcast_message: { })
-    GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) { [] }
-    GitlabNet.any_instance.stub(notify_post_receive: true)
-    expect(Time).to receive(:now).and_return(enqueued_at)
   end
 
   describe "#exec" do
-    before do
-      allow_any_instance_of(GitlabNet).to receive(:redis_client).and_return(redis_client)
-      allow_any_instance_of(GitlabReferenceCounter).to receive(:redis_client).and_return(redis_client)
-      allow(redis_client).to receive(:get).and_return(1)
-      allow(redis_client).to receive(:incr).and_return(true)
-      allow(redis_client).to receive(:decr).and_return(0)
-      allow(redis_client).to receive(:rpush).and_return(true)
-    end
+    context 'when the new post_receive API endpoint is not available' do
+      before do
+        GitlabNet.any_instance.stub(broadcast_message: { })
+        GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) { [] }
+        GitlabNet.any_instance.stub(notify_post_receive: true)
+
+        allow_any_instance_of(GitlabNet).to receive(:post_receive).and_raise(GitlabNet::NotFound)
+        allow_any_instance_of(GitlabNet).to receive(:redis_client).and_return(redis_client)
+        allow_any_instance_of(GitlabReferenceCounter).to receive(:redis_client).and_return(redis_client)
+        allow(redis_client).to receive(:get).and_return(1)
+        allow(redis_client).to receive(:incr).and_return(true)
+        allow(redis_client).to receive(:decr).and_return(0)
+        allow(redis_client).to receive(:rpush).and_return(true)
+        expect(Time).to receive(:now).and_return(enqueued_at)
+      end
 
-    context 'Without broad cast message' do
-      context 'pushing new branch' do
-        before do
-          GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) do
-            [{
-              "branch_name" => "new_branch",
-              "url" => "http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
-              "new_merge_request" => true
-            }]
+      context 'Without broad cast message' do
+        context 'pushing new branch' do
+          before do
+            GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) do
+              new_merge_request_urls
+            end
+          end
+
+          it "prints the new merge request url" do
+            assert_new_mr_printed(gitlab_post_receive)
+
+            gitlab_post_receive.exec
           end
         end
 
-        it "prints the new merge request url" do
-          expect(redis_client).to receive(:rpush)
+        context 'pushing existing branch with merge request created' do
+          before do
+            GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) do
+              existing_merge_request_urls
+            end
+          end
 
-          expect(gitlab_post_receive).to receive(:puts).ordered
-          expect(gitlab_post_receive).to receive(:puts).with(
-            "To create a merge request for new_branch, visit:"
-          ).ordered
-          expect(gitlab_post_receive).to receive(:puts).with(
-            "  http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch"
-          ).ordered
-          expect(gitlab_post_receive).to receive(:puts).ordered
+          it "prints the view merge request url" do
+            assert_existing_mr_printed(gitlab_post_receive)
 
-          gitlab_post_receive.exec
+            gitlab_post_receive.exec
+          end
         end
       end
 
-      context 'pushing existing branch with merge request created' do
+      context 'show broadcast message and merge request link' do
         before do
           GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) do
-            [{
-              "branch_name" => "feature_branch",
-              "url" => "http://localhost/dzaporozhets/gitlab-ci/merge_requests/1",
-              "new_merge_request" => false
-            }]
+            new_merge_request_urls
           end
+          GitlabNet.any_instance.stub(broadcast_message: { "message" => broadcast_message })
         end
 
-        it "prints the view merge request url" do
-          expect(redis_client).to receive(:rpush)
-
-          expect(gitlab_post_receive).to receive(:puts).ordered
-          expect(gitlab_post_receive).to receive(:puts).with(
-            "View merge request for feature_branch:"
-          ).ordered
-          expect(gitlab_post_receive).to receive(:puts).with(
-            "  http://localhost/dzaporozhets/gitlab-ci/merge_requests/1"
-          ).ordered
-          expect(gitlab_post_receive).to receive(:puts).ordered
+        it 'prints the broadcast message and create new merge request link' do
+          assert_broadcast_message_printed(gitlab_post_receive)
+          assert_new_mr_printed(gitlab_post_receive)
 
           gitlab_post_receive.exec
         end
       end
-    end
 
-    context 'show broadcast message and merge request link' do
-      before do
-        GitlabNet.any_instance.stub(:merge_request_urls).with(gl_repository, repo_path, wrongly_encoded_changes) do
-          [{
-            "branch_name" => "new_branch",
-            "url" => "http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
-            "new_merge_request" => true
-          }]
-        end
-        GitlabNet.any_instance.stub(broadcast_message: { "message" => message })
-      end
+      context 'Sidekiq jobs' do
+        it "pushes a Sidekiq job onto the queue" do
+          expect(redis_client).to receive(:rpush).with(
+            'resque:gitlab:queue:post_receive',
+            %Q/{"class":"PostReceive","args":["#{gl_repository}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}","enqueued_at":#{enqueued_at.to_f}}/
+          ).and_return(true)
 
-      it 'prints the broadcast message and create new merge request link' do
-        expect(redis_client).to receive(:rpush)
-
-        expect(gitlab_post_receive).to receive(:puts).ordered
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "========================================================================"
-        ).ordered
-        expect(gitlab_post_receive).to receive(:puts).ordered
-
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "   test test test test test test test test test test message message"
-        ).ordered
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "    message message message message message message message message"
-        ).ordered
-
-        expect(gitlab_post_receive).to receive(:puts).ordered
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "========================================================================"
-        ).ordered
-
-        expect(gitlab_post_receive).to receive(:puts).ordered
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "To create a merge request for new_branch, visit:"
-        ).ordered
-        expect(gitlab_post_receive).to receive(:puts).with(
-          "  http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch"
-        ).ordered
-        expect(gitlab_post_receive).to receive(:puts).ordered
-
-
-        gitlab_post_receive.exec
-      end
-    end
+          gitlab_post_receive.exec
+        end
 
-    context 'Sidekiq jobs' do
-      it "pushes a Sidekiq job onto the queue" do
-        expect(redis_client).to receive(:rpush).with(
-          'resque:gitlab:queue:post_receive',
-           %Q/{"class":"PostReceive","args":["#{gl_repository}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}","enqueued_at":#{enqueued_at.to_f}}/
-        ).and_return(true)
+        context 'when gl_repository is nil' do
+          let(:gl_repository) { nil }
 
-        gitlab_post_receive.exec
-      end
+          it "pushes a Sidekiq job with the repository path" do
+            expect(redis_client).to receive(:rpush).with(
+              'resque:gitlab:queue:post_receive',
+              %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}","enqueued_at":#{enqueued_at.to_f}}/
+            ).and_return(true)
 
-      context 'when gl_repository is nil' do
-        let(:gl_repository) { nil }
+            gitlab_post_receive.exec
+          end
+        end
+      end
 
-        it "pushes a Sidekiq job with the repository path" do
-          expect(redis_client).to receive(:rpush).with(
-            'resque:gitlab:queue:post_receive',
-             %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}","enqueued_at":#{enqueued_at.to_f}}/
-          ).and_return(true)
+      context 'reference counter' do
+        it 'decreases the reference counter for the project' do
+          expect_any_instance_of(GitlabReferenceCounter).to receive(:decrease).and_return(true)
 
           gitlab_post_receive.exec
         end
+
+        context "when the redis command succeeds" do
+          before do
+            allow(redis_client).to receive(:decr).and_return(0)
+          end
+
+          it "returns true" do
+            expect(gitlab_post_receive.exec).to eq(true)
+          end
+        end
+
+        context "when the redis command fails" do
+          before do
+            allow(redis_client).to receive(:decr).and_raise('Fail')
+          end
+
+          it "returns false" do
+            expect(gitlab_post_receive.exec).to eq(false)
+          end
+        end
       end
-    end
 
-    context 'reference counter' do
-      it 'decreases the reference counter for the project' do
-        expect_any_instance_of(GitlabReferenceCounter).to receive(:decrease).and_return(true)
+      context 'post_receive notification' do
+        it 'calls the api to notify the execution of the hook' do
+          expect_any_instance_of(GitlabNet).to receive(:notify_post_receive).
+            with(gl_repository, repo_path)
 
-        gitlab_post_receive.exec
+          gitlab_post_receive.exec
+        end
       end
 
       context "when the redis command succeeds" do
         before do
-          allow(redis_client).to receive(:decr).and_return(0)
+          allow(redis_client).to receive(:rpush).and_return(true)
         end
 
         it "returns true" do
@@ -180,7 +171,7 @@ describe GitlabPostReceive do
 
       context "when the redis command fails" do
         before do
-          allow(redis_client).to receive(:decr).and_raise('Fail')
+          allow(redis_client).to receive(:rpush).and_raise('Fail')
         end
 
         it "returns false" do
@@ -189,35 +180,76 @@ describe GitlabPostReceive do
       end
     end
 
-    context 'post_receive notification' do
+    context 'when the new post_receive API endpoint is available' do
+      let(:response) { { 'reference_counter_decreased' => true } }
+
       it 'calls the api to notify the execution of the hook' do
-        expect_any_instance_of(GitlabNet).to receive(:notify_post_receive).
-          with(gl_repository, repo_path)
+        expect_any_instance_of(GitlabNet).to receive(:post_receive).and_return(response)
 
-        gitlab_post_receive.exec
+        expect(gitlab_post_receive.exec).to eq(true)
       end
-    end
 
-    context "when the redis command succeeds" do
+      context 'merge request urls and broadcast messages' do
+        let(:response) do
+          {
+            'reference_counter_decreased' => true,
+            'merge_request_urls' => new_merge_request_urls,
+            'broadcast_message' => broadcast_message
+          }
+        end
 
-      before do
-        allow(redis_client).to receive(:rpush).and_return(true)
-      end
+        it 'prints the merge request urls and broadcast message' do
+          expect_any_instance_of(GitlabNet).to receive(:post_receive).and_return(response)
+          assert_broadcast_message_printed(gitlab_post_receive)
+          assert_new_mr_printed(gitlab_post_receive)
 
-      it "returns true" do
-        expect(gitlab_post_receive.exec).to eq(true)
+          expect(gitlab_post_receive.exec).to eq(true)
+        end
       end
     end
+  end
 
-    context "when the redis command fails" do
+  private
+
+  def assert_new_mr_printed(gitlab_post_receive)
+    expect(gitlab_post_receive).to receive(:puts).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "To create a merge request for new_branch, visit:"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "  http://localhost/dzaporozhets/gitlab-ci/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).ordered
+  end
 
-      before do
-        allow(redis_client).to receive(:rpush).and_raise('Fail')
-      end
+  def assert_existing_mr_printed(gitlab_post_receive)
+    expect(gitlab_post_receive).to receive(:puts).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "View merge request for feature_branch:"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "  http://localhost/dzaporozhets/gitlab-ci/merge_requests/1"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).ordered
+  end
 
-      it "returns false" do
-        expect(gitlab_post_receive.exec).to eq(false)
-      end
-    end
+  def assert_broadcast_message_printed(gitlab_post_receive)
+    expect(gitlab_post_receive).to receive(:puts).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "========================================================================"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).ordered
+
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "   test test test test test test test test test test message message"
+    ).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "    message message message message message message message message"
+    ).ordered
+
+    expect(gitlab_post_receive).to receive(:puts).ordered
+    expect(gitlab_post_receive).to receive(:puts).with(
+      "========================================================================"
+    ).ordered
   end
 end

spec/vcr_cassettes/post-receive-not-found.yml

+---
+http_interactions:
+- request:
+    method: post
+    uri: http://localhost:3000/api/v4/internal/post_receive
+    body:
+      encoding: US-ASCII
+      string: gl_repository=project-1&identifier=key-1&changes=123456+789012+refs%2Fheads%2Ftest%0A654321+210987+refs%2Ftags%2Ftag&secret_token=0a3938d9d95d807e94d937af3a4fbbea%0A
+    headers:
+      Accept-Encoding:
+      - gzip;q=1.0,deflate;q=0.6,identity;q=0.3
+      Accept:
+      - "*/*"
+      User-Agent:
+      - Ruby
+      Content-Type:
+      - application/x-www-form-urlencoded
+  response:
+    status:
+      code: 404
+      message: Not Found
+    headers:
+      Cache-Control:
+      - no-cache
+      Content-Length:
+      - '25'
+      Content-Type:
+      - application/json
+      Date:
+      - Wed, 30 Aug 2017 22:24:37 GMT
+      Vary:
+      - Origin
+      X-Request-Id:
+      - bbfdb1ed-99dc-4246-a606-3074ffd5d87b
+      X-Runtime:
+      - '0.459681'
+    body:
+      encoding: UTF-8
+      string: '{"error":"404 Not Found"}'
+    http_version:
+  recorded_at: Wed, 30 Aug 2017 22:24:37 GMT
+recorded_with: VCR 2.4.0

spec/vcr_cassettes/post-receive.yml

+---
+http_interactions:
+- request:
+    method: post
+    uri: http://localhost:3000/api/v4/internal/post_receive
+    body:
+      encoding: US-ASCII
+      string: gl_repository=project-1&identifier=key-1&changes=123456+789012+refs%2Fheads%2Ftest%0A654321+210987+refs%2Ftags%2Ftag&secret_token=0a3938d9d95d807e94d937af3a4fbbea%0A
+    headers:
+      Accept-Encoding:
+      - gzip;q=1.0,deflate;q=0.6,identity;q=0.3
+      Accept:
+      - "*/*"
+      User-Agent:
+      - Ruby
+      Content-Type:
+      - application/x-www-form-urlencoded
+  response:
+    status:
+      code: 200
+      message: OK
+    headers:
+      Cache-Control:
+      - max-age=0, private, must-revalidate
+      Content-Length:
+      - '208'
+      Content-Type:
+      - application/json
+      Date:
+      - Wed, 30 Aug 2017 22:08:28 GMT
+      Etag:
+      - W/"1757d1411091b751684cde10119e0942"
+      Vary:
+      - Origin
+      X-Frame-Options:
+      - SAMEORIGIN
+      X-Request-Id:
+      - f7d422a7-c1a3-49d1-9b81-7fc48c954767
+      X-Runtime:
+      - '0.687283'
+    body:
+      encoding: UTF-8
+      string: '{"merge_request_urls":[{"branch_name":"test","url":"http://localhost:3000/gitlab-org/gitlab-test/merge_requests/7","new_merge_request":false}],"broadcast_message":"Message","reference_counter_decreased":true}'
+    http_version:
+  recorded_at: Wed, 30 Aug 2017 22:08:28 GMT
+recorded_with: VCR 2.4.0