diff --git a/app/assets/javascripts/ref_status_typeahead.js b/app/assets/javascripts/ref_status_typeahead.js index 8514b052e6..4b42f74880 100644 --- a/app/assets/javascripts/ref_status_typeahead.js +++ b/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()); diff --git a/app/controllers/commit_statuses_controller.rb b/app/controllers/commit_statuses_controller.rb index 1814b7aacd..3db2dea106 100644 --- a/app/controllers/commit_statuses_controller.rb +++ b/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 diff --git a/app/controllers/integrations/github_controller.rb b/app/controllers/integrations/github_controller.rb index 7fb5ace261..d11479e861 100644 --- a/app/controllers/integrations/github_controller.rb +++ b/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 diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6e411881fc..644374e326 100644 --- a/app/models/commit_status.rb +++ b/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 diff --git a/app/models/github_status.rb b/app/models/github_status.rb deleted file mode 100644 index 7ef93897c4..0000000000 --- a/app/models/github_status.rb +++ /dev/null @@ -1,84 +0,0 @@ -# frozen_string_literal: true -class GithubStatus - Status = Struct.new(:context, :latest_status) do - def state - latest_status.state - end - - def description - latest_status.description - end - - def url - latest_status.target_url - end - - def success? - state == "success" - end - - def failure? - state == "failure" - end - - def pending? - state == "pending" - end - end - - attr_reader :state, :statuses - - def initialize(state, statuses) - @state = state - @statuses = statuses - end - - def self.fetch(release) - repo = release.project.repository_path - ref = release.commit - - # Base the cache key on the Release, so that an update to it effectively - # clears the cache. - cache_key = [name, release] - - response = Rails.cache.read(cache_key) - - # Fetch the data if the cache returned nil. - response ||= - begin - GITHUB.combined_status(repo, ref) - rescue Octokit::Error - nil - end - - # Fall back to a "missing" status. - return new("missing", []) if response.nil? - - statuses = response.statuses.group_by(&:context).map do |context, statuses| - Status.new(context, statuses.max_by { |status| status.created_at.to_i }) - end - - # Don't cache pending statuses, since we expect updates soon. - unless statuses.any?(&:pending?) - Rails.cache.write(cache_key, response, expires_in: 1.hour) - end - - new(response.state, statuses) - end - - def success? - state == "success" - end - - def failure? - state == "failure" - end - - def pending? - state == "pending" - end - - def missing? - state == "missing" - end -end diff --git a/app/models/release.rb b/app/models/release.rb index cfd025a460..3d1ad3b2c6 100644 --- a/app/models/release.rb +++ b/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) diff --git a/app/views/deploys/_buddy_check.html.erb b/app/views/deploys/_buddy_check.html.erb index 465bc7addb..1877c53123 100644 --- a/app/views/deploys/_buddy_check.html.erb +++ b/app/views/deploys/_buddy_check.html.erb @@ -1,14 +1,11 @@
- <% if status = CommitStatus.new(@deploy.stage, @deploy.reference) rescue nil %> -

- Commit status for <%= short_sha @deploy.reference %>: <%= status.status %>
- <% status.status_list.each do |info| %> - <%= info.fetch(:state) %>: <%= info.fetch(:description) %>
- <% end %> -

- <% else %> -

Error fetching commit status.

- <% end %> + <% commit_status = CommitStatus.new(@deploy.stage.project, @deploy.reference, stage: @deploy.stage) %> +

+ Commit status for <%= short_sha @deploy.reference %>: <%= commit_status.state %>
+ <% commit_status.statuses.each do |info| %> + <%= info.fetch(:state) %>: <%= info.fetch(:description) %>
+ <% end %> +



<% if @deploy.started_by?(current_user) %> diff --git a/app/views/releases/_release.html.erb b/app/views/releases/_release.html.erb index fbb3869c17..d3780a53e5 100644 --- a/app/views/releases/_release.html.erb +++ b/app/views/releases/_release.html.erb @@ -4,7 +4,7 @@ <% 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 %> diff --git a/app/views/releases/show.html.erb b/app/views/releases/show.html.erb index dedfb181f6..a5663fc75d 100644 --- a/app/views/releases/show.html.erb +++ b/app/views/releases/show.html.erb @@ -18,8 +18,9 @@ Commit Status

- <% @release.github_status.statuses.each do |status| %> - <%= github_commit_status_icon(status.state) %> <%= status.context %>: <%= link_to status.description, status.url %>
+ <% CommitStatus.new(@release.project, @release.commit).statuses.each do |status| %> + <%= github_commit_status_icon(status.fetch(:state)) %> + <%= status[:context] %>: <%= link_to status[:description], status[:url] %>
<% end %>

diff --git a/test/controllers/commit_statuses_controller_test.rb b/test/controllers/commit_statuses_controller_test.rb index 93721dcb90..d19cd4adcb 100644 --- a/test/controllers/commit_statuses_controller_test.rb +++ b/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 diff --git a/test/controllers/integrations/github_controller_test.rb b/test/controllers/integrations/github_controller_test.rb index c9ffec77c9..07ffed6353 100644 --- a/test/controllers/integrations/github_controller_test.rb +++ b/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 diff --git a/test/controllers/releases_controller_test.rb b/test/controllers/releases_controller_test.rb index ee05b15a7f..aaf796c8a0 100644 --- a/test/controllers/releases_controller_test.rb +++ b/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 } ] diff --git a/test/helpers/releases_helper_test.rb b/test/helpers/releases_helper_test.rb index bb53963561..871ba1283d 100644 --- a/test/helpers/releases_helper_test.rb +++ b/test/helpers/releases_helper_test.rb @@ -45,6 +45,13 @@ html.must_include "text-primary" html.must_include "Github status: pending" end + + it "renders an icon for pending status" do + html = github_commit_status_icon("pending") + html.must_include "glyphicon-hourglass" + html.must_include "text-primary" + html.must_include "Github status: pending" + end end describe "#link_to_deploy_stage" do diff --git a/test/models/commit_status_test.rb b/test/models/commit_status_test.rb index f75db57c6b..32d6787aa1 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -18,7 +18,7 @@ def self.deploying_a_previous_release end def success! - stub_github_api(url, statuses: [{foo: "bar"}], state: "success") + stub_github_api(url, statuses: [{foo: "bar", updated_at: 1.day.ago}], state: "success") end def failure! @@ -26,22 +26,55 @@ def failure! end def status(stage_param: stage, reference_param: reference) - @status ||= CommitStatus.new(stage_param, reference_param) + CommitStatus.new(stage_param.project, reference_param, stage: stage_param) end let(:stage) { stages(:test_staging) } let(:reference) { 'master' } let(:url) { "repos/#{stage.project.repository_path}/commits/#{reference}/status" } - describe "#status" do + describe "#state" do it "returns state" do success! - status.status.must_equal 'success' + status.state.must_equal 'success' end - it "is failure when not found" do + it "is missing when not found" do failure! - status.status.must_equal 'failure' + status.state.must_equal 'missing' + end + + it "works without stage" do + success! + s = status + s.instance_variable_set(:@stage, nil) + s.state.must_equal "success" + end + + it "does not cache changing references" do + request = success! + status.state.must_equal 'success' + status.state.must_equal 'success' + assert_requested request, times: 2 + end + + describe "caching static references" do + let(:reference) { 'v4.2' } + + it "caches github state accross instances" do + request = success! + status.state.must_equal 'success' + status.state.must_equal 'success' + assert_requested request, times: 1 + end + + it "can expire cache" do + request = success! + status.state.must_equal 'success' + status.expire_cache reference + status.state.must_equal 'success' + assert_requested request, times: 2 + end end describe "when deploying a previous release" do @@ -50,7 +83,7 @@ def status(stage_param: stage, reference_param: reference) it "warns" do success! assert_sql_queries 10 do - status.status.must_equal 'error' + status.state.must_equal 'error' end end @@ -59,13 +92,13 @@ def status(stage_param: stage, reference_param: reference) deploys(:succeeded_test).update_column(:reference, 'v4.1') # old is lower deploy.update_column(:reference, 'v4.3') # new is higher success! - status.status.must_equal 'error' + status.state.must_equal 'error' end it "ignores when previous deploy was the same or lower" do deploy.update_column(:reference, reference) success! - status.status.must_equal 'success' + status.state.must_equal 'success' end describe "when previous deploy was higher numerically" do @@ -73,8 +106,9 @@ def status(stage_param: stage, reference_param: reference) it "warns" do success! - status.status.must_equal 'error' - status.status_list[1][:description].must_equal( + status = status() + status.state.must_equal 'error' + status.statuses[1][:description].must_equal( "v4.10 was deployed to deploy groups in this stage by Production" ) end @@ -84,8 +118,9 @@ def status(stage_param: stage, reference_param: reference) other.update_column(:reference, 'v4.9') success! - status.status.must_equal 'error' - status.status_list[1][:description].must_equal( + status = status() + status.state.must_equal 'error' + status.statuses[1][:description].must_equal( "v4.9, v4.10 was deployed to deploy groups in this stage by Staging, Production" ) end @@ -94,43 +129,33 @@ def status(stage_param: stage, reference_param: reference) it "ignores when previous deploy was not a version" do deploy.update_column(:reference, 'master') success! - status.status.must_equal 'success' + status.state.must_equal 'success' end it "ignores when previous deploy was failed" do deploy.job.update_column(:status, 'faild') success! - status.status.must_equal 'success' - end - end - - describe "with bad ref" do - let(:reference) { '[/r' } - let(:url) { "repos/#{stage.project.repository_path}/commits/%255B/r/status" } - - it "escapes the url" do - failure! - status.status.must_equal 'failure' + status.state.must_equal 'success' end end end - describe "#status_list" do + describe "#statuses" do it "returns list" do success! - status.status_list.must_equal [{foo: "bar"}] + status.statuses.map { |s| s[:foo] }.must_equal ["bar"] end it "shows that github is waiting for statuses to come when non has arrived yet ... or none are set up" do stub_github_api(url, statuses: [], state: "pending") - list = status.status_list + list = status.statuses list.map { |s| s[:state] }.must_equal ["pending"] list.first[:description].must_include "No status was reported" end - it "returns failure on Reference when not found list for consistent status display" do + it "returns Reference context for release/show display" do failure! - status.status_list.map { |s| s[:state] }.must_equal ["Reference"] + status.statuses.map { |s| s[:context] }.must_equal ["Reference"] end describe "when deploying a previous release" do @@ -138,7 +163,7 @@ def status(stage_param: stage, reference_param: reference) it "merges" do success! - status.status_list.must_equal [ + status.statuses.each { |s| s.delete(:updated_at) }.must_equal [ {foo: "bar"}, {state: "Old Release", description: "v4.3 was deployed to deploy groups in this stage by Production"} ] @@ -146,20 +171,6 @@ def status(stage_param: stage, reference_param: reference) end end - describe "#resolve_states" do - it 'picks the first state if it has higher priority' do - status.send(:pick_highest_state, 'error', 'success').must_equal 'error' - end - - it 'picks the second state if it has higher priority' do - status.send(:pick_highest_state, 'success', 'error').must_equal 'error' - end - - it 'returns second state if first state is nil' do - status.send(:pick_highest_state, nil, 'pending').must_equal 'pending' - end - end - describe '#ref_statuses' do let(:production_stage) { stages(:test_production) } @@ -197,4 +208,39 @@ def status(stage_param: stage, reference_param: reference) status.send(:ref_statuses).must_equal [{foo: :bar}] end end + + describe "#cache_fetch_if" do + it "fetches old" do + Rails.cache.write('a', 1) + status.send(:cache_fetch_if, true, 'a', expires_in: :raise) { 2 }.must_equal 1 + end + + it "does not cache when not requested" do + Rails.cache.write('a', 1) + status.send(:cache_fetch_if, false, 'a', expires_in: :raise) { 2 }.must_equal 2 + Rails.cache.read('a').must_equal 1 + end + + it "caches with expiration" do + status.send(:cache_fetch_if, true, 'a', expires_in: ->(_) { 1 }) { 2 }.must_equal 2 + Rails.cache.read('a').must_equal 2 + end + end + + describe "#cache_duration" do + it "is short when we do not know if the commit is new or old" do + status.send(:cache_duration, statuses: []).must_equal 5.minutes + end + + it "is long when we do not expect new updates" do + status.send(:cache_duration, statuses: [{updated_at: 1.day.ago}]).must_equal 1.day + end + + it "is short when we expect updates shortly" do + status.send(:cache_duration, statuses: [{updated_at: 10.minutes.ago, state: "pending"}]).must_equal 1.minute + end + it "is medium when some status might still be changing or coming in late" do + status.send(:cache_duration, statuses: [{updated_at: 10.minutes.ago, state: "success"}]).must_equal 10.minutes + end + end end diff --git a/test/models/github_status_test.rb b/test/models/github_status_test.rb deleted file mode 100644 index 20d5d6ac51..0000000000 --- a/test/models/github_status_test.rb +++ /dev/null @@ -1,98 +0,0 @@ -# frozen_string_literal: true -require_relative '../test_helper' - -SingleCov.covered! - -describe GithubStatus do - let(:repo) { "oompa/loompa" } - let(:ref) { "wonka" } - let(:project) { stub("project", repository_path: repo) } - let(:release) { stub("release", project: project, commit: ref) } - - describe "#state" do - let(:status) { GithubStatus.fetch(release) } - - it "returns `missing` if there's no response from Github" do - stub_api({}, 401) - status.state.must_equal "missing" - end - - it "returns the Github state from the response" do - stub_api({state: "party", statuses: []}, 200) - status.state.must_equal "party" - end - end - - describe "#statuses" do - let(:status) { GithubStatus.fetch(release) } - - it "returns a single status per context" do - # The most recent status is used. - statuses = [ - {context: "A", created_at: 1, state: "pending"}, - {context: "B", created_at: 1, state: "success"}, - {context: "A", created_at: 2, state: "failure"}, - ] - - stub_api({state: "pending", statuses: statuses}, 200) - - status.statuses.count.must_equal 2 - - status_a = status.statuses.first - status_b = status.statuses.last - - assert status_a.failure? - assert status_b.success? - end - - it "includes the state of each status" do - statuses = [ - {context: "A", created_at: 1, state: "pending"}, - ] - - stub_api({state: "pending", statuses: statuses}, 200) - - status.statuses.first.state.must_equal "pending" - - assert status.statuses.first.pending? - assert !status.statuses.first.success? - assert !status.statuses.first.failure? - end - - it "includes the URL of each status" do - statuses = [ - {context: "A", created_at: 1, state: "pending", target_url: "http://acme.com/123"}, - ] - - stub_api({state: "pending", statuses: statuses}, 200) - - status.statuses.first.url.must_equal "http://acme.com/123" - end - - it "includes the description of each status" do - statuses = [ - {context: "A", created_at: 1, state: "pending", description: "hello"}, - ] - - stub_api({state: "pending", statuses: statuses}, 200) - - status.statuses.first.description.must_equal "hello" - end - end - - it "has a query method for each state" do - assert GithubStatus.new("success", []).success? - assert GithubStatus.new("failure", []).failure? - assert GithubStatus.new("pending", []).pending? - assert GithubStatus.new("missing", []).missing? - - refute GithubStatus.new("wonka", []).success? - refute GithubStatus.new("wonka", []).failure? - refute GithubStatus.new("wonka", []).pending? - refute GithubStatus.new("wonka", []).missing? - end - - def stub_api(body, status = 200) - stub_github_api "repos/#{repo}/commits/#{ref}/status", body, status - end -end diff --git a/test/models/release_test.rb b/test/models/release_test.rb index d8fd2cf2d3..4d18d7a877 100644 --- a/test/models/release_test.rb +++ b/test/models/release_test.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative '../test_helper' -SingleCov.covered! uncovered: 3 +SingleCov.covered! uncovered: 2 describe Release do let(:author) { users(:deployer) }