fix(github_hooks): raise unknown error on unhandled octokit exception (#152)

When we receive an error from Octokit that we are not currently
handling, the `pull_request` method on the repo host client returns nil,
which can lead to a `undefined method `[]' for nil` error on the hook
handler. See [this
comment](renderedtext/tasks#7601 (comment))
for more details. Instead, to help us troubleshoot issues, we should log the actual error
message that we are receiving from GitHub.

Author: lucaspin

github_hooks/Gemfile.lock

@@ -246,7 +246,7 @@ GEM
       nio4r (~> 2.0)
     raabro (1.4.0)
     racc (1.8.1)
-    rack (2.2.12)
+    rack (2.2.13)
     rack-session (1.0.2)
       rack (< 3)
     rack-test (2.1.0)

github_hooks/lib/internal_api/repo_proxy/repo_proxy_server.rb

@@ -51,11 +51,11 @@ class RepoProxyServer < RepoProxyService::Service
         end
       end
 
-      define_rpc :create do |req|
+      define_rpc :create do |req, logger|
         project = ::Project.find(req.project_id)
 
         if project.repository.integration_type.include?("github")
-          create_for_github_project(req)
+          create_for_github_project(req, logger)
         else
           create_via_hooks_api(req)
         end
@@ -66,7 +66,7 @@ def create_via_hooks_api(req)
         client.create(req)
       end
 
-      def create_for_github_project(req)
+      def create_for_github_project(req, logger)
         project = ::Project.find(req.project_id)
         user    = ::User.find(req.requester_id)
 
@@ -155,6 +155,9 @@ def create_for_github_project(req)
       rescue ::RepoHost::RemoteException::NotFound
         workflow.update(:state => Workflow::STATE_NOT_FOUND_REPO)
         raise GRPC::NotFound, "Reference not found on GitHub #{req.git.reference} #{req.git.commit_sha}"
+      rescue ::RepoHost::RemoteException::Unknown => e
+        logger.error("Unknown error", error: e.message)
+        raise GRPC::Unknown, "Unknown error"
       rescue ::ActiveRecord::RecordNotFound => e
         if e.model == User
           raise GRPC::NotFound, "Requester not found #{req.requester_id}"

github_hooks/lib/repo_host/github/client.rb

@@ -244,6 +244,8 @@ def handle_octokit_exceptions(exception)
         raise ::RepoHost::RemoteException::MaximumNumberOfStatuses, exception.message
       elsif hook_exists?(exception)
         raise ::RepoHost::RemoteException::HookExistsOnRepository, exception.message
+      else
+        raise ::RepoHost::RemoteException::Unknown, exception.message
       end
     end
 

github_hooks/lib/repo_host/remote_exception/unknown.rb

@@ -0,0 +1,2 @@
+class RepoHost::RemoteException::Unknown < RepoHost::RemoteException
+end

github_hooks/lib/semaphore/repo_host/hooks/handler.rb

@@ -179,31 +179,35 @@ def self.run(workflow, logger, hook_payload = "", signature = "", retries = 0)
     end
 
     if workflow.payload.pull_request?
-      state, meta, msg = update_pr_data(workflow.project, workflow.pull_request_number, workflow.commit_sha)
-
-      case state
-      when :not_found
-        logger.info("pr-not-found #{msg}")
-        workflow.update(:state => Workflow::STATE_PR_NOT_FOUND)
-
-        return
-      when :non_mergeable
-        update_pull_request_mergeable(workflow, meta[:mergeable])
-
-        logger.info("pr-non-mergeable")
-        workflow.update(:state => Workflow::STATE_PR_NON_MERGEABLE)
-
-        return
-      when :skip_ci
-        logger.info("request-is-filtered")
-        workflow.update(:state => Workflow::STATE_SKIP_CI)
-
-        return
-      when :without_reference
-        logger.info("without-reference")
-        workflow.update(:commit_author => meta[:commit_author])
-      else
-        workflow.update(:commit_author => meta[:commit_author], :commit_sha => meta[:merge_commit_sha], :git_ref => meta[:ref])
+      begin
+        state, meta, msg = update_pr_data(workflow.project, workflow.pull_request_number, workflow.commit_sha)
+        case state
+        when :not_found
+          logger.info("pr-not-found #{msg}")
+          workflow.update(:state => Workflow::STATE_PR_NOT_FOUND)
+
+          return
+        when :non_mergeable
+          update_pull_request_mergeable(workflow, meta[:mergeable])
+
+          logger.info("pr-non-mergeable")
+          workflow.update(:state => Workflow::STATE_PR_NON_MERGEABLE)
+
+          return
+        when :skip_ci
+          logger.info("request-is-filtered")
+          workflow.update(:state => Workflow::STATE_SKIP_CI)
+
+          return
+        when :without_reference
+          logger.info("without-reference")
+          workflow.update(:commit_author => meta[:commit_author])
+        else
+          workflow.update(:commit_author => meta[:commit_author], :commit_sha => meta[:merge_commit_sha], :git_ref => meta[:ref])
+        end
+      rescue RepoHost::RemoteException::Unknown => e
+        logger.error("Unknown error", error: e.message)
+        raise e.class, e.message
       end
     end
 

github_hooks/spec/lib/internal_api/repo_proxy/repo_proxy_server_spec.rb

@@ -217,6 +217,32 @@
     end
   end
 
+  describe "#create" do
+    before "when unknown remote error is raised" do
+      allow(InternalApi::RepoProxy::PayloadFactory).to receive(
+        :create
+      ).and_raise(RepoHost::RemoteException::Unknown, "Oops")
+
+      owner = FactoryBot.create(:user, :github_connection)
+      repository = FactoryBot.create(:repository, :name => "sandbox", :owner => "renderedtext",
+                                                  :integration_type => "github_app")
+      project = FactoryBot.create(:project, :repository => repository)
+      @req = InternalApi::RepoProxy::CreateRequest.new(
+        :project_id => project.id,
+        :requester_id => owner.id,
+        :git => InternalApi::RepoProxy::CreateRequest::Git.new(
+          reference: "refs/pull/123"
+        )
+      )
+    end
+
+    it "returns an unknown error" do
+      expect do
+        server.create(@req, call)
+      end.to raise_error(GRPC::Unknown)
+    end
+  end
+
   describe "#schedule_blocked_hook" do
     context "when the hook is present" do
       before do

github_hooks/spec/lib/repo_host/github/client_spec.rb

@@ -108,6 +108,19 @@
       end
     end
 
+    context "when unhandled octokit exception occurs" do
+      before do
+        allow_any_instance_of(Octokit::Client).to receive(:repositories)
+          .and_raise(Octokit::TooManyLoginAttempts)
+      end
+
+      it "raises semaphore's RepoHost::RemoteException::NotFound" do
+        expect do
+          @client.repositories
+        end.to raise_error(RepoHost::RemoteException::Unknown)
+      end
+    end
+
     context "when repository hook already exists on GitHub" do
       before do
         allow_any_instance_of(Octokit::Client).to receive(:create_hook)

github_hooks/spec/lib/semaphore/repo_host/hooks/handler_spec.rb

@@ -309,6 +309,22 @@
           end
         end
 
+        context "and the pull-request get throws unknown error" do
+          before do
+            allow(repo_host).to receive(:validate_token_presence!)
+            allow(repo_host).to receive(:pull_request).and_raise(::RepoHost::RemoteException::Unknown,
+                                                                 "Oops")
+          end
+
+          it "logs the error and raises again" do
+            expect(@logger).to receive(:error).with("Unknown error", error: "Oops")
+
+            expect do
+              described_class.run(@workflow, @logger)
+            end.to raise_error(RepoHost::RemoteException::Unknown)
+          end
+        end
+
         context "and the pull-request is eventually found, but non mergeable" do
           before do
             allow(repo_host).to receive(:validate_token_presence!)