fix(hooks-processor): Don't retry processing of hooks of unsupported type (#413)

## 📝 Description

If the webhook received from the git provider is of an unsupported type
(e.g. merge requests on GitLab), we should not retry
processing since that is not a transient issue.

## ✅ Checklist
- [x] I have tested this change
- [x] ~This change requires documentation update~ -N/A

Co-authored-by: Amir Hasanbasic <[email protected]>

Author: DamjanBecirovic

hooks_processor/lib/hooks_processor/hooks/processing/bitbucket_worker.ex

@@ -93,11 +93,14 @@ defmodule HooksProcessor.Hooks.Processing.BitbucketWorker do
     e -> e
   end
 
-  defp process_webhook(hook_type, _webhook, _project, _requester_id) do
+  defp process_webhook(hook_type, webhook, _project, requester_id) do
+    params = %{provider: "bitbucket", requester_id: requester_id}
+    HooksQueries.update_webhook(webhook, params, "failed", "BAD REQUEST")
+
     # Increment unsupported hook type metric
     Watchman.increment({"hooks.processing.bitbucket", ["unsupported_hook"]})
 
-    "Unsuported type of the hook: '#{hook_type}'"
+    {:error, "Unsuported type of the hook: '#{hook_type}'"}
   end
 
   defp perform_actions(webhook, parsed_data, hook_type, action_type)

hooks_processor/lib/hooks_processor/hooks/processing/git_worker.ex

@@ -136,11 +136,11 @@ defmodule HooksProcessor.Hooks.Processing.GitWorker do
     e -> e
   end
 
-  defp process_webhook(hook_type, webhook, _project, _requester_id) do
-    webhook
-    |> LT.warn("Unsupported type of the hook: '#{hook_type}'")
+  defp process_webhook(hook_type, webhook, _project, requester_id) do
+    params = %{provider: "git", requester_id: requester_id}
+    HooksQueries.update_webhook(webhook, params, "failed", "BAD REQUEST")
 
-    HooksQueries.update_webhook(webhook, %{}, "failed")
+    {:error, "Unsupported type of the hook: '#{hook_type}' for webhook: #{inspect(webhook)}"}
   end
 
   defp perform_actions(webhook, parsed_data) do

hooks_processor/lib/hooks_processor/hooks/processing/gitlab_worker.ex

@@ -95,8 +95,11 @@ defmodule HooksProcessor.Hooks.Processing.GitlabWorker do
     e -> e
   end
 
-  defp process_webhook(hook_type, _webhook, _project, _requester_id) do
-    "Unsuported type of the hook: '#{hook_type}'"
+  defp process_webhook(hook_type, webhook, _project, requester_id) do
+    params = %{provider: "gitlab", requester_id: requester_id}
+    HooksQueries.update_webhook(webhook, params, "failed", "BAD REQUEST")
+
+    {:error, "Unsuported type of the hook: '#{hook_type}'"}
   end
 
   defp should_build?(repository, hook_data, hook_type) do

hooks_processor/test/hooks/processing/bitbucket_worker_test.exs

@@ -701,4 +701,60 @@ defmodule HooksProcessor.Hooks.Processing.BitbucketWorkerTest do
 
     GrpcMock.verify!(ProjectHubServiceMock)
   end
+
+  test "unsupported hook type => hook is recorded as failed" do
+    params = %{
+      received_at: DateTime.utc_now(),
+      webhook: BitbucketHooks.pull_request_open(),
+      repository_id: UUID.uuid4(),
+      project_id: UUID.uuid4(),
+      organization_id: UUID.uuid4(),
+      provider: "bitbucket"
+    }
+
+    assert {:ok, webhook} = HooksQueries.insert(params)
+
+    # setup mocks
+
+    ProjectHubServiceMock
+    |> GrpcMock.expect(:describe, fn req, _ ->
+      assert req.id == webhook.project_id
+
+      %Projecthub.DescribeResponse{
+        project: %{
+          metadata: %{
+            id: req.id,
+            org_id: UUID.uuid4()
+          },
+          spec: %{
+            repository: %{
+              pipeline_file: ".semaphore/semaphore.yml",
+              run_on: [:BRANCHES, :TAGS],
+              whitelist: %{tags: ["/v1.*/", "/release-.*/"]}
+            }
+          }
+        },
+        metadata: %{status: %{code: :OK}}
+      }
+    end)
+
+    # wait for worker to finish and check results
+
+    assert {:ok, pid} = WorkersSupervisor.start_worker_for_webhook(webhook.id)
+
+    Test.Helpers.wait_for_worker_to_finish(pid, 15_000)
+
+    assert {:ok, webhook} = HooksQueries.get_by_id(webhook.id)
+    assert webhook.provider == "bitbucket"
+    assert webhook.state == "failed"
+    assert webhook.result == "BAD REQUEST"
+    assert webhook.wf_id == nil
+    assert webhook.ppl_id == nil
+    assert webhook.branch_id == nil
+    assert webhook.commit_sha == nil
+    assert webhook.commit_author == nil
+    assert webhook.git_ref == nil
+
+    GrpcMock.verify!(ProjectHubServiceMock)
+  end
 end

hooks_processor/test/hooks/processing/git_worker_test.exs

@@ -503,4 +503,61 @@ defmodule HooksProcessor.Hooks.Processing.GitWorkerTest do
 
     GrpcMock.verify!(ProjectHubServiceMock)
   end
+
+  test "unsupported hook type => hook is recorded as failed" do
+    params = %{
+      received_at: DateTime.utc_now(),
+      webhook: GitHooks.unsupported_hook_type(),
+      repository_id: UUID.uuid4(),
+      project_id: UUID.uuid4(),
+      organization_id: UUID.uuid4(),
+      provider: "git"
+    }
+
+    assert {:ok, webhook} = HooksQueries.insert(params)
+
+    # setup mocks
+
+    ProjectHubServiceMock
+    |> GrpcMock.expect(:describe, fn req, _ ->
+      assert req.id == webhook.project_id
+
+      %Projecthub.DescribeResponse{
+        project: %{
+          metadata: %{
+            id: req.id,
+            org_id: UUID.uuid4()
+          },
+          spec: %{
+            repository: %{
+              owner: "semaphore",
+              name: "elixir-project",
+              pipeline_file: ".semaphore/semaphore.yml",
+              run_on: [:BRANCHES, :TAGS],
+              whitelist: %{tags: ["/release-.*/"]}
+            }
+          }
+        },
+        metadata: %{status: %{code: :OK}}
+      }
+    end)
+
+    # wait for worker to finish and check results
+
+    assert {:ok, pid} = WorkersSupervisor.start_worker_for_webhook(webhook.id)
+
+    Test.Helpers.wait_for_worker_to_finish(pid, 15_000)
+
+    assert {:ok, webhook} = HooksQueries.get_by_id(webhook.id)
+    assert webhook.state == "failed"
+    assert webhook.result == "BAD REQUEST"
+    assert webhook.wf_id == nil
+    assert webhook.ppl_id == nil
+    assert webhook.branch_id == nil
+    assert webhook.commit_sha == nil
+    assert webhook.commit_author == nil
+    assert webhook.git_ref == nil
+
+    GrpcMock.verify!(ProjectHubServiceMock)
+  end
 end

hooks_processor/test/hooks/processing/gitlab_worker_test.exs

@@ -701,4 +701,59 @@ defmodule HooksProcessor.Hooks.Processing.GitlabWorkerTest do
 
     GrpcMock.verify!(ProjectHubServiceMock)
   end
+
+  test "unsupported hook type => hook is recorded as failed" do
+    params = %{
+      received_at: DateTime.utc_now(),
+      webhook: GitlabHooks.merge_request_open(),
+      repository_id: UUID.uuid4(),
+      project_id: UUID.uuid4(),
+      organization_id: UUID.uuid4(),
+      provider: "gitlab"
+    }
+
+    assert {:ok, webhook} = HooksQueries.insert(params)
+
+    # setup mocks
+
+    ProjectHubServiceMock
+    |> GrpcMock.expect(:describe, fn req, _ ->
+      assert req.id == webhook.project_id
+
+      %Projecthub.DescribeResponse{
+        project: %{
+          metadata: %{
+            id: req.id,
+            org_id: UUID.uuid4()
+          },
+          spec: %{
+            repository: %{
+              pipeline_file: ".semaphore/semaphore.yml",
+              run_on: [:BRANCHES, :TAGS],
+              whitelist: %{tags: ["/release-.*/"]}
+            }
+          }
+        },
+        metadata: %{status: %{code: :OK}}
+      }
+    end)
+
+    # wait for worker to finish and check results
+
+    assert {:ok, pid} = WorkersSupervisor.start_worker_for_webhook(webhook.id)
+
+    Test.Helpers.wait_for_worker_to_finish(pid, 15_000)
+
+    assert {:ok, webhook} = HooksQueries.get_by_id(webhook.id)
+    assert webhook.state == "failed"
+    assert webhook.result == "BAD REQUEST"
+    assert webhook.wf_id == nil
+    assert webhook.ppl_id == nil
+    assert webhook.branch_id == nil
+    assert webhook.commit_sha == nil
+    assert webhook.commit_author == nil
+    assert webhook.git_ref == nil
+
+    GrpcMock.verify!(ProjectHubServiceMock)
+  end
 end

hooks_processor/test/support/git_hooks.ex

@@ -49,4 +49,18 @@ defmodule Support.GitHooks do
       }
     }
   end
+
+  def unsupported_hook_type do
+    %{
+      "reference" => "refs/puls/123",
+      "commit" => %{
+        "sha" => "023becf74ae8a5d93911db4bad7967f94343b44b",
+        "message" => "Initial commit"
+      },
+      "author" => %{
+        "name" => "Radek",
+        "email" => "[email protected]"
+      }
+    }
+  end
 end