Skip to content

Commit

Permalink
fix(webhook): repo transfer handling with renaming - support newer Gi…
Browse files Browse the repository at this point in the history
…tHub versions (#955)

* fix(webhook): repo transfer handling with renaming - support newer GitHub versions

* integration test

* change func name and add comment in integration tests

* sigh

* find and replace woes

---------

Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com>
  • Loading branch information
ecrupper and wass3rw3rk authored Sep 7, 2023
1 parent e9f0808 commit ccc46bf
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 35 deletions.
27 changes: 12 additions & 15 deletions api/webhook/post.go
Original file line number Diff line number Diff line change
Expand Up @@ -778,19 +778,16 @@ func handleRepositoryEvent(ctx context.Context, c *gin.Context, m *types.Metadat
func renameRepository(ctx context.Context, h *library.Hook, r *library.Repo, c *gin.Context, m *types.Metadata) (*library.Repo, error) {
logrus.Infof("renaming repository from %s to %s", r.GetPreviousName(), r.GetName())

// get the old name of the repo
prevOrg, prevRepo := util.SplitFullName(r.GetPreviousName())

// get the repo from the database that matches the old name
dbR, err := database.FromContext(c).GetRepoForOrg(ctx, prevOrg, prevRepo)
// get any matching hook with the repo's unique webhook ID in the SCM
hook, err := database.FromContext(c).GetHookByWebhookID(ctx, h.GetWebhookID())
if err != nil {
retErr := fmt.Errorf("%s: failed to get repo %s/%s from database", baseErr, prevOrg, prevRepo)
util.HandleError(c, http.StatusBadRequest, retErr)

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())
return nil, fmt.Errorf("%s: failed to get hook with webhook ID %d from database", baseErr, h.GetWebhookID())
}

return nil, retErr
// get the repo from the database using repo id of matching hook
dbR, err := database.FromContext(c).GetRepo(ctx, hook.GetRepoID())
if err != nil {
return nil, fmt.Errorf("%s: failed to get repo %d from database", baseErr, hook.GetRepoID())
}

// update hook object which will be added to DB upon reaching deferred function in PostWebhook
Expand Down Expand Up @@ -818,7 +815,7 @@ func renameRepository(ctx context.Context, h *library.Hook, r *library.Repo, c *
// get total number of secrets associated with repository
t, err := database.FromContext(c).CountSecretsForRepo(dbR, map[string]interface{}{})
if err != nil {
return nil, fmt.Errorf("unable to get secret count for repo %s/%s: %w", prevOrg, prevRepo, err)
return nil, fmt.Errorf("unable to get secret count for repo %s/%s: %w", dbR.GetOrg(), dbR.GetName(), err)
}

secrets := []*library.Secret{}
Expand All @@ -827,7 +824,7 @@ func renameRepository(ctx context.Context, h *library.Hook, r *library.Repo, c *
for repoSecrets := int64(0); repoSecrets < t; repoSecrets += 100 {
s, _, err := database.FromContext(c).ListSecretsForRepo(dbR, map[string]interface{}{}, page, 100)
if err != nil {
return nil, fmt.Errorf("unable to get secret list for repo %s/%s: %w", prevOrg, prevRepo, err)
return nil, fmt.Errorf("unable to get secret list for repo %s/%s: %w", dbR.GetOrg(), dbR.GetName(), err)
}

secrets = append(secrets, s...)
Expand All @@ -842,7 +839,7 @@ func renameRepository(ctx context.Context, h *library.Hook, r *library.Repo, c *

_, err = database.FromContext(c).UpdateSecret(secret)
if err != nil {
return nil, fmt.Errorf("unable to update secret for repo %s/%s: %w", prevOrg, prevRepo, err)
return nil, fmt.Errorf("unable to update secret for repo %s/%s: %w", dbR.GetOrg(), dbR.GetName(), err)
}
}

Expand Down Expand Up @@ -889,7 +886,7 @@ func renameRepository(ctx context.Context, h *library.Hook, r *library.Repo, c *
// update the repo in the database
dbR, err = database.FromContext(c).UpdateRepo(ctx, dbR)
if err != nil {
retErr := fmt.Errorf("%s: failed to update repo %s/%s in database", baseErr, prevOrg, prevRepo)
retErr := fmt.Errorf("%s: failed to update repo %s/%s in database", baseErr, dbR.GetOrg(), dbR.GetName())
util.HandleError(c, http.StatusBadRequest, retErr)

h.SetStatus(constants.StatusFailure)
Expand Down
36 changes: 36 additions & 0 deletions database/hook/get_webhook.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) 2023 Target Brands, Inc. All rights reserved.
//
// Use of this source code is governed by the LICENSE file in this repository.

package hook

import (
"context"

"github.com/go-vela/types/constants"
"github.com/go-vela/types/database"
"github.com/go-vela/types/library"
)

// GetHookByWebhookID gets a single hook with a matching webhook id in the database.
func (e *engine) GetHookByWebhookID(ctx context.Context, webhookID int64) (*library.Hook, error) {
e.logger.Tracef("getting a hook with webhook id %d from the database", webhookID)

// variable to store query results
h := new(database.Hook)

// send query to the database and store result in variable
err := e.client.
Table(constants.TableHook).
Where("webhook_id = ?", webhookID).
Take(h).
Error
if err != nil {
return nil, err
}

// return the hook
//
// https://pkg.go.dev/github.com/go-vela/types/database#Hook.ToLibrary
return h.ToLibrary(), nil
}
88 changes: 88 additions & 0 deletions database/hook/get_webhook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright (c) 2023 Target Brands, Inc. All rights reserved.
//
// Use of this source code is governed by the LICENSE file in this repository.

package hook

import (
"context"
"reflect"
"testing"

"github.com/DATA-DOG/go-sqlmock"
"github.com/go-vela/types/library"
)

func TestHook_Engine_GetHookByWebhookID(t *testing.T) {
// setup types
_hook := testHook()
_hook.SetID(1)
_hook.SetRepoID(1)
_hook.SetBuildID(1)
_hook.SetNumber(1)
_hook.SetSourceID("c8da1302-07d6-11ea-882f-4893bca275b8")
_hook.SetWebhookID(123456)

_postgres, _mock := testPostgres(t)
defer func() { _sql, _ := _postgres.client.DB(); _sql.Close() }()

// create expected result in mock
_rows := sqlmock.NewRows(
[]string{"id", "repo_id", "build_id", "number", "source_id", "created", "host", "event", "event_action", "branch", "error", "status", "link", "webhook_id"},
).AddRow(1, 1, 1, 1, "c8da1302-07d6-11ea-882f-4893bca275b8", 0, "", "", "", "", "", "", "", 123456)

// ensure the mock expects the query
_mock.ExpectQuery(`SELECT * FROM "hooks" WHERE webhook_id = $1 LIMIT 1`).WithArgs(123456).WillReturnRows(_rows)

_sqlite := testSqlite(t)
defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }()

_, err := _sqlite.CreateHook(context.TODO(), _hook)
if err != nil {
t.Errorf("unable to create test hook for sqlite: %v", err)
}

// setup tests
tests := []struct {
failure bool
name string
database *engine
want *library.Hook
}{
{
failure: false,
name: "postgres",
database: _postgres,
want: _hook,
},
{
failure: false,
name: "sqlite3",
database: _sqlite,
want: _hook,
},
}

// run tests
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got, err := test.database.GetHookByWebhookID(context.TODO(), 123456)

if test.failure {
if err == nil {
t.Errorf("GetHookByWebhookID for %s should have returned err", test.name)
}

return
}

if err != nil {
t.Errorf("GetHookByWebhookID for %s returned err: %v", test.name, err)
}

if !reflect.DeepEqual(got, test.want) {
t.Errorf("GetHookByWebhookID for %s is %v, want %v", test.name, got, test.want)
}
})
}
}
2 changes: 2 additions & 0 deletions database/hook/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type HookInterface interface {
DeleteHook(context.Context, *library.Hook) error
// GetHook defines a function that gets a hook by ID.
GetHook(context.Context, int64) (*library.Hook, error)
// GetHookByWebhookID defines a function that gets any hook with a matching webhook_id.
GetHookByWebhookID(context.Context, int64) (*library.Hook, error)
// GetHookForRepo defines a function that gets a hook by repo ID and number.
GetHookForRepo(context.Context, *library.Repo, int) (*library.Hook, error)
// LastHookForRepo defines a function that gets the last hook by repo ID.
Expand Down
31 changes: 29 additions & 2 deletions database/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ func testHooks(t *testing.T, db Interface, resources *Resources) {
if err != nil {
t.Errorf("unable to list hooks for repo %d: %v", resources.Repos[0].GetID(), err)
}
if int(count) != len(resources.Hooks) {
// only 2 of 3 hooks belong to Repos[0] repo
if int(count) != len(resources.Hooks)-1 {
t.Errorf("ListHooksForRepo() is %v, want %v", count, len(resources.Hooks))
}
if !reflect.DeepEqual(list, []*library.Hook{resources.Hooks[1], resources.Hooks[0]}) {
Expand All @@ -512,6 +513,16 @@ func testHooks(t *testing.T, db Interface, resources *Resources) {
}
methods["LastHookForRepo"] = true

// lookup a hook with matching webhook_id
got, err = db.GetHookByWebhookID(context.TODO(), resources.Hooks[2].GetWebhookID())
if err != nil {
t.Errorf("unable to get last hook for repo %d: %v", resources.Repos[0].GetID(), err)
}
if !reflect.DeepEqual(got, resources.Hooks[2]) {
t.Errorf("GetHookByWebhookID() is %v, want %v", got, resources.Hooks[2])
}
methods["GetHookByWebhookID"] = true

// lookup the hooks by name
for _, hook := range resources.Hooks {
repo := resources.Repos[hook.GetRepoID()-1]
Expand Down Expand Up @@ -1964,6 +1975,22 @@ func newResources() *Resources {
hookTwo.SetLink("https://github.com/github/octocat/settings/hooks/1")
hookTwo.SetWebhookID(123456)

hookThree := new(library.Hook)
hookThree.SetID(3)
hookThree.SetRepoID(2)
hookThree.SetBuildID(5)
hookThree.SetNumber(1)
hookThree.SetSourceID("c8da1302-07d6-11ea-882f-6793bca275b8")
hookThree.SetCreated(time.Now().UTC().Unix())
hookThree.SetHost("github.com")
hookThree.SetEvent("push")
hookThree.SetEventAction("")
hookThree.SetBranch("main")
hookThree.SetError("")
hookThree.SetStatus("success")
hookThree.SetLink("https://github.com/github/octocat/settings/hooks/1")
hookThree.SetWebhookID(78910)

logServiceOne := new(library.Log)
logServiceOne.SetID(1)
logServiceOne.SetBuildID(1)
Expand Down Expand Up @@ -2278,7 +2305,7 @@ func newResources() *Resources {
Builds: []*library.Build{buildOne, buildTwo},
Deployments: []*library.Deployment{deploymentOne, deploymentTwo},
Executables: []*library.BuildExecutable{executableOne, executableTwo},
Hooks: []*library.Hook{hookOne, hookTwo},
Hooks: []*library.Hook{hookOne, hookTwo, hookThree},
Logs: []*library.Log{logServiceOne, logServiceTwo, logStepOne, logStepTwo},
Pipelines: []*library.Pipeline{pipelineOne, pipelineTwo},
Repos: []*library.Repo{repoOne, repoTwo},
Expand Down
16 changes: 0 additions & 16 deletions scm/github/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,22 +465,6 @@ func (c *client) processRepositoryEvent(h *library.Hook, payload *github.Reposit
r.SetActive(!repo.GetArchived())
r.SetTopics(repo.Topics)

// if action is renamed, then get the previous name from payload
if payload.GetAction() == constants.ActionRenamed {
r.SetPreviousName(repo.GetOwner().GetLogin() + "/" + payload.GetChanges().GetRepo().GetName().GetFrom())
}

// if action is transferred, then get the previous owner from payload
// could be a user or an org, but both are User structs
if payload.GetAction() == constants.ActionTransferred {
org := payload.GetChanges().GetOwner().GetOwnerInfo().GetOrg()
if org == nil {
org = payload.GetChanges().GetOwner().GetOwnerInfo().GetUser()
}

r.SetPreviousName(org.GetLogin() + "/" + repo.GetName())
}

h.SetEvent(constants.EventRepository)
h.SetEventAction(payload.GetAction())
h.SetBranch(r.GetBranch())
Expand Down
2 changes: 0 additions & 2 deletions scm/github/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,6 @@ func TestGitHub_ProcessWebhook_RepositoryRename(t *testing.T) {
wantRepo.SetClone("https://octocoders.github.io/Codertocat/Hello-World.git")
wantRepo.SetBranch("master")
wantRepo.SetPrivate(false)
wantRepo.SetPreviousName("Codertocat/Hello-Old-World")
wantRepo.SetTopics(nil)

want := &types.Webhook{
Expand Down Expand Up @@ -1047,7 +1046,6 @@ func TestGitHub_ProcessWebhook_RepositoryTransfer(t *testing.T) {
wantRepo.SetClone("https://octocoders.github.io/Codertocat/Hello-World.git")
wantRepo.SetBranch("master")
wantRepo.SetPrivate(false)
wantRepo.SetPreviousName("Old-Codertocat/Hello-World")
wantRepo.SetTopics(nil)

want := &types.Webhook{
Expand Down

0 comments on commit ccc46bf

Please sign in to comment.