From 2cf228166cb80703ee6dbba6b87b9d0082236168 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Fri, 21 Sep 2018 20:53:49 -0700 Subject: [PATCH 1/7] covert git references to commits so we can cache them --- app/models/commit_status.rb | 5 ++--- test/models/commit_status_test.rb | 19 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6e411881fc..99eedd2309 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -54,10 +54,9 @@ def pick_highest_state(a, b) 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 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 + commit = @stage.project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) + GITHUB.combined_status(@stage.project.repository_path, commit).to_h rescue Octokit::NotFound { state: "failure", diff --git a/test/models/commit_status_test.rb b/test/models/commit_status_test.rb index f75db57c6b..079fdc88f8 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -31,7 +31,9 @@ def status(stage_param: stage, reference_param: reference) let(:stage) { stages(:test_staging) } let(:reference) { 'master' } - let(:url) { "repos/#{stage.project.repository_path}/commits/#{reference}/status" } + let(:url) { "repos/#{stage.project.repository_path}/commits/abcabcabc/status" } + + before { GitRepository.any_instance.stubs(:commit_from_ref).returns("abcabcabc") } describe "#status" do it "returns state" do @@ -44,6 +46,11 @@ def status(stage_param: stage, reference_param: reference) status.status.must_equal 'failure' end + it "is failure commit is not found" do + GitRepository.any_instance.stubs(:commit_from_ref).returns(nil) + status.status.must_equal 'failure' + end + describe "when deploying a previous release" do deploying_a_previous_release @@ -103,16 +110,6 @@ def status(stage_param: stage, reference_param: reference) 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' - end - end end describe "#status_list" do From 65d90cfe9374a2079a352dd6bea151b0dd6a36a9 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Fri, 21 Sep 2018 22:06:35 -0700 Subject: [PATCH 2/7] refactor commit_status to only use github when no stage is given get rid of nil logic and make methods more aligned with github response fields --- .../javascripts/ref_status_typeahead.js | 8 +-- app/controllers/commit_statuses_controller.rb | 4 +- app/models/commit_status.rb | 29 ++++++----- app/views/deploys/_buddy_check.html.erb | 17 +++---- test/models/commit_status_test.rb | 49 ++++++++++--------- 5 files changed, 53 insertions(+), 54 deletions(-) diff --git a/app/assets/javascripts/ref_status_typeahead.js b/app/assets/javascripts/ref_status_typeahead.js index 8514b052e6..19834bae68 100644 --- a/app/assets/javascripts/ref_status_typeahead.js +++ b/app/assets/javascripts/ref_status_typeahead.js @@ -63,24 +63,24 @@ 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": $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/models/commit_status.rb b/app/models/commit_status.rb index 99eedd2309..ed78069368 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -2,7 +2,7 @@ # Used to display all warnings/failures before user actually deploys class CommitStatus # See ref_status_typeahead.js for how statuses are handled - STATUS_PRIORITY = { + STATE_PRIORITY = { success: 0, pending: 1, failure: 2, @@ -10,16 +10,17 @@ class CommitStatus fatal: 4 }.freeze - def initialize(stage, reference) - @stage = stage + 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 << { @@ -36,27 +37,25 @@ def status_list 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.fetch(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 + STATE_PRIORITY[a.to_sym] > STATE_PRIORITY[b.to_sym] ? a : b end def github_status - commit = @stage.project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) - GITHUB.combined_status(@stage.project.repository_path, commit).to_h + commit = @project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) + GITHUB.combined_status(@project.repository_path, commit).to_h rescue Octokit::NotFound { state: "failure", 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/test/models/commit_status_test.rb b/test/models/commit_status_test.rb index 079fdc88f8..1284420329 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -26,7 +26,7 @@ def failure! end def status(stage_param: stage, reference_param: reference) - @status ||= CommitStatus.new(stage_param, reference_param) + @status ||= CommitStatus.new(stage_param.project, reference_param, stage: stage_param) end let(:stage) { stages(:test_staging) } @@ -35,20 +35,27 @@ def status(stage_param: stage, reference_param: reference) before { GitRepository.any_instance.stubs(:commit_from_ref).returns("abcabcabc") } - 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 failure! - status.status.must_equal 'failure' + status.state.must_equal 'failure' end it "is failure commit is not found" do GitRepository.any_instance.stubs(:commit_from_ref).returns(nil) - status.status.must_equal 'failure' + status.state.must_equal 'failure' + end + + it "works without stage" do + success! + s = status + s.instance_variable_set(:@stage, nil) + s.state.must_equal "success" end describe "when deploying a previous release" do @@ -57,7 +64,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 @@ -66,13 +73,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 @@ -80,8 +87,8 @@ 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.state.must_equal 'error' + status.statuses[1][:description].must_equal( "v4.10 was deployed to deploy groups in this stage by Production" ) end @@ -91,8 +98,8 @@ 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.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 @@ -101,33 +108,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' + 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.must_equal [{foo: "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 failure! - status.status_list.map { |s| s[:state] }.must_equal ["Reference"] + status.statuses.map { |s| s[:state] }.must_equal ["Reference"] end describe "when deploying a previous release" do @@ -135,7 +142,7 @@ def status(stage_param: stage, reference_param: reference) it "merges" do success! - status.status_list.must_equal [ + status.statuses.must_equal [ {foo: "bar"}, {state: "Old Release", description: "v4.3 was deployed to deploy groups in this stage by Production"} ] @@ -151,10 +158,6 @@ def status(stage_param: stage, reference_param: reference) 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 From 23f979c205c5b78fccb28a6560e6adc27bd81e04 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Fri, 21 Sep 2018 22:32:17 -0700 Subject: [PATCH 3/7] cache github commit status when it is findable and not pending --- .../integrations/github_controller.rb | 5 ++- app/models/commit_status.rb | 22 ++++++++++- test/models/commit_status_test.rb | 38 ++++++++++++++++++- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/app/controllers/integrations/github_controller.rb b/app/controllers/integrations/github_controller.rb index 7fb5ace261..93d0545259 100644 --- a/app/controllers/integrations/github_controller.rb +++ b/app/controllers/integrations/github_controller.rb @@ -20,8 +20,9 @@ def create 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) + commit = params[:sha].to_s + CommitStatus.new(project, commit).expire_cache(commit) + project.releases.where(commit: commit).each(&:touch) end def payload diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index ed78069368..17bee5aa39 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -33,6 +33,10 @@ def statuses list end + def expire_cache(commit) + Rails.cache.delete(cache_key(commit)) + end + private def combined_status @@ -55,7 +59,10 @@ def pick_highest_state(a, b) def github_status commit = @project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) - GITHUB.combined_status(@project.repository_path, commit).to_h + write_if = ->(s) { s.fetch(:statuses).none? { |s| s.fetch(:state) == "pending" } } + cache_fetch cache_key(commit), expires_in: 1.hour, write_if: write_if do + GITHUB.combined_status(@project.repository_path, commit).to_h + end rescue Octokit::NotFound { state: "failure", @@ -63,6 +70,19 @@ def github_status } end + def cache_key(commit) + ['commit-status', @project.id, commit] + end + + def cache_fetch(key, options) + old = Rails.cache.read(key) + return old if old + + current = yield + Rails.cache.write(key, current, options) if options[:write_if].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/test/models/commit_status_test.rb b/test/models/commit_status_test.rb index 1284420329..073f100c89 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -26,7 +26,7 @@ def failure! end def status(stage_param: stage, reference_param: reference) - @status ||= CommitStatus.new(stage_param.project, reference_param, stage: stage_param) + CommitStatus.new(stage_param.project, reference_param, stage: stage_param) end let(:stage) { stages(:test_staging) } @@ -58,6 +58,23 @@ def status(stage_param: stage, reference_param: reference) s.state.must_equal "success" end + describe "caching" do + 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 'abcabcabc' + status.state.must_equal 'success' + assert_requested request, times: 2 + end + end + describe "when deploying a previous release" do deploying_a_previous_release @@ -87,6 +104,7 @@ def status(stage_param: stage, reference_param: reference) it "warns" do success! + 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" @@ -98,6 +116,7 @@ def status(stage_param: stage, reference_param: reference) other.update_column(:reference, 'v4.9') success! + 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" @@ -197,4 +216,21 @@ def status(stage_param: stage, reference_param: reference) status.send(:ref_statuses).must_equal [{foo: :bar}] end end + + describe "#cache_fetch" do + it "fetches old" do + Rails.cache.write('a', 1) + status.send(:cache_fetch, 'a', write_if: :raise) { 2 }.must_equal 1 + end + + it "writes new when ok" do + status.send(:cache_fetch, 'a', write_if: ->(_) { true }) { 2 }.must_equal 2 + Rails.cache.read('a').must_equal 2 + end + + it "does not write new when not ok" do + status.send(:cache_fetch, 'a', write_if: ->(_) { false }) { 2 }.must_equal 2 + Rails.cache.read('a').must_equal nil + end + end end From 3a975ab44bd66730b87d2eb2e6b69468c6447667 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Sat, 22 Sep 2018 09:06:34 -0700 Subject: [PATCH 4/7] use array for priority --- app/models/commit_status.rb | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 17bee5aa39..8340290c8c 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -2,13 +2,9 @@ # Used to display all warnings/failures before user actually deploys class CommitStatus # See ref_status_typeahead.js for how statuses are handled - STATE_PRIORITY = { - success: 0, - pending: 1, - failure: 2, - error: 3, - fatal: 4 - }.freeze + # See https://developer.github.com/v3/repos/statuses for api details + # fatal is our own state that blocks deploys + STATE_PRIORITY = [:success, :pending, :failure, :error, :fatal].freeze def initialize(project, reference, stage: nil) @project = project @@ -48,15 +44,10 @@ def combined_status end def merge(a, b) - a[:state] = [a.fetch(:state), b.fetch(:state)].max_by { |state| STATE_PRIORITY.fetch(state.to_sym) } + 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) - STATE_PRIORITY[a.to_sym] > STATE_PRIORITY[b.to_sym] ? a : b - end - def github_status commit = @project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) write_if = ->(s) { s.fetch(:statuses).none? { |s| s.fetch(:state) == "pending" } } From a19d8bccfb64d91bf8ad4ad39a6fb97676cb1d92 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Sat, 22 Sep 2018 09:39:01 -0700 Subject: [PATCH 5/7] combine commit_status and github_status --- .../javascripts/ref_status_typeahead.js | 2 +- .../integrations/github_controller.rb | 6 +- app/models/commit_status.rb | 16 ++- app/models/github_status.rb | 84 ---------------- app/models/release.rb | 4 - app/views/releases/_release.html.erb | 2 +- app/views/releases/show.html.erb | 5 +- .../commit_statuses_controller_test.rb | 4 +- .../integrations/github_controller_test.rb | 28 +----- test/helpers/releases_helper_test.rb | 7 ++ test/models/commit_status_test.rb | 22 ++--- test/models/github_status_test.rb | 98 ------------------- test/models/release_test.rb | 2 +- 13 files changed, 38 insertions(+), 242 deletions(-) delete mode 100644 app/models/github_status.rb delete mode 100644 test/models/github_status_test.rb diff --git a/app/assets/javascripts/ref_status_typeahead.js b/app/assets/javascripts/ref_status_typeahead.js index 19834bae68..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); @@ -69,6 +68,7 @@ console.log(isDanger); $tag_form_group.addClass("has-success"); break; case "pending": + case "missing": $tag_form_group.addClass("has-warning"); show_status_problems(response.statuses, false); break; diff --git a/app/controllers/integrations/github_controller.rb b/app/controllers/integrations/github_controller.rb index 93d0545259..d11479e861 100644 --- a/app/controllers/integrations/github_controller.rb +++ b/app/controllers/integrations/github_controller.rb @@ -12,17 +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 + def expire_commit_status commit = params[:sha].to_s CommitStatus.new(project, commit).expire_cache(commit) - project.releases.where(commit: commit).each(&:touch) end def payload diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 8340290c8c..6cd1e8a20c 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -3,8 +3,10 @@ class CommitStatus # See ref_status_typeahead.js for how statuses are handled # See https://developer.github.com/v3/repos/statuses for api details - # fatal is our own state that blocks deploys - STATE_PRIORITY = [:success, :pending, :failure, :error, :fatal].freeze + # - 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 @@ -50,14 +52,18 @@ def merge(a, b) def github_status commit = @project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) - write_if = ->(s) { s.fetch(:statuses).none? { |s| s.fetch(:state) == "pending" } } + write_if = ->(s) { s.fetch(:statuses).none? { |s| UNDETERMINED.include?(s[:state]) } } cache_fetch cache_key(commit), expires_in: 1.hour, write_if: write_if do GITHUB.combined_status(@project.repository_path, commit).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 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/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/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 073f100c89..5a9eaeaab4 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -41,14 +41,14 @@ def status(stage_param: stage, reference_param: reference) status.state.must_equal 'success' end - it "is failure when not found" do + it "is missing when not found" do failure! - status.state.must_equal 'failure' + status.state.must_equal 'missing' end - it "is failure commit is not found" do + it "is missing commit is not found" do GitRepository.any_instance.stubs(:commit_from_ref).returns(nil) - status.state.must_equal 'failure' + status.state.must_equal 'missing' end it "works without stage" do @@ -151,9 +151,9 @@ def status(stage_param: stage, reference_param: reference) 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.statuses.map { |s| s[:state] }.must_equal ["Reference"] + status.statuses.map { |s| s[:context] }.must_equal ["Reference"] end describe "when deploying a previous release" do @@ -169,16 +169,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 - end - describe '#ref_statuses' do let(:production_stage) { stages(:test_production) } 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) } From eac54610d58d23120269da1cd09efcdffee42de8 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Sat, 22 Sep 2018 14:41:20 -0700 Subject: [PATCH 6/7] only cache static references and do not perform expensive pulls for autocomplete --- app/models/commit_status.rb | 10 +++++---- test/models/commit_status_test.rb | 36 +++++++++++++++++++------------ 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6cd1e8a20c..2b0fedc8f8 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -51,10 +51,10 @@ def merge(a, b) end def github_status - commit = @project.repository.commit_from_ref(@reference) || raise(Octokit::NotFound) + static = @reference.match?(Build::SHA1_REGEX) || @reference.match?(Release::VERSION_REGEX) write_if = ->(s) { s.fetch(:statuses).none? { |s| UNDETERMINED.include?(s[:state]) } } - cache_fetch cache_key(commit), expires_in: 1.hour, write_if: write_if do - GITHUB.combined_status(@project.repository_path, commit).to_h + cache_fetch_if static, cache_key(@reference), expires_in: 1.hour, write_if: write_if do + GITHUB.combined_status(@project.repository_path, @reference).to_h end rescue Octokit::NotFound { @@ -71,7 +71,9 @@ def cache_key(commit) ['commit-status', @project.id, commit] end - def cache_fetch(key, options) + def cache_fetch_if(condition, key, options) + return yield unless condition + old = Rails.cache.read(key) return old if old diff --git a/test/models/commit_status_test.rb b/test/models/commit_status_test.rb index 5a9eaeaab4..cb75abb872 100644 --- a/test/models/commit_status_test.rb +++ b/test/models/commit_status_test.rb @@ -31,9 +31,7 @@ def status(stage_param: stage, reference_param: reference) let(:stage) { stages(:test_staging) } let(:reference) { 'master' } - let(:url) { "repos/#{stage.project.repository_path}/commits/abcabcabc/status" } - - before { GitRepository.any_instance.stubs(:commit_from_ref).returns("abcabcabc") } + let(:url) { "repos/#{stage.project.repository_path}/commits/#{reference}/status" } describe "#state" do it "returns state" do @@ -46,11 +44,6 @@ def status(stage_param: stage, reference_param: reference) status.state.must_equal 'missing' end - it "is missing commit is not found" do - GitRepository.any_instance.stubs(:commit_from_ref).returns(nil) - status.state.must_equal 'missing' - end - it "works without stage" do success! s = status @@ -58,7 +51,16 @@ def status(stage_param: stage, reference_param: reference) s.state.must_equal "success" end - describe "caching" do + 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' @@ -69,7 +71,7 @@ def status(stage_param: stage, reference_param: reference) it "can expire cache" do request = success! status.state.must_equal 'success' - status.expire_cache 'abcabcabc' + status.expire_cache reference status.state.must_equal 'success' assert_requested request, times: 2 end @@ -207,19 +209,25 @@ def status(stage_param: stage, reference_param: reference) end end - describe "#cache_fetch" do + describe "#cache_fetch_if" do it "fetches old" do Rails.cache.write('a', 1) - status.send(:cache_fetch, 'a', write_if: :raise) { 2 }.must_equal 1 + status.send(:cache_fetch_if, true, 'a', write_if: :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', write_if: :raise) { 2 }.must_equal 2 + Rails.cache.read('a').must_equal 1 end it "writes new when ok" do - status.send(:cache_fetch, 'a', write_if: ->(_) { true }) { 2 }.must_equal 2 + status.send(:cache_fetch_if, true, 'a', write_if: ->(_) { true }) { 2 }.must_equal 2 Rails.cache.read('a').must_equal 2 end it "does not write new when not ok" do - status.send(:cache_fetch, 'a', write_if: ->(_) { false }) { 2 }.must_equal 2 + status.send(:cache_fetch_if, true, 'a', write_if: ->(_) { false }) { 2 }.must_equal 2 Rails.cache.read('a').must_equal nil end end From 61aa2f6f8ccc77cd0e3a0acaa331cae73a9f616b Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Sun, 23 Sep 2018 13:52:31 -0700 Subject: [PATCH 7/7] add some logic to the amount of time we cache and always cache at least a bit --- app/models/commit_status.rb | 22 +++++++++++--- test/controllers/releases_controller_test.rb | 1 + test/models/commit_status_test.rb | 32 ++++++++++++++------ 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 2b0fedc8f8..644374e326 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -50,10 +50,11 @@ def merge(a, b) a.fetch(:statuses).concat b.fetch(:statuses) end + # NOTE: reply is an api object that does not support .fetch def github_status static = @reference.match?(Build::SHA1_REGEX) || @reference.match?(Release::VERSION_REGEX) - write_if = ->(s) { s.fetch(:statuses).none? { |s| UNDETERMINED.include?(s[:state]) } } - cache_fetch_if static, cache_key(@reference), expires_in: 1.hour, write_if: write_if do + 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 @@ -67,18 +68,31 @@ def github_status } 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, options) + 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, options) if options[:write_if].call(current) + Rails.cache.write(key, current, expires_in: expires_in.call(current)) current 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/models/commit_status_test.rb b/test/models/commit_status_test.rb index cb75abb872..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! @@ -143,7 +143,7 @@ def status(stage_param: stage, reference_param: reference) describe "#statuses" do it "returns list" do success! - status.statuses.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 @@ -163,7 +163,7 @@ def status(stage_param: stage, reference_param: reference) it "merges" do success! - status.statuses.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"} ] @@ -212,23 +212,35 @@ def status(stage_param: stage, reference_param: reference) describe "#cache_fetch_if" do it "fetches old" do Rails.cache.write('a', 1) - status.send(:cache_fetch_if, true, 'a', write_if: :raise) { 2 }.must_equal 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', write_if: :raise) { 2 }.must_equal 2 + status.send(:cache_fetch_if, false, 'a', expires_in: :raise) { 2 }.must_equal 2 Rails.cache.read('a').must_equal 1 end - it "writes new when ok" do - status.send(:cache_fetch_if, true, 'a', write_if: ->(_) { true }) { 2 }.must_equal 2 + 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 "does not write new when not ok" do - status.send(:cache_fetch_if, true, 'a', write_if: ->(_) { false }) { 2 }.must_equal 2 - Rails.cache.read('a').must_equal nil + 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