Shopify / shipit-engine

Deployment coordination
https://shopify.engineering/introducing-shipit
MIT License
1.41k stars 144 forks source link

Shipit appears to miss some check run statuses for a commit (missing pagination?) #1323

Open sjagoe opened 7 months ago

sjagoe commented 7 months ago

We have been encountering a rare issue where check run statuses for a commit don't always update for the final check run status (thus blocking automatic deployment). When this does happen, the Refresh statuses & commits button does not appear to solve the issue.

This has only occurred on repositories with a large number of check runs (e.g. larger matrix test runs). In the past we've brute-forced our way past this by configuring the stack to ignore most check runs.

Checking one of the commits, it looks like the Commit.refresh_check_runs! method will only pull the first page of 30 results.

irb(main):041:0> response = stack.github_api.check_runs(stack.github_repo_name, commit.sha)
=>
{:total_count=>44,
...
irb(main):045:0> response.total_count
=> 44
irb(main):046:0> response.check_runs.length
=> 30

Simply increasing the page size (as seems to have been done for commit statuses) will probably make this much more unlikely to hit, but maybe pagination for both of check_runs and statuses would be useful.

sjagoe commented 7 months ago

I've validated a patch in our own shipit deployment, but being completely new to rails, I an unable to craft a unit test to validate this (specifically how to mock multiple different calls to github_api.last_response with different return values). Here's the patch I have applied locally if somebody would like to pick it up:

diff --git a/app/models/shipit/commit.rb b/app/models/shipit/commit.rb
index a1417cc5b..c86133753 100644
--- a/app/models/shipit/commit.rb
+++ b/app/models/shipit/commit.rb
@@ -168,10 +168,26 @@ module Shipit
       end
     end

-    def refresh_check_runs!
+    def paginated_check_runs
       response = stack.handle_github_redirections do
-        stack.github_api.check_runs(github_repo_name, sha)
+        stack.github_api.check_runs(github_repo_name, sha, per_page: 100)
+      end
+
+      return response if stack.github_api.last_response.rels[:next].nil?
+
+      loop do
+        page = stack.handle_github_redirections do
+          stack.github_api.get(stack.github_api.last_response.rels[:next].href)
+        end
+        response.check_runs.concat(page.check_runs)
+        break if stack.github_api.last_response.rels[:next].nil?
       end
+
+      response
+    end
+
+    def refresh_check_runs!
+      response = paginated_check_runs
       response.check_runs.each do |check_run|
         create_or_update_check_run_from_github!(check_run)
       end
casperisfine commented 7 months ago

if somebody would like to pick it up:

Please just submit a PR.