Merge pull request #98 from RodrigoMNardi/bug/github/check_run/failed_to_update_ci

GitHub CI Sync

Author: RodrigoMNardi

.simplecov

@@ -11,7 +11,7 @@
 SimpleCov.start do
   enable_coverage :branch
   primary_coverage :branch
-  add_filter %r{^/spec/}
+  add_filter %r{^/(spec|config)/}
   add_filter 'database_loader.rb'
   add_group 'Models', 'lib/models'
   add_group 'GitHub Functions', 'lib/github'

config/delayed_job.rb

@@ -20,7 +20,7 @@ class << self
   end
 end
 
-DELAYED_JOB_TIMER = 5
+DELAYED_JOB_TIMER = 30
 
 Rails.logger = GithubLogger.instance.create('delayed_job.log', Logger::INFO)
 ActiveRecord::Base.logger = GithubLogger.instance.create('delayed_job.log', Logger::INFO)
@@ -34,5 +34,9 @@ class << self
 Delayed::Worker.max_attempts = 5
 Delayed::Worker.max_run_time = 5.minutes
 
+Delayed::Job.delete_all unless ENV.fetch('RAILS_ENV', 'test') == 'test'
+
+# Load the database configuration
+
 config = YAML.load_file('config/database.yml')[ENV.fetch('RACK_ENV', 'development')]
 ActiveRecord::Base.establish_connection(config)

lib/github/build/summary.rb

@@ -34,7 +34,6 @@ def initialize(job, logger_level: Logger::INFO, agent: 'Github')
       def build_summary
         current_stage = @job.stage
         current_stage = fetch_parent_stage if current_stage.nil?
-        current_stage.refresh_reference(@github)
 
         logger(Logger::INFO, "build_summary: #{current_stage.inspect}")
         msg = "Github::Build::Summary - #{@job.inspect}, #{current_stage.inspect}, bamboo info: #{bamboo_info}"
@@ -81,8 +80,6 @@ def must_cancel_next_stages(current_stage)
           .where(check_suite: @check_suite)
           .where(configuration: { position: [(current_stage.configuration.position + 1)..] })
           .each do |stage|
-          next if stage.cancelled?
-
           cancelling_next_stage(stage)
         end
       end
@@ -98,7 +95,7 @@ def must_continue_next_stage(current_stage)
           .where(configuration: { position: current_stage.configuration.position + 1 })
           .first
 
-        return if next_stage.nil? or next_stage.cancelled?
+        return if next_stage.nil? or next_stage.finished?
 
         update_summary(next_stage)
       end

lib/github/check.rb

@@ -138,25 +138,29 @@ def basic_status(check_ref, status, output)
     def completed(check_ref, status, conclusion, output)
       return if check_ref.nil?
 
-      opts = {
-        status: status,
-        conclusion: conclusion
-      }
+      retry_count = 0
 
-      opts[:output] = output unless output.empty?
+      begin
+        opts = {
+          status: status,
+          conclusion: conclusion
 
-      resp =
-        @app.update_check_run(
-          @check_suite.pull_request.repository,
-          check_ref,
-          opts
-        ).to_h
+        }
 
-      @logger.info("completed: #{check_ref}, status: #{status}, conclusion: #{conclusion} -> resp: #{resp}")
+        opts[:output] = output unless output.empty?
 
-      resp
-    rescue Octokit::NotFound
-      @logger.error "#{check_ref} not found at GitHub"
+        send_update(check_ref, opts, conclusion)
+      rescue Octokit::NotFound, RuntimeError
+        retry_count += 1
+
+        sleep retry_count * 5
+
+        retry if retry_count <= 3
+
+        @logger.error "#{check_ref} not found at GitHub"
+
+        {}
+      end
     end
 
     def authenticate_app
@@ -190,5 +194,20 @@ def authenticate(jwt)
 
       @app = Octokit::Client.new(bearer_token: token)
     end
+
+    def send_update(check_ref, opts, conclusion)
+      resp =
+        @app.update_check_run(
+          @check_suite.pull_request.repository,
+          check_ref,
+          opts
+        ).to_h
+
+      raise 'GitHub failed to update status' if resp[:conclusion] != conclusion
+
+      @logger.info("completed: #{check_ref}, conclusion: #{conclusion} -> resp: #{resp}")
+
+      resp
+    end
   end
 end

lib/github/update_status.rb

@@ -82,26 +82,17 @@ def update_status
     def insert_new_delayed_job
       queue = @job.check_suite.pull_request.github_pr_id % 10
 
-      if can_add_new_job?
-        return CiJobStatus
-               .delay(run_at: DELAYED_JOB_TIMER.seconds.from_now, queue: queue)
-               .update(@job.check_suite.id, @job.id)
-      end
-
       delete_and_create_delayed_job(queue)
     end
 
     def delete_and_create_delayed_job(queue)
-      fetch_delayed_job.destroy_all
+      fetch_delayed_job&.destroy_all
+
       CiJobStatus
         .delay(run_at: DELAYED_JOB_TIMER.seconds.from_now, queue: queue)
         .update(@job.check_suite.id, @job.id)
     end
 
-    def can_add_new_job?
-      fetch_delayed_job.empty?
-    end
-
     def fetch_delayed_job
       Delayed::Job.where('handler LIKE ?', "%method_name: :update\nargs:\n- #{@job.check_suite.id}%")
     end

lib/models/stage.rb

@@ -55,43 +55,39 @@ def update_output(github, output: output_in_progress)
   end
 
   def cancelled(github, output: {}, agent: 'Github')
-    return if cancelled?
+    check_run = refresh_reference(github)
+    github.cancelled(check_run.id, output)
 
-    create_github_check(github)
-    github.cancelled(check_ref, output)
-    update(status: :cancelled)
+    update(status: :cancelled, check_ref: check_run.id)
     AuditStatus.create(auditable: self, status: :cancelled, agent: agent, created_at: Time.now)
     notification
   end
 
   def failure(github, output: {}, agent: 'Github')
-    return if failure?
+    check_run = refresh_reference(github)
+    github.failure(check_run.id, output)
 
-    create_github_check(github)
-    github.failure(check_ref, output)
-    update(status: :failure)
+    update(status: :failure, check_ref: check_run.id)
     AuditStatus.create(auditable: self, status: :failure, agent: agent, created_at: Time.now)
     notification
   end
 
   def success(github, output: {}, agent: 'Github')
-    return if success?
+    check_run = refresh_reference(github)
+    github.success(check_run.id, output)
 
-    create_github_check(github)
-    github.success(check_ref, output)
-    update(status: :success)
+    reload
+    update(status: :success, check_ref: check_run.id)
     AuditStatus.create(auditable: self, status: :success, agent: agent, created_at: Time.now)
     notification
   end
 
-  def refresh_reference(github, agent: 'Github')
-    check_run = github.create(github_stage_full_name(name))
-    update(check_ref: check_run.id)
-    AuditStatus.create(auditable: self, status: :refresh, agent: agent, created_at: Time.now)
-  end
-
   private
 
+  def refresh_reference(github)
+    github.create(github_stage_full_name(name))
+  end
+
   def in_progress_notification
     SlackBot.instance.stage_in_progress_notification(self)
   end

spec/lib/github/build/unavailable_jobs_spec.rb

@@ -19,7 +19,7 @@
     allow(Octokit::Client).to receive(:new).and_return(fake_client)
     allow(fake_client).to receive(:find_app_installations).and_return([{ 'id' => 1 }])
     allow(fake_client).to receive(:create_app_installation_access_token).and_return({ 'token' => 1 })
-    allow(fake_client).to receive(:update_check_run)
+    allow(fake_client).to receive(:update_check_run).and_return({ conclusion: 'skipped' })
     allow(File).to receive(:read).and_return('')
     allow(OpenSSL::PKey::RSA).to receive(:new).and_return(OpenSSL::PKey::RSA.new(2048))
   end

spec/lib/github/check_spec.rb

@@ -104,7 +104,7 @@
   context 'when call create' do
     let(:pr_id) { 1 }
     let(:name) { 'test' }
-    let(:pr_info) { { name: name } }
+    let(:pr_info) { { name: name, conclusion: 'success' } }
 
     before do
       allow(fake_client).to receive(:create_check_run)
@@ -141,7 +141,7 @@
   context 'when call in_progress' do
     let(:id) { 1 }
     let(:status) { 'in_progress' }
-    let(:pr_info) { { status: status } }
+    let(:pr_info) { { status: status, conclusion: status } }
     let(:output) { { title: 'Title', summary: 'Summary' } }
 
     before do
@@ -173,11 +173,11 @@
                 status: status,
                 conclusion: conclusion
               })
-        .and_return({})
+        .and_return({ conclusion: conclusion })
     end
 
     it 'must returns success' do
-      expect(check.cancelled(id)).to eq({})
+      expect(check.cancelled(id)).to eq({ conclusion: conclusion })
     end
   end
 
@@ -194,11 +194,11 @@
                 status: status,
                 conclusion: conclusion
               })
-        .and_return({})
+        .and_return({ conclusion: conclusion })
     end
 
     it 'must returns success' do
-      expect(check.success(id)).to eq({})
+      expect(check.success(id)).to eq({ conclusion: conclusion })
     end
   end
 
@@ -238,11 +238,11 @@
                 conclusion: conclusion,
                 output: output
               })
-        .and_return({})
+        .and_return({ conclusion: conclusion })
     end
 
     it 'must returns success' do
-      expect(check.failure(id, output)).to eq({})
+      expect(check.failure(id, output)).to eq({ conclusion: conclusion })
     end
   end
 
@@ -259,11 +259,11 @@
                 status: status,
                 conclusion: conclusion
               })
-        .and_return({})
+        .and_return({ conclusion: conclusion })
     end
 
     it 'must returns success' do
-      expect(check.skipped(id)).to eq({})
+      expect(check.skipped(id)).to eq({ conclusion: conclusion })
     end
   end
 
@@ -350,4 +350,30 @@
       end
     end
   end
+
+  describe 'retry send_update ' do
+    let(:id) { 1 }
+    let(:status) { 'completed' }
+    let(:conclusion) { 'success' }
+
+    context 'when send_update returns error' do
+      before do
+        allow(fake_client).to receive(:update_check_run).and_raise(Octokit::NotFound)
+      end
+
+      it 'must returns raise' do
+        expect(check.skipped(id)).to eq({})
+      end
+    end
+
+    context 'when send_update returns error and success' do
+      before do
+        allow(fake_client).to receive(:update_check_run).and_return({}, { conclusion: conclusion })
+      end
+
+      it 'must returns a valid data' do
+        expect(check.skipped(id)).to eq({})
+      end
+    end
+  end
 end

spec/spec_helper.rb

@@ -61,6 +61,7 @@ def app
   end
 
   config.before(:each) do
+    allow_any_instance_of(Object).to receive(:sleep)
     Delayed::Worker.delay_jobs = false
   end