Skip to content

Complete push webhooks #2530

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

Merged
merged 7 commits into from
Sep 21, 2017
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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
65f1bf27bc3bf70f64657658635e66094edbcb4d
57 changes: 41 additions & 16 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const (
ActionReopenIssue // 13
ActionClosePullRequest // 14
ActionReopenPullRequest // 15
ActionDeleteTag // 16
ActionDeleteBranch // 17
)

var (
Expand Down Expand Up @@ -554,6 +556,12 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
// Check it's tag push or branch.
if strings.HasPrefix(opts.RefFullName, git.TagPrefix) {
opType = ActionPushTag
if opts.NewCommitID == git.EmptySHA {
opType = ActionDeleteTag
}
opts.Commits = &PushCommits{}
} else if opts.NewCommitID == git.EmptySHA {
opType = ActionDeleteBranch
opts.Commits = &PushCommits{}
} else {
// if not the first commit, set the compare URL.
Expand Down Expand Up @@ -599,40 +607,38 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
apiRepo := repo.APIFormat(AccessModeNone)

var shaSum string
var isHookEventPush = false
switch opType {
case ActionCommitRepo: // Push
if err = PrepareWebhooks(repo, HookEventPush, &api.PushPayload{
Ref: opts.RefFullName,
Before: opts.OldCommitID,
After: opts.NewCommitID,
CompareURL: setting.AppURL + opts.Commits.CompareURL,
Commits: opts.Commits.ToAPIPayloadCommits(repo.HTMLURL()),
Repo: apiRepo,
Pusher: apiPusher,
Sender: apiPusher,
}); err != nil {
return fmt.Errorf("PrepareWebhooks: %v", err)
}
isHookEventPush = true
Copy link
Member

@ethantkoenig ethantkoenig Sep 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment may have no effect, as you will return on line 626 if isNewBranch is true. I assume that you still want to fire the push webhook even if it is a new branch, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes correct. Forgot to replace the return by an error check.


if isNewBranch {
gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil {
log.Error(4, "OpenRepository[%s]: %v", repo.RepoPath(), err)
}

shaSum, err = gitRepo.GetBranchCommitID(refName)
if err != nil {
log.Error(4, "GetBranchCommitID[%s]: %v", opts.RefFullName, err)
}
return PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
Ref: refName,
Sha: shaSum,
RefType: "branch",
Repo: apiRepo,
Sender: apiPusher,
})
}); err != nil {
return fmt.Errorf("PrepareWebhooks: %v", err)
}
}

case ActionDeleteBranch: // Delete Branch
isHookEventPush = true

case ActionPushTag: // Create
isHookEventPush = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment has no effect, since if you reach here you will return on line 649.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.


gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil {
log.Error(4, "OpenRepository[%s]: %v", repo.RepoPath(), err)
Expand All @@ -641,13 +647,32 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
if err != nil {
log.Error(4, "GetTagCommitID[%s]: %v", opts.RefFullName, err)
}
return PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
Ref: refName,
Sha: shaSum,
RefType: "tag",
Repo: apiRepo,
Sender: apiPusher,
})
}); err != nil {
return fmt.Errorf("PrepareWebhooks: %v", err)
}
case ActionDeleteTag: // Delete Tag
isHookEventPush = true
}

if isHookEventPush {
if err = PrepareWebhooks(repo, HookEventPush, &api.PushPayload{
Ref: opts.RefFullName,
Before: opts.OldCommitID,
After: opts.NewCommitID,
CompareURL: setting.AppURL + opts.Commits.CompareURL,
Commits: opts.Commits.ToAPIPayloadCommits(repo.HTMLURL()),
Repo: apiRepo,
Pusher: apiPusher,
Sender: apiPusher,
}); err != nil {
return fmt.Errorf("PrepareWebhooks: %v", err)
}
}

return nil
Expand Down
144 changes: 103 additions & 41 deletions models/action_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"strings"
"testing"

"code.gitea.io/git"
"code.gitea.io/gitea/modules/setting"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -202,55 +203,116 @@ func TestUpdateIssuesCommit(t *testing.T) {
CheckConsistencyFor(t, &Action{})
}

func TestCommitRepoAction(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{ID: 2, OwnerID: user.ID}).(*Repository)
repo.Owner = user
func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) {
AssertNotExistsBean(t, actionBean)
assert.NoError(t, CommitRepoAction(opts))
AssertExistsAndLoadBean(t, actionBean)
CheckConsistencyFor(t, &Action{})
}

pushCommits := NewPushCommits()
pushCommits.Commits = []*PushCommit{
func TestCommitRepoAction(t *testing.T) {
samples := []struct {
userID int64
repositoryID int64
commitRepoActionOptions CommitRepoActionOptions
action Action
}{
{
Sha1: "abcdef1",
CommitterEmail: "[email protected]",
CommitterName: "User Two",
AuthorEmail: "[email protected]",
AuthorName: "User Four",
Message: "message1",
userID: 2,
repositoryID: 2,
commitRepoActionOptions: CommitRepoActionOptions{
RefFullName: "refName",
OldCommitID: "oldCommitID",
NewCommitID: "newCommitID",
Commits: &PushCommits{
avatars: make(map[string]string),
Commits: []*PushCommit{
{
Sha1: "abcdef1",
CommitterEmail: "[email protected]",
CommitterName: "User Two",
AuthorEmail: "[email protected]",
AuthorName: "User Four",
Message: "message1",
},
{
Sha1: "abcdef2",
CommitterEmail: "[email protected]",
CommitterName: "User Two",
AuthorEmail: "[email protected]",
AuthorName: "User Two",
Message: "message2",
},
},
Len: 2,
},
},
action: Action{
OpType: ActionCommitRepo,
RefName: "refName",
},
},
{
Sha1: "abcdef2",
CommitterEmail: "[email protected]",
CommitterName: "User Two",
AuthorEmail: "[email protected]",
AuthorName: "User Two",
Message: "message2",
userID: 2,
repositoryID: 1,
commitRepoActionOptions: CommitRepoActionOptions{
RefFullName: git.TagPrefix + "v1.1",
OldCommitID: git.EmptySHA,
NewCommitID: "newCommitID",
Commits: &PushCommits{},
},
action: Action{
OpType: ActionPushTag,
RefName: "v1.1",
},
},
{
userID: 2,
repositoryID: 1,
commitRepoActionOptions: CommitRepoActionOptions{
RefFullName: git.TagPrefix + "v1.1",
OldCommitID: "oldCommitID",
NewCommitID: git.EmptySHA,
Commits: &PushCommits{},
},
action: Action{
OpType: ActionDeleteTag,
RefName: "v1.1",
},
},
{
userID: 2,
repositoryID: 1,
commitRepoActionOptions: CommitRepoActionOptions{
RefFullName: git.BranchPrefix + "feature/1",
OldCommitID: "oldCommitID",
NewCommitID: git.EmptySHA,
Commits: &PushCommits{},
},
action: Action{
OpType: ActionDeleteBranch,
RefName: "feature/1",
},
},
}
pushCommits.Len = len(pushCommits.Commits)

actionBean := &Action{
OpType: ActionCommitRepo,
ActUserID: user.ID,
ActUser: user,
RepoID: repo.ID,
Repo: repo,
RefName: "refName",
IsPrivate: repo.IsPrivate,
for _, s := range samples {
prepareTestEnv(t)

user := AssertExistsAndLoadBean(t, &User{ID: s.userID}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{ID: s.repositoryID, OwnerID: user.ID}).(*Repository)
repo.Owner = user

s.commitRepoActionOptions.PusherName = user.Name
s.commitRepoActionOptions.RepoOwnerID = user.ID
s.commitRepoActionOptions.RepoName = repo.Name

s.action.ActUserID = user.ID
s.action.RepoID = repo.ID
s.action.IsPrivate = repo.IsPrivate

testCorrectRepoAction(t, s.commitRepoActionOptions, &s.action)
}
AssertNotExistsBean(t, actionBean)
assert.NoError(t, CommitRepoAction(CommitRepoActionOptions{
PusherName: user.Name,
RepoOwnerID: user.ID,
RepoName: repo.Name,
RefFullName: "refName",
OldCommitID: "oldCommitID",
NewCommitID: "newCommitID",
Commits: pushCommits,
}))
AssertExistsAndLoadBean(t, actionBean)
CheckConsistencyFor(t, &Action{})
}

func TestTransferRepoAction(t *testing.T) {
Expand Down
10 changes: 10 additions & 0 deletions models/unit_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@
package models

import (
"os"
"testing"

"code.gitea.io/gitea/modules/setting"

"github.com/Unknwon/com"
"github.com/go-xorm/core"
"github.com/go-xorm/xorm"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -38,6 +42,12 @@ func PrepareTestDatabase() error {
return LoadFixtures()
}

func prepareTestEnv(t testing.TB) {
assert.NoError(t, PrepareTestDatabase())
assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
assert.NoError(t, com.CopyDir("../integrations/gitea-repositories-meta", setting.RepoRootPath))
}

type testCond struct {
query interface{}
args []interface{}
Expand Down
Loading