Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch(sync): address sync bug with update webhook call #962

Merged
merged 1 commit into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/repo/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
36 changes: 30 additions & 6 deletions api/scm/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
}
}

Expand Down
34 changes: 28 additions & 6 deletions api/scm/sync_org.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
}
}
}
Expand Down
12 changes: 5 additions & 7 deletions scm/github/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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.
Expand Down
76 changes: 75 additions & 1 deletion scm/github/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion scm/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down