Merge pull request #2952 from zendesk/grosser/combine

combine commit-status and github_status

app/assets/javascripts/ref_status_typeahead.js

@@ -39,7 +39,6 @@ function refStatusTypeahead(options){
 
   function show_status_problems(status_list, isDanger) {
     $ref_status_container.removeClass("hidden");
-console.log(isDanger);
     $ref_status_container.toggleClass('alert-danger', isDanger);
     $ref_status_container.toggleClass('alert-warning', !isDanger);
 
@@ -63,24 +62,25 @@ console.log(isDanger);
       url: $("#new_deploy").data("commit-status-url"),
       data: { ref: ref },
       success: function(response) {
-        switch(response.status) {
+        switch(response.state) {
           case "success":
             $ref_status_container.addClass("hidden");
             $tag_form_group.addClass("has-success");
             break;
           case "pending":
+          case "missing":
             $tag_form_group.addClass("has-warning");
-            show_status_problems(response.status_list, false);
+            show_status_problems(response.statuses, false);
             break;
           case "failure":
           case "error":
             $tag_form_group.addClass("has-error");
-            show_status_problems(response.status_list, false);
+            show_status_problems(response.statuses, false);
             break;
           case "fatal":
             $tag_form_group.addClass("has-error");
             $submit_button.prop("disabled", true);
-            show_status_problems(response.status_list, true);
+            show_status_problems(response.statuses, true);
             break;
           default:
             alert("Unexpected response: " + response.toString());

app/controllers/commit_statuses_controller.rb

@@ -6,7 +6,7 @@ class CommitStatusesController < ApplicationController
 
   def show
     stage = current_project.stages.find_by_permalink!(params.require(:stage_id))
-    commit_status = CommitStatus.new(stage, params.require(:ref))
-    render json: {status: commit_status.status, status_list: commit_status.status_list}
+    commit_status = CommitStatus.new(stage.project, params.require(:ref), stage: stage)
+    render json: {state: commit_status.state, statuses: commit_status.statuses}
   end
 end

app/controllers/integrations/github_controller.rb

@@ -12,16 +12,15 @@ def self.secret_token
   end
 
   def create
-    handle_commit_status_event if github_event_type == "status"
-
+    expire_commit_status if github_event_type == "status"
     super
   end
 
   protected
 
-  def handle_commit_status_event
-    # Touch all releases of the sha in the project.
-    project.releases.where(commit: params[:sha].to_s).each(&:touch)
+  def expire_commit_status
+    commit = params[:sha].to_s
+    CommitStatus.new(project, commit).expire_cache(commit)
   end
 
   def payload

app/models/commit_status.rb

@@ -2,24 +2,23 @@
 # Used to display all warnings/failures before user actually deploys
 class CommitStatus
   # See ref_status_typeahead.js for how statuses are handled
-  STATUS_PRIORITY = {
-    success: 0,
-    pending: 1,
-    failure: 2,
-    error: 3,
-    fatal: 4
-  }.freeze
-
-  def initialize(stage, reference)
-    @stage = stage
+  # See https://developer.github.com/v3/repos/statuses for api details
+  # - fatal is our own state that blocks deploys
+  # - missing is our own state that means we could not determine the status
+  STATE_PRIORITY = [:success, :pending, :missing, :failure, :error, :fatal].freeze
+  UNDETERMINED = ["pending", "missing"].freeze
+
+  def initialize(project, reference, stage: nil)
+    @project = project
     @reference = reference
+    @stage = stage
   end
 
-  def status
+  def state
     combined_status.fetch(:state)
   end
 
-  def status_list
+  def statuses
     list = combined_status.fetch(:statuses).map(&:to_h)
     if list.empty?
       list << {
@@ -32,39 +31,71 @@ def status_list
     list
   end
 
+  def expire_cache(commit)
+    Rails.cache.delete(cache_key(commit))
+  end
+
   private
 
   def combined_status
     @combined_status ||= begin
-      statuses = [github_status, release_status, *ref_statuses]
-
-      statuses.each_with_object({}) { |status, merged_statuses| merge(merged_statuses, status) }
+      statuses = [github_status]
+      statuses += [release_status, *ref_statuses].compact if @stage
+      statuses[1..-1].each_with_object(statuses[0]) { |status, merged| merge(merged, status) }
     end
   end
 
   def merge(a, b)
-    return a unless b
-    a[:state] = pick_highest_state(a[:state], b.fetch(:state))
-    (a[:statuses] ||= []).concat b.fetch(:statuses)
+    a[:state] = [a.fetch(:state), b.fetch(:state)].max_by { |state| STATE_PRIORITY.index(state.to_sym) }
+    a.fetch(:statuses).concat b.fetch(:statuses)
   end
 
-  # picks the state with the higher priority
-  def pick_highest_state(a, b)
-    return b if a.nil?
-    STATUS_PRIORITY[a.to_sym] > STATUS_PRIORITY[b.to_sym] ? a : b
-  end
-
-  # need to do weird escape logic since other wise either 'foo/bar' or 'bar[].foo' do not work
+  # NOTE: reply is an api object that does not support .fetch
   def github_status
-    escaped_ref = @reference.gsub(/[^a-zA-Z\/\d_-]+/) { |v| CGI.escape(v) }
-    GITHUB.combined_status(@stage.project.repository_path, escaped_ref).to_h
+    static = @reference.match?(Build::SHA1_REGEX) || @reference.match?(Release::VERSION_REGEX)
+    expires_in = ->(reply) { cache_duration(reply) }
+    cache_fetch_if static, cache_key(@reference), expires_in: expires_in do
+      GITHUB.combined_status(@project.repository_path, @reference).to_h
+    end
   rescue Octokit::NotFound
     {
-      state: "failure",
-      statuses: [{"state": "Reference", description: "'#{@reference}' does not exist"}]
+      state: "missing",
+      statuses: [{
+        context: "Reference", # for releases/show.html.erb
+        state: "missing",
+        description: "'#{@reference}' does not exist"
+      }]
     }
   end
 
+  def cache_duration(github_status)
+    statuses = github_status[:statuses]
+    if statuses.empty? # does not have any statuses, chances are commit is new
+      5.minutes # NOTE: could fetch commit locally without pulling to check it's age
+    elsif (Time.now - statuses.map { |s| s[:updated_at] }.max) > 1.hour # no new updates expected
+      1.day
+    elsif statuses.any? { |s| UNDETERMINED.include?(s[:state]) } # expecting update shortly
+      1.minute
+    else # user might re-run test or success changes into failure when new status arrives
+      10.minutes
+    end
+  end
+
+  def cache_key(commit)
+    ['commit-status', @project.id, commit]
+  end
+
+  def cache_fetch_if(condition, key, expires_in:)
+    return yield unless condition
+
+    old = Rails.cache.read(key)
+    return old if old
+
+    current = yield
+    Rails.cache.write(key, current, expires_in: expires_in.call(current))
+    current
+  end
+
   # checks if other stages that deploy to the same hosts as this stage have deployed a newer release
   # @return [nil, error-state]
   def release_status

app/models/release.rb

@@ -32,10 +32,6 @@ def to_param
     version
   end
 
-  def github_status
-    @github_status ||= GithubStatus.fetch(self)
-  end
-
   def self.find_by_param!(version)
     if number = version[VERSION_REGEX, 1]
       find_by_number!(number)

app/views/deploys/_buddy_check.html.erb

@@ -1,14 +1,11 @@
 <div class="row deploy-check">
-  <% if status = CommitStatus.new(@deploy.stage, @deploy.reference) rescue nil %>
-    <p>
-      Commit status for <%= short_sha @deploy.reference %>: <%= status.status %><br/>
-      <% status.status_list.each do |info| %>
-        <%= info.fetch(:state) %>: <%= info.fetch(:description) %><br/>
-      <% end %>
-    </p>
-  <% else %>
-    <p>Error fetching commit status.</p>
-  <% end %>
+  <% commit_status = CommitStatus.new(@deploy.stage.project, @deploy.reference, stage: @deploy.stage) %>
+  <p>
+    Commit status for <%= short_sha @deploy.reference %>: <%= commit_status.state %><br/>
+    <% commit_status.statuses.each do |info| %>
+      <%= info.fetch(:state) %>: <%= info.fetch(:description) %><br/>
+    <% end %>
+  </p>
   <br/><br/>
 
   <% if @deploy.started_by?(current_user) %>

app/views/releases/_release.html.erb

@@ -4,7 +4,7 @@
   </td>
   <td>
     <% if github_ok? %>
-      <%= github_commit_status_icon release.github_status.state %>
+      <%= github_commit_status_icon CommitStatus.new(release.project, release.commit).state %>
     <% else %>
       <%= github_commit_status_icon "missing" %>
     <% end %>

app/views/releases/show.html.erb

@@ -18,8 +18,9 @@
 
   <legend>Commit Status</legend>
   <p>
-    <% @release.github_status.statuses.each do |status| %>
-      <%= github_commit_status_icon(status.state) %> <b><%= status.context %>:</b> <%= link_to status.description, status.url %><br/>
+    <% CommitStatus.new(@release.project, @release.commit).statuses.each do |status| %>
+      <%= github_commit_status_icon(status.fetch(:state)) %>
+      <b><%= status[:context] %>:</b> <%= link_to status[:description], status[:url] %><br/>
     <% end %>
   </p>
 

test/controllers/commit_statuses_controller_test.rb

@@ -34,8 +34,8 @@
       describe 'valid' do
         let(:commit_status_data) do
           {
-            status: 'pending',
-            status_list: [{status: 'pending', description: 'the Travis build is still running'}]
+            state: 'pending',
+            statuses: [{status: 'pending', description: 'the Travis build is still running'}]
           }
         end
 

test/controllers/integrations/github_controller_test.rb

@@ -89,32 +89,12 @@
   end
 
   describe 'with a commit status event' do
-    it 'updates all releases of that commit' do
-      sha = "dc395381e650f3bac18457909880829fc20e34ba"
-
-      release = project.releases.create!(
-        commit: sha,
-        author: users(:deployer)
-      )
-
-      # Fast forward the clock.
-      later = 1.minute.from_now
-      Time.stubs(:now).returns later
-
-      payload = {
-        token: project.token,
-        sha: sha
-      }
-
-      request.headers['X-Github-Event'] = 'status'
-      post :create, params: payload
+    before { request.headers['X-Github-Event'] = 'status' }
 
+    it 'expires github status' do
+      Rails.cache.expects(:delete).with(['commit-status', project.id, 'dc395381e650f3bac18457909880829fc20e34ba'])
+      post :create, params: {token: project.token, sha: "dc395381e650f3bac18457909880829fc20e34ba"}
       assert_response :success
-
-      # Time objects can't be reliably compared due to the use of floating
-      # point numbers in their representation, so we convert to Integer before
-      # comparing.
-      release.reload.updated_at.to_i.must_equal later.to_i
     end
   end
 

test/controllers/releases_controller_test.rb

@@ -16,6 +16,7 @@
           context: "oompa/loompa",
           target_url: "https://chocolate-factory.com/test/wonka",
           description: "Ooompa Loompa!",
+          updated_at: Time.now.iso8601,
           created_at: Time.now.iso8601
         }
       ]