From 13441bd88ee1e82636f17b47680bc682f4d3eb7e Mon Sep 17 00:00:00 2001 From: ecrupper Date: Thu, 7 Sep 2023 14:42:03 -0500 Subject: [PATCH] patch(sync): address sync bug with update webhook call --- api/repo/update.go | 2 +- api/scm/sync.go | 36 +++++++++++++++---- api/scm/sync_org.go | 34 ++++++++++++++---- scm/github/repo.go | 12 +++---- scm/github/repo_test.go | 76 ++++++++++++++++++++++++++++++++++++++++- scm/service.go | 2 +- 6 files changed, 140 insertions(+), 22 deletions(-) diff --git a/api/repo/update.go b/api/repo/update.go index 374772d42..3ada10316 100644 --- a/api/repo/update.go +++ b/api/repo/update.go @@ -284,7 +284,7 @@ func UpdateRepo(c *gin.Context) { }).Infof("platform admin %s updating repo webhook events for repo %s", admn, r.GetFullName()) } // update webhook with new events - err = scm.FromContext(c).Update(u, r, lastHook.GetWebhookID()) + _, err = scm.FromContext(c).Update(u, r, lastHook.GetWebhookID()) if err != nil { retErr := fmt.Errorf("unable to update repo webhook for %s: %w", r.GetFullName(), err) diff --git a/api/scm/sync.go b/api/scm/sync.go index b160f5fa0..7b4da0f66 100644 --- a/api/scm/sync.go +++ b/api/scm/sync.go @@ -110,8 +110,9 @@ func SyncRepo(c *gin.Context) { return } - // if we have webhook validation, update the repo hook in the SCM - if c.Value("webhookvalidation").(bool) { + // if we have webhook validation and the repo is active in the database, + // update the repo hook in the SCM + if c.Value("webhookvalidation").(bool) && r.GetActive() { // grab last hook from repo to fetch the webhook ID lastHook, err := database.FromContext(c).LastHookForRepo(r) if err != nil { @@ -123,13 +124,36 @@ func SyncRepo(c *gin.Context) { } // update webhook - err = scm.FromContext(c).Update(u, r, lastHook.GetWebhookID()) + webhookExists, err := scm.FromContext(c).Update(u, r, lastHook.GetWebhookID()) if err != nil { - retErr := fmt.Errorf("unable to update repo webhook for %s: %w", r.GetFullName(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) + // if webhook has been manually deleted from GitHub, + // set to inactive in database + if !webhookExists { - return + r.SetActive(false) + + err := database.FromContext(c).UpdateRepo(r) + if err != nil { + retErr := fmt.Errorf("unable to update repo for org %s: %w", o, err) + + util.HandleError(c, http.StatusInternalServerError, retErr) + + return + } + + c.JSON(http.StatusOK, fmt.Sprintf("webhook not found, repo %s deactivated", r.GetFullName())) + + return + + } else { + + retErr := fmt.Errorf("unable to update repo webhook for %s: %w", r.GetFullName(), err) + + util.HandleError(c, http.StatusInternalServerError, retErr) + + return + } } } diff --git a/api/scm/sync_org.go b/api/scm/sync_org.go index ac18ec551..e8f3e0495 100644 --- a/api/scm/sync_org.go +++ b/api/scm/sync_org.go @@ -123,8 +123,9 @@ func SyncReposForOrg(c *gin.Context) { } } - // if we have webhook validation, update the repo hook in the SCM - if c.Value("webhookvalidation").(bool) { + // if we have webhook validation and the repo is active in the database, + // update the repo hook in the SCM + if c.Value("webhookvalidation").(bool) && repo.GetActive() { // grab last hook from repo to fetch the webhook ID lastHook, err := database.FromContext(c).LastHookForRepo(repo) if err != nil { @@ -136,13 +137,34 @@ func SyncReposForOrg(c *gin.Context) { } // update webhook - err = scm.FromContext(c).Update(u, repo, lastHook.GetWebhookID()) + webhookExists, err := scm.FromContext(c).Update(u, repo, lastHook.GetWebhookID()) if err != nil { - retErr := fmt.Errorf("unable to update repo webhook for %s: %w", repo.GetFullName(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) + // if webhook has been manually deleted from GitHub, + // set to inactive in database + if !webhookExists { - return + repo.SetActive(false) + + err := database.FromContext(c).UpdateRepo(repo) + if err != nil { + retErr := fmt.Errorf("unable to update repo for org %s: %w", o, err) + + util.HandleError(c, http.StatusInternalServerError, retErr) + + return + } + + continue + + } else { + + retErr := fmt.Errorf("unable to update repo webhook for %s: %w", repo.GetFullName(), err) + + util.HandleError(c, http.StatusInternalServerError, retErr) + + return + } } } } diff --git a/scm/github/repo.go b/scm/github/repo.go index 682a2db94..723213cce 100644 --- a/scm/github/repo.go +++ b/scm/github/repo.go @@ -217,7 +217,7 @@ func (c *client) Enable(u *library.User, r *library.Repo, h *library.Hook) (*lib } // Update edits a repo webhook. -func (c *client) Update(u *library.User, r *library.Repo, hookID int64) error { +func (c *client) Update(u *library.User, r *library.Repo, hookID int64) (bool, error) { c.Logger.WithFields(logrus.Fields{ "org": r.GetOrg(), "repo": r.GetName(), @@ -258,13 +258,11 @@ func (c *client) Update(u *library.User, r *library.Repo, hookID int64) error { } // send API call to update the webhook - _, _, err := client.Repositories.EditHook(ctx, r.GetOrg(), r.GetName(), hookID, hook) + _, resp, err := client.Repositories.EditHook(ctx, r.GetOrg(), r.GetName(), hookID, hook) - if err != nil { - return err - } - - return nil + // track if webhook exists in GitHub; a missing webhook + // indicates the webhook has been manually deleted from GitHub + return resp.StatusCode != http.StatusNotFound, err } // Status sends the commit status for the given SHA from the GitHub repo. diff --git a/scm/github/repo_test.go b/scm/github/repo_test.go index 7482c71ce..1a77c9e4c 100644 --- a/scm/github/repo_test.go +++ b/scm/github/repo_test.go @@ -673,7 +673,7 @@ func TestGithub_Update(t *testing.T) { client, _ := NewTest(s.URL) // run test - err := client.Update(u, r, hookID) + _, err := client.Update(u, r, hookID) if resp.Code != http.StatusOK { t.Errorf("Update returned %v, want %v", resp.Code, http.StatusOK) @@ -684,6 +684,80 @@ func TestGithub_Update(t *testing.T) { } } +func TestGithub_Update_webhookExists_True(t *testing.T) { + // setup context + gin.SetMode(gin.TestMode) + + resp := httptest.NewRecorder() + _, engine := gin.CreateTestContext(resp) + + // setup mock server + engine.PATCH("/api/v3/repos/:org/:repo/hooks/:hook_id", func(c *gin.Context) { + c.Header("Content-Type", "application/json") + c.Status(http.StatusOK) + }) + + s := httptest.NewServer(engine) + defer s.Close() + + // setup types + u := new(library.User) + u.SetName("foo") + u.SetToken("bar") + + r := new(library.Repo) + + client, _ := NewTest(s.URL) + + // run test + webhookExists, err := client.Update(u, r, 0) + + if !webhookExists { + t.Errorf("Update returned %v, want %v", webhookExists, true) + } + + if err != nil { + t.Errorf("Update returned err: %v", err) + } +} + +func TestGithub_Update_webhookExists_False(t *testing.T) { + // setup context + gin.SetMode(gin.TestMode) + + resp := httptest.NewRecorder() + _, engine := gin.CreateTestContext(resp) + + // setup mock server + engine.PATCH("/api/v3/repos/:org/:repo/hooks/:hook_id", func(c *gin.Context) { + c.Header("Content-Type", "application/json") + c.Status(http.StatusNotFound) + }) + + s := httptest.NewServer(engine) + defer s.Close() + + // setup types + u := new(library.User) + u.SetName("foo") + u.SetToken("bar") + + r := new(library.Repo) + + client, _ := NewTest(s.URL) + + // run test + webhookExists, err := client.Update(u, r, 0) + + if webhookExists { + t.Errorf("Update returned %v, want %v", webhookExists, false) + } + + if err == nil { + t.Error("Update should return error") + } +} + func TestGithub_Status_Deployment(t *testing.T) { // setup context gin.SetMode(gin.TestMode) diff --git a/scm/service.go b/scm/service.go index c92cbedbf..df90162f8 100644 --- a/scm/service.go +++ b/scm/service.go @@ -101,7 +101,7 @@ type Service interface { Enable(*library.User, *library.Repo, *library.Hook) (*library.Hook, string, error) // Update defines a function that updates // a webhook for a specified repo. - Update(*library.User, *library.Repo, int64) error + Update(*library.User, *library.Repo, int64) (bool, error) // Status defines a function that sends the // commit status for the given SHA from a repo. Status(*library.User, *library.Build, string, string) error