Merge branch '24185-legacy-ci-status-reactive-cache' into 'security'

Use ReactiveCaching to update external CI status asynchronously

See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2055

9 files changed, 145 insertions, 165 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 6004e7d7115..aaebd4efa00 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -409,10 +409,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     else
       ci_service = @merge_request.source_project.try(:ci_service)
       status = ci_service.commit_status(merge_request.diff_head_sha, merge_request.source_branch) if ci_service
-
-      if ci_service.respond_to?(:commit_coverage)
-        coverage = ci_service.commit_coverage(merge_request.diff_head_sha, merge_request.source_branch)
-      end
     end
 
     response = {
diff --git a/app/models/concerns/reactive_caching.rb b/app/models/concerns/reactive_caching.rb
index 944519a3070..2589215ad19 100644
--- a/app/models/concerns/reactive_caching.rb
+++ b/app/models/concerns/reactive_caching.rb
@@ -55,30 +55,30 @@ module ReactiveCaching
     self.reactive_cache_refresh_interval = 1.minute
     self.reactive_cache_lifetime = 10.minutes
 
-    def calculate_reactive_cache
+    def calculate_reactive_cache(*args)
       raise NotImplementedError
     end
 
-    def with_reactive_cache(&blk)
-      within_reactive_cache_lifetime do
-        data = Rails.cache.read(full_reactive_cache_key)
+    def with_reactive_cache(*args, &blk)
+      within_reactive_cache_lifetime(*args) do
+        data = Rails.cache.read(full_reactive_cache_key(*args))
         yield data if data.present?
       end
     ensure
-      Rails.cache.write(full_reactive_cache_key('alive'), true, expires_in: self.class.reactive_cache_lifetime)
-      ReactiveCachingWorker.perform_async(self.class, id)
+      Rails.cache.write(alive_reactive_cache_key(*args), true, expires_in: self.class.reactive_cache_lifetime)
+      ReactiveCachingWorker.perform_async(self.class, id, *args)
     end
 
-    def clear_reactive_cache!
-      Rails.cache.delete(full_reactive_cache_key)
+    def clear_reactive_cache!(*args)
+      Rails.cache.delete(full_reactive_cache_key(*args))
     end
 
-    def exclusively_update_reactive_cache!
-      locking_reactive_cache do
-        within_reactive_cache_lifetime do
-          enqueuing_update do
-            value = calculate_reactive_cache
-            Rails.cache.write(full_reactive_cache_key, value)
+    def exclusively_update_reactive_cache!(*args)
+      locking_reactive_cache(*args) do
+        within_reactive_cache_lifetime(*args) do
+          enqueuing_update(*args) do
+            value = calculate_reactive_cache(*args)
+            Rails.cache.write(full_reactive_cache_key(*args), value)
           end
         end
       end
@@ -93,22 +93,26 @@ module ReactiveCaching
       ([prefix].flatten + qualifiers).join(':')
     end
 
-    def locking_reactive_cache
-      lease = Gitlab::ExclusiveLease.new(full_reactive_cache_key, timeout: reactive_cache_lease_timeout)
+    def alive_reactive_cache_key(*qualifiers)
+      full_reactive_cache_key(*(qualifiers + ['alive']))
+    end
+
+    def locking_reactive_cache(*args)
+      lease = Gitlab::ExclusiveLease.new(full_reactive_cache_key(*args), timeout: reactive_cache_lease_timeout)
       uuid = lease.try_obtain
       yield if uuid
     ensure
-      Gitlab::ExclusiveLease.cancel(full_reactive_cache_key, uuid)
+      Gitlab::ExclusiveLease.cancel(full_reactive_cache_key(*args), uuid)
     end
 
-    def within_reactive_cache_lifetime
-      yield if Rails.cache.read(full_reactive_cache_key('alive'))
+    def within_reactive_cache_lifetime(*args)
+      yield if Rails.cache.read(alive_reactive_cache_key(*args))
     end
 
-    def enqueuing_update
+    def enqueuing_update(*args)
       yield
     ensure
-      ReactiveCachingWorker.perform_in(self.class.reactive_cache_refresh_interval, self.class, id)
+      ReactiveCachingWorker.perform_in(self.class.reactive_cache_refresh_interval, self.class, id, *args)
     end
   end
 end
diff --git a/app/models/concerns/reactive_service.rb b/app/models/concerns/reactive_service.rb
new file mode 100644
index 00000000000..e1f868a299b
--- /dev/null
+++ b/app/models/concerns/reactive_service.rb
@@ -0,0 +1,10 @@
+module ReactiveService
+  extend ActiveSupport::Concern
+
+  included do
+    include ReactiveCaching
+
+    # Default cache key: class name + project_id
+    self.reactive_cache_key = ->(service) { [ service.class.model_name.singular, service.project_id ] }
+  end
+end
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index b5c76e4d4fe..4819bdbef8c 100644
--- a/app/models/project_services/bamboo_service.rb
+++ b/app/models/project_services/bamboo_service.rb
@@ -1,4 +1,6 @@
 class BambooService < CiService
+  include ReactiveService
+
   prop_accessor :bamboo_url, :build_key, :username, :password
 
   validates :bamboo_url, presence: true, url: true, if: :activated?
@@ -58,31 +60,46 @@ class BambooService < CiService
     %w(push)
   end
 
-  def build_info(sha)
-    @response = get_path("rest/api/latest/result?label=#{sha}")
+  def build_page(sha, ref)
+    with_reactive_cache(sha, ref) {|cached| cached[:build_page] }
   end
 
-  def build_page(sha, ref)
-    build_info(sha) if @response.nil? || !@response.code
+  def commit_status(sha, ref)
+    with_reactive_cache(sha, ref) {|cached| cached[:commit_status] }
+  end
 
-    if @response.code != 200 || @response['results']['results']['size'] == '0'
+  def execute(data)
+    return unless supported_events.include?(data[:object_kind])
+
+    get_path("updateAndBuild.action?buildKey=#{build_key}")
+  end
+
+  def calculate_reactive_cache(sha, ref)
+    response = get_path("rest/api/latest/result?label=#{sha}")
+
+    { build_page: read_build_page(response), commit_status: read_commit_status(response) }
+  end
+
+  private
+
+  def read_build_page(response)
+    if response.code != 200 || response['results']['results']['size'] == '0'
       # If actual build link can't be determined, send user to build summary page.
       URI.join("#{bamboo_url}/", "browse/#{build_key}").to_s
     else
       # If actual build link is available, go to build result page.
-      result_key = @response['results']['results']['result']['planResultKey']['key']
+      result_key = response['results']['results']['result']['planResultKey']['key']
       URI.join("#{bamboo_url}/", "browse/#{result_key}").to_s
     end
   end
 
-  def commit_status(sha, ref)
-    build_info(sha) if @response.nil? || !@response.code
-    return :error unless @response.code == 200 || @response.code == 404
+  def read_commit_status(response)
+    return :error unless response.code == 200 || response.code == 404
 
-    status = if @response.code == 404 || @response['results']['results']['size'] == '0'
+    status = if response.code == 404 || response['results']['results']['size'] == '0'
                'Pending'
              else
-               @response['results']['results']['result']['buildState']
+               response['results']['results']['result']['buildState']
              end
 
     if status.include?('Success')
@@ -96,14 +113,6 @@ class BambooService < CiService
     end
   end
 
-  def execute(data)
-    return unless supported_events.include?(data[:object_kind])
-
-    get_path("updateAndBuild.action?buildKey=#{build_key}")
-  end
-
-  private
-
   def build_url(path)
     URI.join("#{bamboo_url}/", path).to_s
   end
diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb
index fe6d7aabb22..e77942d8f3c 100644
--- a/app/models/project_services/buildkite_service.rb
+++ b/app/models/project_services/buildkite_service.rb
@@ -1,6 +1,8 @@
 require "addressable/uri"
 
 class BuildkiteService < CiService
+  include ReactiveService
+
   ENDPOINT = "https://buildkite.com"
 
   prop_accessor :project_url, :token
@@ -33,13 +35,7 @@ class BuildkiteService < CiService
   end
 
   def commit_status(sha, ref)
-    response = HTTParty.get(commit_status_path(sha), verify: false)
-
-    if response.code == 200 && response['status']
-      response['status']
-    else
-      :error
-    end
+    with_reactive_cache(sha, ref) {|cached| cached[:commit_status] }
   end
 
   def commit_status_path(sha)
@@ -78,6 +74,19 @@ class BuildkiteService < CiService
     ]
   end
 
+  def calculate_reactive_cache(sha, ref)
+    response = HTTParty.get(commit_status_path(sha), verify: false)
+
+    status =
+      if response.code == 200 && response['status']
+        response['status']
+      else
+        :error
+      end
+
+    { commit_status: status }
+  end
+
   private
 
   def webhook_token
diff --git a/app/models/project_services/ci_service.rb b/app/models/project_services/ci_service.rb
index 596c00705ad..4de0106707e 100644
--- a/app/models/project_services/ci_service.rb
+++ b/app/models/project_services/ci_service.rb
@@ -12,15 +12,7 @@ class CiService < Service
     %w(push)
   end
 
-  def merge_request_page(iid, sha, ref)
-    commit_page(sha, ref)
-  end
-
-  def commit_page(sha, ref)
-    build_page(sha, ref)
-  end
-
-  # Return complete url to merge_request page
+  # Return complete url to build page
   #
   # Ex.
   #   http://jenkins.example.com:8888/job/test1/scm/bySHA1/12d65c
@@ -35,23 +27,6 @@ class CiService < Service
   #
   #
   # Ex.
-  #   @service.merge_request_status(9, '13be4ac', 'dev')
-  #   # => 'success'
-  #
-  #   @service.merge_request_status(10, '2abe4ac', 'dev)
-  #   # => 'running'
-  #
-  #
-  def merge_request_status(iid, sha, ref)
-    commit_status(sha, ref)
-  end
-
-  # Return string with build status or :error symbol
-  #
-  # Allowed states: 'success', 'failed', 'running', 'pending', 'skipped'
-  #
-  #
-  # Ex.
   #   @service.commit_status('13be4ac', 'master')
   #   # => 'success'
   #
diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb
index adc78a427ee..4bbbebf54cb 100644
--- a/app/models/project_services/drone_ci_service.rb
+++ b/app/models/project_services/drone_ci_service.rb
@@ -1,4 +1,6 @@
 class DroneCiService < CiService
+  include ReactiveService
+
   prop_accessor :drone_url, :token
   boolean_accessor :enable_ssl_verification
 
@@ -34,14 +36,6 @@ class DroneCiService < CiService
     %w(push merge_request tag_push)
   end
 
-  def merge_request_status_path(iid, sha = nil, ref = nil)
-    url = [drone_url,
-           "gitlab/#{project.namespace.path}/#{project.path}/pulls/#{iid}",
-           "?access_token=#{token}"]
-
-    URI.join(*url).to_s
-  end
-
   def commit_status_path(sha, ref)
     url = [drone_url,
            "gitlab/#{project.namespace.path}/#{project.path}/commits/#{sha}",
@@ -50,54 +44,34 @@ class DroneCiService < CiService
     URI.join(*url).to_s
   end
 
-  def merge_request_status(iid, sha, ref)
-    response = HTTParty.get(merge_request_status_path(iid), verify: enable_ssl_verification)
-
-    if response.code == 200 and response['status']
-      case response['status']
-      when 'killed'
-        :canceled
-      when 'failure', 'error'
-        # Because drone return error if some test env failed
-        :failed
-      else
-        response["status"]
-      end
-    else
-      :error
-    end
-  rescue Errno::ECONNREFUSED
-    :error
+  def commit_status(sha, ref)
+    with_reactive_cache(sha, ref) {|cached| cached[:commit_status] }
   end
 
-  def commit_status(sha, ref)
+  def calculate_reactive_cache(sha, ref)
     response = HTTParty.get(commit_status_path(sha, ref), verify: enable_ssl_verification)
 
-    if response.code == 200 and response['status']
-      case response['status']
-      when 'killed'
-        :canceled
-      when 'failure', 'error'
-        # Because drone return error if some test env failed
-        :failed
+    status =
+      if response.code == 200 and response['status']
+        case response['status']
+        when 'killed'
+          :canceled
+        when 'failure', 'error'
+          # Because drone return error if some test env failed
+          :failed
+        else
+          response["status"]
+        end
       else
-        response["status"]
+        :error
       end
-    else
-      :error
-    end
-  rescue Errno::ECONNREFUSED
-    :error
-  end
 
-  def merge_request_page(iid, sha, ref)
-    url = [drone_url,
-           "gitlab/#{project.namespace.path}/#{project.path}/redirect/pulls/#{iid}"]
-
-    URI.join(*url).to_s
+    { commit_status: status }
+  rescue Errno::ECONNREFUSED
+    { commit_status: :error }
   end
 
-  def commit_page(sha, ref)
+  def build_page(sha, ref)
     url = [drone_url,
            "gitlab/#{project.namespace.path}/#{project.path}/redirect/commits/#{sha}",
            "?branch=#{URI::encode(ref.to_s)}"]
@@ -105,14 +79,6 @@ class DroneCiService < CiService
     URI.join(*url).to_s
   end
 
-  def commit_coverage(sha, ref)
-    nil
-  end
-
-  def build_page(sha, ref)
-    commit_page(sha, ref)
-  end
-
   def title
     'Drone CI'
   end
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index a4a967c9bc9..6726082048f 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -1,4 +1,6 @@
 class TeamcityService < CiService
+  include ReactiveService
+
   prop_accessor :teamcity_url, :build_type, :username, :password
 
   validates :teamcity_url, presence: true, url: true, if: :activated?
@@ -61,43 +63,18 @@ class TeamcityService < CiService
     ]
   end
 
-  def build_info(sha)
-    @response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}")
-  end
-
   def build_page(sha, ref)
-    build_info(sha) if @response.nil? || !@response.code
-
-    if @response.code != 200
-      # If actual build link can't be determined,
-      # send user to build summary page.
-      build_url("viewLog.html?buildTypeId=#{build_type}")
-    else
-      # If actual build link is available, go to build result page.
-      built_id = @response['build']['id']
-      build_url("viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}")
-    end
+    with_reactive_cache(sha, ref) {|cached| cached[:build_page] }
   end
 
   def commit_status(sha, ref)
-    build_info(sha) if @response.nil? || !@response.code
-    return :error unless @response.code == 200 || @response.code == 404
+    with_reactive_cache(sha, ref) {|cached| cached[:commit_status] }
+  end
 
-    status = if @response.code == 404
-               'Pending'
-             else
-               @response['build']['status']
-             end
+  def calculate_reactive_cache(sha, ref)
+    response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}")
 
-    if status.include?('SUCCESS')
-      'success'
-    elsif status.include?('FAILURE')
-      'failed'
-    elsif status.include?('Pending')
-      'pending'
-    else
-      :error
-    end
+    { build_page: read_build_page(response), commit_status: read_commit_status(response) }
   end
 
   def execute(data)
@@ -122,6 +99,40 @@ class TeamcityService < CiService
 
   private
 
+  def read_build_page(response)
+    if response.code != 200
+      # If actual build link can't be determined,
+      # send user to build summary page.
+      build_url("viewLog.html?buildTypeId=#{build_type}")
+    else
+      # If actual build link is available, go to build result page.
+      built_id = response['build']['id']
+      build_url("viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}")
+    end
+  end
+
+  def read_commit_status(response)
+    return :error unless response.code == 200 || response.code == 404
+
+    status = if response.code == 404
+               'Pending'
+             else
+               response['build']['status']
+             end
+
+    return :error unless status.present?
+
+    if status.include?('SUCCESS')
+      'success'
+    elsif status.include?('FAILURE')
+      'failed'
+    elsif status.include?('Pending')
+      'pending'
+    else
+      :error
+    end
+  end
+
   def build_url(path)
     URI.join("#{teamcity_url}/", path).to_s
   end
diff --git a/app/workers/reactive_caching_worker.rb b/app/workers/reactive_caching_worker.rb
index 9af9dae04f0..18b8daf4e1e 100644
--- a/app/workers/reactive_caching_worker.rb
+++ b/app/workers/reactive_caching_worker.rb
@@ -2,7 +2,7 @@ class ReactiveCachingWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
-  def perform(class_name, id)
+  def perform(class_name, id, *args)
     klass = begin
       Kernel.const_get(class_name)
     rescue NameError
@@ -10,6 +10,6 @@ class ReactiveCachingWorker
     end
     return unless klass
 
-    klass.find_by(id: id).try(:exclusively_update_reactive_cache!)
+    klass.find_by(id: id).try(:exclusively_update_reactive_cache!, *args)
   end
 end