From 3b5cf448e9bc388e1daa89fbebc24fae176bd815 Mon Sep 17 00:00:00 2001 From: Anton Bracke Date: Sun, 16 Jan 2022 14:24:54 +0100 Subject: [PATCH 01/20] load changed files for PRs --- server/api/hook.go | 9 +++- server/remote/bitbucket/bitbucket.go | 2 +- server/remote/bitbucket/bitbucket_test.go | 2 +- .../remote/bitbucketserver/bitbucketserver.go | 2 +- server/remote/coding/coding.go | 2 +- server/remote/coding/coding_test.go | 2 +- server/remote/gitea/gitea.go | 2 +- server/remote/github/convert.go | 2 +- server/remote/github/github.go | 50 ++++++++++++++++++- server/remote/gitlab/gitlab.go | 45 ++++++++++++++++- server/remote/gitlab/gitlab_test.go | 6 +-- server/remote/gogs/gogs.go | 2 +- server/remote/mocks/remote.go | 2 +- server/remote/remote.go | 2 +- 14 files changed, 112 insertions(+), 18 deletions(-) diff --git a/server/api/hook.go b/server/api/hook.go index 3865ed1e7a..dcddc6be2d 100644 --- a/server/api/hook.go +++ b/server/api/hook.go @@ -78,7 +78,7 @@ func BlockTilQueueHasRunningItem(c *gin.Context) { func PostHook(c *gin.Context) { _store := store.FromContext(c) - tmpRepo, build, err := server.Config.Services.Remote.Hook(c.Request) + tmpRepo, build, err := server.Config.Services.Remote.Hook(c, c.Request) if err != nil { msg := "failure to parse hook" log.Debug().Err(err).Msg(msg) @@ -288,6 +288,7 @@ func branchFiltered(build *model.Build, remoteYamlConfigs []*remote.FileMeta) (b return false, nil } } + return true, nil } @@ -311,6 +312,12 @@ func zeroSteps(build *model.Build, remoteYamlConfigs []*remote.FileMeta) bool { return true } + for _, buildItem := range buildItems { + if buildItem.Proc.State != model.StatusPending { + return false + } + } + return false } diff --git a/server/remote/bitbucket/bitbucket.go b/server/remote/bitbucket/bitbucket.go index 4f3a9b4a0b..f5d1527767 100644 --- a/server/remote/bitbucket/bitbucket.go +++ b/server/remote/bitbucket/bitbucket.go @@ -284,7 +284,7 @@ func (c *config) Branches(ctx context.Context, u *model.User, r *model.Repo) ([] // Hook parses the incoming Bitbucket hook and returns the Repository and // Build details. If the hook is unsupported nil values are returned. -func (c *config) Hook(req *http.Request) (*model.Repo, *model.Build, error) { +func (c *config) Hook(ctx context.Context, req *http.Request) (*model.Repo, *model.Build, error) { return parseHook(req) } diff --git a/server/remote/bitbucket/bitbucket_test.go b/server/remote/bitbucket/bitbucket_test.go index 7ed57df0d5..216109a47b 100644 --- a/server/remote/bitbucket/bitbucket_test.go +++ b/server/remote/bitbucket/bitbucket_test.go @@ -264,7 +264,7 @@ func Test_bitbucket(t *testing.T) { req.Header = http.Header{} req.Header.Set(hookEvent, hookPush) - r, _, err := c.Hook(req) + r, _, err := c.Hook(ctx, req) g.Assert(err).IsNil() g.Assert(r.FullName).Equal("user_name/repo_name") }) diff --git a/server/remote/bitbucketserver/bitbucketserver.go b/server/remote/bitbucketserver/bitbucketserver.go index 54c0763d92..2dd75e68c4 100644 --- a/server/remote/bitbucketserver/bitbucketserver.go +++ b/server/remote/bitbucketserver/bitbucketserver.go @@ -236,7 +236,7 @@ func (c *Config) Deactivate(ctx context.Context, u *model.User, r *model.Repo, l return client.DeleteHook(r.Owner, r.Name, link) } -func (c *Config) Hook(r *http.Request) (*model.Repo, *model.Build, error) { +func (c *Config) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { return parseHook(r, c.URL) } diff --git a/server/remote/coding/coding.go b/server/remote/coding/coding.go index c251f69efc..d7a2a13761 100644 --- a/server/remote/coding/coding.go +++ b/server/remote/coding/coding.go @@ -284,7 +284,7 @@ func (c *Coding) Branches(ctx context.Context, u *model.User, r *model.Repo) ([] // Hook parses the post-commit hook from the Request body and returns the // required data in a standard format. -func (c *Coding) Hook(r *http.Request) (*model.Repo, *model.Build, error) { +func (c *Coding) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { repo, build, err := parseHook(r) if build != nil { build.Avatar = c.resourceLink(build.Avatar) diff --git a/server/remote/coding/coding_test.go b/server/remote/coding/coding_test.go index 3034f2fbc2..9cd1b2170c 100644 --- a/server/remote/coding/coding_test.go +++ b/server/remote/coding/coding_test.go @@ -223,7 +223,7 @@ func Test_coding(t *testing.T) { req.Header = http.Header{} req.Header.Set(hookEvent, hookPush) - r, _, err := c.Hook(req) + r, _, err := c.Hook(ctx, req) g.Assert(err).IsNil() g.Assert(r.FullName).Equal("demo1/test1") }) diff --git a/server/remote/gitea/gitea.go b/server/remote/gitea/gitea.go index e1726783b5..67e6538e6d 100644 --- a/server/remote/gitea/gitea.go +++ b/server/remote/gitea/gitea.go @@ -442,7 +442,7 @@ func (c *Gitea) Branches(ctx context.Context, u *model.User, r *model.Repo) ([]s // Hook parses the incoming Gitea hook and returns the Repository and Build // details. If the hook is unsupported nil values are returned. -func (c *Gitea) Hook(r *http.Request) (*model.Repo, *model.Build, error) { +func (c *Gitea) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { return parseHook(r) } diff --git a/server/remote/github/convert.go b/server/remote/github/convert.go index 8efff0e23d..f15e122cdd 100644 --- a/server/remote/github/convert.go +++ b/server/remote/github/convert.go @@ -221,7 +221,7 @@ func convertDeployHook(from *webhook) *model.Build { Deploy: from.Deployment.Env, Sender: from.Sender.Login, } - // if the ref is a sha or short sha we need to manuallyconstruct the ref. + // if the ref is a sha or short sha we need to manually construct the ref. if strings.HasPrefix(build.Commit, build.Ref) || build.Commit == build.Ref { build.Branch = from.Repo.DefaultBranch if build.Branch == "" { diff --git a/server/remote/github/github.go b/server/remote/github/github.go index ad4cbe8595..8d07b4e7f3 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -32,6 +32,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/common" + "github.com/woodpecker-ci/woodpecker/server/store" ) const ( @@ -491,6 +492,51 @@ func (c *client) Branches(ctx context.Context, u *model.User, r *model.Repo) ([] // Hook parses the post-commit hook from the Request body // and returns the required data in a standard format. -func (c *client) Hook(r *http.Request) (*model.Repo, *model.Build, error) { - return parseHook(r, c.MergeRef) +func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { + repo, build, err := parseHook(r, c.MergeRef) + if err != nil { + return nil, nil, err + } + + if build.Event == model.EventPull { + build, err = c.loadChangedFilesFromPullRequest(ctx, repo, build) + if err != nil { + return nil, nil, err + } + } + + return repo, build, nil +} + +func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, repo *model.Repo, build *model.Build) (*model.Build, error) { + user, err := store.FromContext(ctx).GetUser(repo.UserID) + if err != nil { + return nil, err + } + + client := c.newClientToken(ctx, user.Token) + + // extract id from ref: "refs/pull/%d/merge" + pullRequestID, err := strconv.Atoi(build.Ref) + if err != nil { + return nil, err + } + + opts := new(github.ListOptions) + opts.Page = 1 + + for opts.Page > 0 { + files, resp, err := client.PullRequests.ListFiles(ctx, repo.Owner, repo.Name, pullRequestID, opts) + if err != nil { + return nil, err + } + + for _, file := range files { + build.ChangedFiles = append(build.ChangedFiles, *file.Filename) + } + + opts.Page = resp.NextPage + } + + return build, nil } diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index 8165198f62..a4ce16b62a 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -22,6 +22,7 @@ import ( "net" "net/http" "net/url" + "strconv" "strings" "github.com/rs/zerolog/log" @@ -31,6 +32,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/common" + "github.com/woodpecker-ci/woodpecker/server/store" "github.com/woodpecker-ci/woodpecker/shared/oauth2" ) @@ -524,7 +526,7 @@ func (g *Gitlab) Branches(ctx context.Context, user *model.User, repo *model.Rep // Hook parses the post-commit hook from the Request body // and returns the required data in a standard format. -func (g *Gitlab) Hook(req *http.Request) (*model.Repo, *model.Build, error) { +func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *model.Build, error) { defer req.Body.Close() payload, err := ioutil.ReadAll(req.Body) if err != nil { @@ -540,10 +542,49 @@ func (g *Gitlab) Hook(req *http.Request) (*model.Repo, *model.Build, error) { case *gitlab.MergeEvent: return convertMergeRequestHock(event, req) case *gitlab.PushEvent: - return convertPushHock(event) + repo, build, err := convertPushHock(event) + if err != nil { + return nil, nil, err + } + + build, err = g.loadChangedFilesFromPullRequest(ctx, repo, build) + if err != nil { + return nil, nil, err + } + + return repo, build, nil case *gitlab.TagEvent: return convertTagHock(event) default: return nil, nil, nil } } + +func (g *Gitlab) loadChangedFilesFromPullRequest(ctx context.Context, repo *model.Repo, build *model.Build) (*model.Build, error) { + user, err := store.FromContext(ctx).GetUser(repo.UserID) + if err != nil { + return nil, err + } + + client, err := newClient(g.URL, user.Token, g.SkipVerify) + if err != nil { + return nil, err + } + + // extract id from ref: "refs/merge-requests/%d/head" + pullRequestID, err := strconv.Atoi(build.Ref) + if err != nil { + return nil, err + } + + changes, _, err := client.MergeRequests.GetMergeRequestChanges(repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) + if err != nil { + return nil, err + } + + for _, file := range changes.Changes { + build.ChangedFiles = append(build.ChangedFiles, file.NewPath) + } + + return build, nil +} diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index b90a09715d..82d711af45 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -169,7 +169,7 @@ func Test_Gitlab(t *testing.T) { ) req.Header = testdata.ServiceHookHeaders - hookRepo, build, err := client.Hook(req) + hookRepo, build, err := client.Hook(ctx, req) assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { assert.Equal(t, build.Event, model.EventPush) @@ -191,7 +191,7 @@ func Test_Gitlab(t *testing.T) { ) req.Header = testdata.ServiceHookHeaders - hookRepo, build, err := client.Hook(req) + hookRepo, build, err := client.Hook(ctx, req) assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { assert.Equal(t, "test", hookRepo.Owner) @@ -212,7 +212,7 @@ func Test_Gitlab(t *testing.T) { ) req.Header = testdata.ServiceHookHeaders - hookRepo, build, err := client.Hook(req) + hookRepo, build, err := client.Hook(ctx, req) assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { assert.Equal(t, "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", hookRepo.Avatar) diff --git a/server/remote/gogs/gogs.go b/server/remote/gogs/gogs.go index 97f0182288..553c40d832 100644 --- a/server/remote/gogs/gogs.go +++ b/server/remote/gogs/gogs.go @@ -264,7 +264,7 @@ func (c *client) Branches(ctx context.Context, u *model.User, r *model.Repo) ([] // Hook parses the incoming Gogs hook and returns the Repository and Build // details. If the hook is unsupported nil values are returned. -func (c *client) Hook(r *http.Request) (*model.Repo, *model.Build, error) { +func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { return parseHook(r) } diff --git a/server/remote/mocks/remote.go b/server/remote/mocks/remote.go index c56bedeec6..e391b98e20 100644 --- a/server/remote/mocks/remote.go +++ b/server/remote/mocks/remote.go @@ -137,7 +137,7 @@ func (_m *Remote) File(ctx context.Context, u *model.User, r *model.Repo, b *mod } // Hook provides a mock function with given fields: r -func (_m *Remote) Hook(r *http.Request) (*model.Repo, *model.Build, error) { +func (_m *Remote) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { ret := _m.Called(r) var r0 *model.Repo diff --git a/server/remote/remote.go b/server/remote/remote.go index 78a567f2a3..e79ba57bea 100644 --- a/server/remote/remote.go +++ b/server/remote/remote.go @@ -75,7 +75,7 @@ type Remote interface { // Hook parses the post-commit hook from the Request body and returns the // required data in a standard format. - Hook(r *http.Request) (*model.Repo, *model.Build, error) + Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) } // FileMeta represents a file in version control From f0492cca0027929d50215aadf545712177acb34b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 03:14:13 +0100 Subject: [PATCH 02/20] fix dublicated changed file bug for gitea --- server/remote/gitea/helper.go | 6 +++--- shared/utils/strings.go | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 shared/utils/strings.go diff --git a/server/remote/gitea/helper.go b/server/remote/gitea/helper.go index ed28a5f89a..e49cdb4d4e 100644 --- a/server/remote/gitea/helper.go +++ b/server/remote/gitea/helper.go @@ -25,6 +25,7 @@ import ( "code.gitea.io/sdk/gitea" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/shared/utils" ) // helper function that converts a Gitea repository to a Woodpecker repository. @@ -110,15 +111,14 @@ func buildFromPush(hook *pushHook) *model.Build { } func getChangedFilesFromPushHook(hook *pushHook) []string { - files := make([]string, 0) - + files := make([]string, 0, len(hook.Commits)*4) for _, c := range hook.Commits { files = append(files, c.Added...) files = append(files, c.Removed...) files = append(files, c.Modified...) } - return files + return utils.DedupStrings(files) } // helper function that extracts the Build data from a Gitea tag hook diff --git a/shared/utils/strings.go b/shared/utils/strings.go new file mode 100644 index 0000000000..0b6ddcdc44 --- /dev/null +++ b/shared/utils/strings.go @@ -0,0 +1,18 @@ +package utils + +// DedupStrings deduplicate string list, empty items are dropped +func DedupStrings(list []string) []string { + m := make(map[string]struct{}, len(list)) + + for i := range list { + if s := list[i]; len(s) > 0 { + m[list[i]] = struct{}{} + } + } + + newList := make([]string, 0, len(m)) + for k := range m { + newList = append(newList, k) + } + return newList +} From aae0dd54a4fe7ff7c7b3a968091de897c3912e61 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 03:14:36 +0100 Subject: [PATCH 03/20] fix gitlab --- server/remote/gitlab/convert.go | 17 ++++++++++-- server/remote/gitlab/gitlab.go | 43 +---------------------------- server/remote/gitlab/gitlab_test.go | 5 +++- 3 files changed, 20 insertions(+), 45 deletions(-) diff --git a/server/remote/gitlab/convert.go b/server/remote/gitlab/convert.go index aa8ba050f4..0a32c2d258 100644 --- a/server/remote/gitlab/convert.go +++ b/server/remote/gitlab/convert.go @@ -24,6 +24,7 @@ import ( "github.com/xanzy/go-gitlab" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/shared/utils" ) func (g *Gitlab) convertGitlabRepo(_repo *gitlab.Project) (*model.Repo, error) { @@ -114,7 +115,6 @@ func convertMergeRequestHock(hook *gitlab.MergeEvent, req *http.Request) (*model build.Remote = obj.Source.HTTPURL build.Ref = fmt.Sprintf("refs/merge-requests/%d/head", obj.IID) - build.Branch = obj.SourceBranch author := lastCommit.Author @@ -129,6 +129,14 @@ func convertMergeRequestHock(hook *gitlab.MergeEvent, req *http.Request) (*model build.Title = obj.Title build.Link = obj.URL + if hook.ObjectAttributes.StDiffs != nil { + files := make([]string, 0, len(hook.ObjectAttributes.StDiffs)*2) + for _, diff := range hook.ObjectAttributes.StDiffs { + files = append(files, diff.OldPath, diff.NewPath) + } + build.ChangedFiles = utils.DedupStrings(files) + } + return repo, build, nil } @@ -161,6 +169,7 @@ func convertPushHock(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) build.Branch = strings.TrimPrefix(hook.Ref, "refs/heads/") build.Ref = hook.Ref + files := make([]string, 0, len(hook.Commits)*4) for _, cm := range hook.Commits { if hook.After == cm.ID { build.Author = cm.Author.Name @@ -170,9 +179,13 @@ func convertPushHock(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) if len(build.Email) != 0 { build.Avatar = getUserAvatar(build.Email) } - break } + + files = append(files, cm.Added...) + files = append(files, cm.Removed...) + files = append(files, cm.Modified...) } + build.ChangedFiles = utils.DedupStrings(files) return repo, build, nil } diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index a4ce16b62a..775cd85597 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -22,7 +22,6 @@ import ( "net" "net/http" "net/url" - "strconv" "strings" "github.com/rs/zerolog/log" @@ -32,7 +31,6 @@ import ( "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/common" - "github.com/woodpecker-ci/woodpecker/server/store" "github.com/woodpecker-ci/woodpecker/shared/oauth2" ) @@ -542,49 +540,10 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod case *gitlab.MergeEvent: return convertMergeRequestHock(event, req) case *gitlab.PushEvent: - repo, build, err := convertPushHock(event) - if err != nil { - return nil, nil, err - } - - build, err = g.loadChangedFilesFromPullRequest(ctx, repo, build) - if err != nil { - return nil, nil, err - } - - return repo, build, nil + return convertPushHock(event) case *gitlab.TagEvent: return convertTagHock(event) default: return nil, nil, nil } } - -func (g *Gitlab) loadChangedFilesFromPullRequest(ctx context.Context, repo *model.Repo, build *model.Build) (*model.Build, error) { - user, err := store.FromContext(ctx).GetUser(repo.UserID) - if err != nil { - return nil, err - } - - client, err := newClient(g.URL, user.Token, g.SkipVerify) - if err != nil { - return nil, err - } - - // extract id from ref: "refs/merge-requests/%d/head" - pullRequestID, err := strconv.Atoi(build.Ref) - if err != nil { - return nil, err - } - - changes, _, err := client.MergeRequests.GetMergeRequestChanges(repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) - if err != nil { - return nil, err - } - - for _, file := range changes.Changes { - build.ChangedFiles = append(build.ChangedFiles, file.NewPath) - } - - return build, nil -} diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index 82d711af45..faa43b3037 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -178,6 +178,7 @@ func Test_Gitlab(t *testing.T) { assert.Equal(t, "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", hookRepo.Avatar) assert.Equal(t, "develop", hookRepo.Branch) assert.Equal(t, "refs/heads/master", build.Ref) + assert.Equal(t, []string{"cmd/cli/main.go"}, build.ChangedFiles) } }) }) @@ -199,6 +200,7 @@ func Test_Gitlab(t *testing.T) { assert.Equal(t, "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", hookRepo.Avatar) assert.Equal(t, "develop", hookRepo.Branch) assert.Equal(t, "refs/tags/v22", build.Ref) + assert.Len(t, build.ChangedFiles, 0) } }) }) @@ -208,7 +210,7 @@ func Test_Gitlab(t *testing.T) { req, _ := http.NewRequest( testdata.ServiceHookMethod, testdata.ServiceHookURL.String(), - bytes.NewReader(testdata.ServiceHookMergeRequestBody), + bytes.NewReader(testdata.ServiceHookMergeRequestBody), // TODO: update test data with new hooks ) req.Header = testdata.ServiceHookHeaders @@ -220,6 +222,7 @@ func Test_Gitlab(t *testing.T) { assert.Equal(t, "test", hookRepo.Owner) assert.Equal(t, "woodpecker", hookRepo.Name) assert.Equal(t, "Update client.go 🎉", build.Title) + assert.Len(t, build.ChangedFiles, 0) // TODO: update test data with new hooks } }) }) From 1b93027f0c2668d5144914e70698b7edbcd31dd4 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 03:39:43 +0100 Subject: [PATCH 04/20] update docs --- docs/docs/20-usage/22-conditional-execution.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/20-usage/22-conditional-execution.md b/docs/docs/20-usage/22-conditional-execution.md index bc73a8b84f..ffcb81a0ad 100644 --- a/docs/docs/20-usage/22-conditional-execution.md +++ b/docs/docs/20-usage/22-conditional-execution.md @@ -159,7 +159,7 @@ when: ## `path` -> NOTE: This feature is currently only available for GitHub and Gitea repositories. +> NOTE: This feature is currently only available for GitHub, Gitlab and Gitea repositories. Execute a step only on a pipeline with certain files being changed: From d04a45722499a77448ace955e743478ac6395c48 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 05:18:10 +0100 Subject: [PATCH 05/20] rm unrelated --- server/api/hook.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/api/hook.go b/server/api/hook.go index dcddc6be2d..a2e1aa2a6f 100644 --- a/server/api/hook.go +++ b/server/api/hook.go @@ -312,12 +312,6 @@ func zeroSteps(build *model.Build, remoteYamlConfigs []*remote.FileMeta) bool { return true } - for _, buildItem := range buildItems { - if buildItem.Proc.State != model.StatusPending { - return false - } - } - return false } From a36d1aaf08e2923a53fbe8cb8dd032d0ef494fc4 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 05:19:20 +0100 Subject: [PATCH 06/20] Start using github lib for webhook parsing --- server/remote/github/convert.go | 128 +------------- server/remote/github/convert_test.go | 238 ++++++++++++++------------- server/remote/github/github.go | 27 ++- server/remote/github/parse.go | 163 +++++++++++++----- server/remote/github/parse_test.go | 66 ++++---- server/remote/github/types.go | 102 ------------ 6 files changed, 299 insertions(+), 425 deletions(-) delete mode 100644 server/remote/github/types.go diff --git a/server/remote/github/convert.go b/server/remote/github/convert.go index f15e122cdd..10101a78dd 100644 --- a/server/remote/github/convert.go +++ b/server/remote/github/convert.go @@ -15,9 +15,6 @@ package github import ( - "fmt" - "strings" - "github.com/google/go-github/v39/github" "github.com/woodpecker-ci/woodpecker/server/model" @@ -44,7 +41,7 @@ const ( const ( headRefs = "refs/pull/%d/head" // pull request unmerged mergeRefs = "refs/pull/%d/merge" // pull request merged with base - refspec = "%s:%s" + refSpec = "%s:%s" ) // convertStatus is a helper function used to convert a Woodpecker status to a @@ -146,129 +143,22 @@ func convertTeam(from *github.Organization) *model.Team { // convertRepoHook is a helper function used to extract the Repository details // from a webhook and convert to the common Woodpecker repository structure. -func convertRepoHook(from *webhook) *model.Repo { +func convertRepoHook(eventRepo *github.PushEventRepository) *model.Repo { repo := &model.Repo{ - Owner: from.Repo.Owner.Login, - Name: from.Repo.Name, - FullName: from.Repo.FullName, - Link: from.Repo.HTMLURL, - IsSCMPrivate: from.Repo.Private, - Clone: from.Repo.CloneURL, - Branch: from.Repo.DefaultBranch, + Owner: eventRepo.GetOwner().GetLogin(), + Name: eventRepo.GetName(), + FullName: eventRepo.GetFullName(), + Link: eventRepo.GetHTMLURL(), + IsSCMPrivate: eventRepo.GetPrivate(), + Clone: eventRepo.GetCloneURL(), + Branch: eventRepo.GetDefaultBranch(), SCMKind: model.RepoGit, } if repo.Branch == "" { repo.Branch = defaultBranch } - if repo.Owner == "" { // legacy webhooks - repo.Owner = from.Repo.Owner.Name - } if repo.FullName == "" { repo.FullName = repo.Owner + "/" + repo.Name } return repo } - -// convertPushHook is a helper function used to extract the Build details -// from a push webhook and convert to the common Woodpecker Build structure. -func convertPushHook(from *webhook) *model.Build { - files := getChangedFilesFromWebhook(from) - build := &model.Build{ - Event: model.EventPush, - Commit: from.Head.ID, - Ref: from.Ref, - Link: from.Head.URL, - Branch: strings.Replace(from.Ref, "refs/heads/", "", -1), - Message: from.Head.Message, - Email: from.Head.Author.Email, - Avatar: from.Sender.Avatar, - Author: from.Sender.Login, - Remote: from.Repo.CloneURL, - Sender: from.Sender.Login, - ChangedFiles: files, - } - if len(build.Author) == 0 { - build.Author = from.Head.Author.Username - } - // if len(build.Email) == 0 { - // TODO: default to gravatar? - // } - if strings.HasPrefix(build.Ref, "refs/tags/") { - // just kidding, this is actually a tag event. Why did this come as a push - // event we'll never know! - build.Event = model.EventTag - // For tags, if the base_ref (tag's base branch) is set, we're using it - // as build's branch so that we can filter events base on it - if strings.HasPrefix(from.BaseRef, "refs/heads/") { - build.Branch = strings.Replace(from.BaseRef, "refs/heads/", "", -1) - } - } - return build -} - -// convertPushHook is a helper function used to extract the Build details -// from a deploy webhook and convert to the common Woodpecker Build structure. -func convertDeployHook(from *webhook) *model.Build { - build := &model.Build{ - Event: model.EventDeploy, - Commit: from.Deployment.Sha, - Link: from.Deployment.URL, - Message: from.Deployment.Desc, - Avatar: from.Sender.Avatar, - Author: from.Sender.Login, - Ref: from.Deployment.Ref, - Branch: from.Deployment.Ref, - Deploy: from.Deployment.Env, - Sender: from.Sender.Login, - } - // if the ref is a sha or short sha we need to manually construct the ref. - if strings.HasPrefix(build.Commit, build.Ref) || build.Commit == build.Ref { - build.Branch = from.Repo.DefaultBranch - if build.Branch == "" { - build.Branch = defaultBranch - } - build.Ref = fmt.Sprintf("refs/heads/%s", build.Branch) - } - // if the ref is a branch we should make sure it has refs/heads prefix - if !strings.HasPrefix(build.Ref, "refs/") { // branch or tag - build.Ref = fmt.Sprintf("refs/heads/%s", build.Branch) - } - return build -} - -// convertPullHook is a helper function used to extract the Build details -// from a pull request webhook and convert to the common Woodpecker Build structure. -func convertPullHook(from *webhook, merge bool) *model.Build { - build := &model.Build{ - Event: model.EventPull, - Commit: from.PullRequest.Head.SHA, - Link: from.PullRequest.HTMLURL, - Ref: fmt.Sprintf(headRefs, from.PullRequest.Number), - Branch: from.PullRequest.Base.Ref, - Message: from.PullRequest.Title, - Author: from.PullRequest.User.Login, - Avatar: from.PullRequest.User.Avatar, - Title: from.PullRequest.Title, - Sender: from.Sender.Login, - Remote: from.PullRequest.Head.Repo.CloneURL, - Refspec: fmt.Sprintf(refspec, - from.PullRequest.Head.Ref, - from.PullRequest.Base.Ref, - ), - } - if merge { - build.Ref = fmt.Sprintf(mergeRefs, from.PullRequest.Number) - } - return build -} - -func getChangedFilesFromWebhook(from *webhook) []string { - var files []string - files = append(files, from.Head.Added...) - files = append(files, from.Head.Removed...) - files = append(files, from.Head.Modified...) - if len(files) == 0 { - files = make([]string, 0) - } - return files -} diff --git a/server/remote/github/convert_test.go b/server/remote/github/convert_test.go index cb408bc4ea..3c3d408b42 100644 --- a/server/remote/github/convert_test.go +++ b/server/remote/github/convert_test.go @@ -158,126 +158,130 @@ func Test_helper(t *testing.T) { }) g.It("should convert a repository from webhook", func() { - from := &webhook{} - from.Repo.Owner.Login = "octocat" - from.Repo.Owner.Name = "octocat" - from.Repo.Name = "hello-world" - from.Repo.FullName = "octocat/hello-world" - from.Repo.Private = true - from.Repo.HTMLURL = "https://github.com/octocat/hello-world" - from.Repo.CloneURL = "https://github.com/octocat/hello-world.git" - from.Repo.DefaultBranch = "develop" + from := &github.PushEventRepository{} + from.Owner.Login = github.String("octocat") + from.Owner.Name = github.String("octocat") + from.Name = github.String("hello-world") + from.FullName = github.String("octocat/hello-world") + from.Private = github.Bool(true) + from.HTMLURL = github.String("https://github.com/octocat/hello-world") + from.CloneURL = github.String("https://github.com/octocat/hello-world.git") + from.DefaultBranch = github.String("develop") repo := convertRepoHook(from) - g.Assert(repo.Owner).Equal(from.Repo.Owner.Login) - g.Assert(repo.Name).Equal(from.Repo.Name) - g.Assert(repo.FullName).Equal(from.Repo.FullName) - g.Assert(repo.IsSCMPrivate).Equal(from.Repo.Private) - g.Assert(repo.Link).Equal(from.Repo.HTMLURL) - g.Assert(repo.Clone).Equal(from.Repo.CloneURL) - g.Assert(repo.Branch).Equal(from.Repo.DefaultBranch) + g.Assert(repo.Owner).Equal(*from.Owner.Login) + g.Assert(repo.Name).Equal(*from.Name) + g.Assert(repo.FullName).Equal(*from.FullName) + g.Assert(repo.IsSCMPrivate).Equal(*from.Private) + g.Assert(repo.Link).Equal(*from.HTMLURL) + g.Assert(repo.Clone).Equal(*from.CloneURL) + g.Assert(repo.Branch).Equal(*from.DefaultBranch) }) - g.It("should convert a pull request from webhook", func() { - from := &webhook{} - from.PullRequest.Base.Ref = "master" - from.PullRequest.Head.Ref = "changes" - from.PullRequest.Head.SHA = "f72fc19" - from.PullRequest.Head.Repo.CloneURL = "https://github.com/octocat/hello-world-fork" - from.PullRequest.HTMLURL = "https://github.com/octocat/hello-world/pulls/42" - from.PullRequest.Number = 42 - from.PullRequest.Title = "Updated README.md" - from.PullRequest.User.Login = "octocat" - from.PullRequest.User.Avatar = "https://avatars1.githubusercontent.com/u/583231" - from.Sender.Login = "octocat" - - build := convertPullHook(from, true) - g.Assert(build.Event).Equal(model.EventPull) - g.Assert(build.Branch).Equal(from.PullRequest.Base.Ref) - g.Assert(build.Ref).Equal("refs/pull/42/merge") - g.Assert(build.Refspec).Equal("changes:master") - g.Assert(build.Remote).Equal("https://github.com/octocat/hello-world-fork") - g.Assert(build.Commit).Equal(from.PullRequest.Head.SHA) - g.Assert(build.Message).Equal(from.PullRequest.Title) - g.Assert(build.Title).Equal(from.PullRequest.Title) - g.Assert(build.Author).Equal(from.PullRequest.User.Login) - g.Assert(build.Avatar).Equal(from.PullRequest.User.Avatar) - g.Assert(build.Sender).Equal(from.Sender.Login) - }) - - g.It("should convert a deployment from webhook", func() { - from := &webhook{} - from.Deployment.Desc = ":shipit:" - from.Deployment.Env = "production" - from.Deployment.ID = 42 - from.Deployment.Ref = "master" - from.Deployment.Sha = "f72fc19" - from.Deployment.URL = "https://github.com/octocat/hello-world" - from.Sender.Login = "octocat" - from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" - - build := convertDeployHook(from) - g.Assert(build.Event).Equal(model.EventDeploy) - g.Assert(build.Branch).Equal("master") - g.Assert(build.Ref).Equal("refs/heads/master") - g.Assert(build.Commit).Equal(from.Deployment.Sha) - g.Assert(build.Message).Equal(from.Deployment.Desc) - g.Assert(build.Link).Equal(from.Deployment.URL) - g.Assert(build.Author).Equal(from.Sender.Login) - g.Assert(build.Avatar).Equal(from.Sender.Avatar) - }) - - g.It("should convert a push from webhook", func() { - from := &webhook{} - from.Sender.Login = "octocat" - from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" - from.Repo.CloneURL = "https://github.com/octocat/hello-world.git" - from.Head.Author.Email = "octocat@github.com" - from.Head.Message = "updated README.md" - from.Head.URL = "https://github.com/octocat/hello-world" - from.Head.ID = "f72fc19" - from.Ref = "refs/heads/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventPush) - g.Assert(build.Branch).Equal("master") - g.Assert(build.Ref).Equal("refs/heads/master") - g.Assert(build.Commit).Equal(from.Head.ID) - g.Assert(build.Message).Equal(from.Head.Message) - g.Assert(build.Link).Equal(from.Head.URL) - g.Assert(build.Author).Equal(from.Sender.Login) - g.Assert(build.Avatar).Equal(from.Sender.Avatar) - g.Assert(build.Email).Equal(from.Head.Author.Email) - g.Assert(build.Remote).Equal(from.Repo.CloneURL) - }) - - g.It("should convert a tag from webhook", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Ref).Equal("refs/tags/v1.0.0") - }) - - g.It("should convert tag's base branch from webhook to build's branch ", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - from.BaseRef = "refs/heads/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Branch).Equal("master") - }) - - g.It("should not convert tag's base_ref from webhook if not prefixed with 'ref/heads/'", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - from.BaseRef = "refs/refs/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Branch).Equal("refs/tags/v1.0.0") - }) + /* + + g.It("should convert a pull request from webhook", func() { + from := &github.PullRequestEvent{} + from.PullRequest.Base.Ref = github.String("master") + from.PullRequest.Head.Ref = github.String("changes") + from.PullRequest.Head.SHA = github.String("f72fc19") + from.PullRequest.Head.Repo.CloneURL = github.String("https://github.com/octocat/hello-world-fork") + from.PullRequest.HTMLURL = github.String("https://github.com/octocat/hello-world/pulls/42") + from.PullRequest.Number = 42 + from.PullRequest.Title = github.String("Updated README.md") + from.PullRequest.User.Login = github.String("octocat") + from.PullRequest.User.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") + from.Sender.Login = github.String("octocat") + + build := convertPullHook(from, true) + g.Assert(build.Event).Equal(model.EventPull) + g.Assert(build.Branch).Equal(*from.PullRequest.Base.Ref) + g.Assert(build.Ref).Equal("refs/pull/42/merge") + g.Assert(build.Refspec).Equal("changes:master") + g.Assert(build.Remote).Equal("https://github.com/octocat/hello-world-fork") + g.Assert(build.Commit).Equal(*from.PullRequest.Head.SHA) + g.Assert(build.Message).Equal(*from.PullRequest.Title) + g.Assert(build.Title).Equal(*from.PullRequest.Title) + g.Assert(build.Author).Equal(*from.PullRequest.User.Login) + g.Assert(build.Avatar).Equal(*from.PullRequest.User.AvatarURL) + g.Assert(build.Sender).Equal(*from.Sender.Login) + }) + + g.It("should convert a deployment from webhook", func() { + from := &webhook{} + from.Deployment.Desc = ":shipit:" + from.Deployment.Env = "production" + from.Deployment.ID = 42 + from.Deployment.Ref = "master" + from.Deployment.Sha = "f72fc19" + from.Deployment.URL = "https://github.com/octocat/hello-world" + from.Sender.Login = "octocat" + from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" + + build := convertDeployHook(from) + g.Assert(build.Event).Equal(model.EventDeploy) + g.Assert(build.Branch).Equal("master") + g.Assert(build.Ref).Equal("refs/heads/master") + g.Assert(build.Commit).Equal(from.Deployment.Sha) + g.Assert(build.Message).Equal(from.Deployment.Desc) + g.Assert(build.Link).Equal(from.Deployment.URL) + g.Assert(build.Author).Equal(from.Sender.Login) + g.Assert(build.Avatar).Equal(from.Sender.Avatar) + }) + + g.It("should convert a push from webhook", func() { + from := &webhook{} + from.Sender.Login = "octocat" + from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" + from.Repo.CloneURL = "https://github.com/octocat/hello-world.git" + from.Head.Author.Email = "octocat@github.com" + from.Head.Message = "updated README.md" + from.Head.URL = "https://github.com/octocat/hello-world" + from.Head.ID = "f72fc19" + from.Ref = "refs/heads/master" + + build := convertPushHook(from) + g.Assert(build.Event).Equal(model.EventPush) + g.Assert(build.Branch).Equal("master") + g.Assert(build.Ref).Equal("refs/heads/master") + g.Assert(build.Commit).Equal(from.Head.ID) + g.Assert(build.Message).Equal(from.Head.Message) + g.Assert(build.Link).Equal(from.Head.URL) + g.Assert(build.Author).Equal(from.Sender.Login) + g.Assert(build.Avatar).Equal(from.Sender.Avatar) + g.Assert(build.Email).Equal(from.Head.Author.Email) + g.Assert(build.Remote).Equal(from.Repo.CloneURL) + }) + + g.It("should convert a tag from webhook", func() { + from := &webhook{} + from.Ref = "refs/tags/v1.0.0" + + build := convertPushHook(from) + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Ref).Equal("refs/tags/v1.0.0") + }) + + g.It("should convert tag's base branch from webhook to build's branch ", func() { + from := &webhook{} + from.Ref = "refs/tags/v1.0.0" + from.BaseRef = "refs/heads/master" + + build := convertPushHook(from) + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Branch).Equal("master") + }) + + g.It("should not convert tag's base_ref from webhook if not prefixed with 'ref/heads/'", func() { + from := &webhook{} + from.Ref = "refs/tags/v1.0.0" + from.BaseRef = "refs/refs/master" + + build := convertPushHook(from) + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Branch).Equal("refs/tags/v1.0.0") + }) + + */ }) } diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 8d07b4e7f3..2949c0534f 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -33,6 +33,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/common" "github.com/woodpecker-ci/woodpecker/server/store" + "github.com/woodpecker-ci/woodpecker/shared/utils" ) const ( @@ -493,13 +494,13 @@ func (c *client) Branches(ctx context.Context, u *model.User, r *model.Repo) ([] // Hook parses the post-commit hook from the Request body // and returns the required data in a standard format. func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { - repo, build, err := parseHook(r, c.MergeRef) + pull, repo, build, err := parseHook(r, c.MergeRef, c.PrivateMode) if err != nil { return nil, nil, err } - if build.Event == model.EventPull { - build, err = c.loadChangedFilesFromPullRequest(ctx, repo, build) + if pull != nil && len(build.ChangedFiles) == 0 { + build, err = c.loadChangedFilesFromPullRequest(ctx, pull, repo, build) if err != nil { return nil, nil, err } @@ -508,35 +509,27 @@ func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model return repo, build, nil } -func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, repo *model.Repo, build *model.Build) (*model.Build, error) { +func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, pull *github.PullRequest, repo *model.Repo, build *model.Build) (*model.Build, error) { user, err := store.FromContext(ctx).GetUser(repo.UserID) if err != nil { return nil, err } - client := c.newClientToken(ctx, user.Token) - - // extract id from ref: "refs/pull/%d/merge" - pullRequestID, err := strconv.Atoi(build.Ref) - if err != nil { - return nil, err - } - - opts := new(github.ListOptions) - opts.Page = 1 - + opts := &github.ListOptions{Page: 1} + fileList := make([]string, 0, 16) for opts.Page > 0 { - files, resp, err := client.PullRequests.ListFiles(ctx, repo.Owner, repo.Name, pullRequestID, opts) + files, resp, err := c.newClientToken(ctx, user.Token).PullRequests.ListFiles(ctx, repo.Owner, repo.Name, pull.GetNumber(), opts) if err != nil { return nil, err } for _, file := range files { - build.ChangedFiles = append(build.ChangedFiles, *file.Filename) + fileList = append(fileList, file.GetFilename(), file.GetPreviousFilename()) } opts.Page = resp.NextPage } + build.ChangedFiles = utils.DedupStrings(fileList) return build, nil } diff --git a/server/remote/github/parse.go b/server/remote/github/parse.go index 2f512fc93f..ae9420bb85 100644 --- a/server/remote/github/parse.go +++ b/server/remote/github/parse.go @@ -16,20 +16,20 @@ package github import ( "bytes" - "encoding/json" + "fmt" "io" "io/ioutil" "net/http" + "strings" + + "github.com/google/go-github/v39/github" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/shared/utils" ) const ( - hookEvent = "X-Github-Event" - hookField = "payload" - hookDeploy = "deployment" - hookPush = "push" - hookPull = "pull_request" + hookField = "payload" actionOpen = "opened" actionSync = "synchronize" @@ -39,7 +39,7 @@ const ( // parseHook parses a GitHub hook from an http.Request request and returns // Repo and Build detail. If a hook type is unsupported nil values are returned. -func parseHook(r *http.Request, merge bool) (*model.Repo, *model.Build, error) { +func parseHook(r *http.Request, merge, privateMode bool) (*github.PullRequest, *model.Repo, *model.Build, error) { var reader io.Reader = r.Body if payload := r.FormValue(hookField); payload != "" { @@ -48,59 +48,142 @@ func parseHook(r *http.Request, merge bool) (*model.Repo, *model.Build, error) { raw, err := ioutil.ReadAll(reader) if err != nil { - return nil, nil, err + return nil, nil, nil, err + } + + payload, err := github.ParseWebHook(github.WebHookType(r), raw) + if err != nil { + return nil, nil, nil, err } - switch r.Header.Get(hookEvent) { - case hookPush: - return parsePushHook(raw) - case hookDeploy: - return parseDeployHook(raw) - case hookPull: - return parsePullHook(raw, merge) + switch hook := payload.(type) { + case *github.PushEvent: + repo, build, err := parsePushHook(hook) + return nil, repo, build, err + case *github.DeploymentEvent: + repo, build, err := parseDeployHook(hook, privateMode) + return nil, repo, build, err + case *github.PullRequestEvent: + return parsePullHook(hook, merge, privateMode) } - return nil, nil, nil + return nil, nil, nil, nil } // parsePushHook parses a push hook and returns the Repo and Build details. // If the commit type is unsupported nil values are returned. -func parsePushHook(payload []byte) (*model.Repo, *model.Build, error) { - hook := new(webhook) - err := json.Unmarshal(payload, hook) - if err != nil { - return nil, nil, err +func parsePushHook(hook *github.PushEvent) (*model.Repo, *model.Build, error) { + if hook.Deleted != nil && *hook.Deleted { + return nil, nil, nil } - if hook.Deleted { - return nil, nil, err + + build := &model.Build{ + Event: model.EventPush, + Commit: hook.GetHeadCommit().GetID(), + Ref: hook.GetRef(), + Link: hook.GetHeadCommit().GetURL(), + Branch: strings.Replace(hook.GetRef(), "refs/heads/", "", -1), + Message: hook.GetHeadCommit().GetMessage(), + Email: hook.GetHeadCommit().GetAuthor().GetEmail(), + Avatar: hook.GetSender().GetAvatarURL(), + Author: hook.GetSender().GetLogin(), + Remote: hook.GetRepo().GetCloneURL(), + Sender: hook.GetSender().GetLogin(), + ChangedFiles: getChangedFilesFromCommits(hook.Commits), + } + + if len(build.Author) == 0 { + build.Author = hook.GetHeadCommit().GetAuthor().GetLogin() } - return convertRepoHook(hook), convertPushHook(hook), nil + // if len(build.Email) == 0 { + // TODO: default to gravatar? + // } + if strings.HasPrefix(build.Ref, "refs/tags/") { + // just kidding, this is actually a tag event. Why did this come as a push + // event we'll never know! + build.Event = model.EventTag + build.ChangedFiles = nil + // For tags, if the base_ref (tag's base branch) is set, we're using it + // as build's branch so that we can filter events base on it + if strings.HasPrefix(hook.GetBaseRef(), "refs/heads/") { + build.Branch = strings.Replace(hook.GetBaseRef(), "refs/heads/", "", -1) + } + } + + return convertRepoHook(hook.GetRepo()), build, nil } // parseDeployHook parses a deployment and returns the Repo and Build details. // If the commit type is unsupported nil values are returned. -func parseDeployHook(payload []byte) (*model.Repo, *model.Build, error) { - hook := new(webhook) - if err := json.Unmarshal(payload, hook); err != nil { - return nil, nil, err +func parseDeployHook(hook *github.DeploymentEvent, privateMode bool) (*model.Repo, *model.Build, error) { + build := &model.Build{ + Event: model.EventDeploy, + Commit: hook.GetDeployment().GetSHA(), + Link: hook.GetDeployment().GetURL(), + Message: hook.GetDeployment().GetDescription(), + Ref: hook.GetDeployment().GetRef(), + Branch: hook.GetDeployment().GetRef(), + Deploy: hook.GetDeployment().GetEnvironment(), + Avatar: hook.GetSender().GetAvatarURL(), + Author: hook.GetSender().GetLogin(), + Sender: hook.GetSender().GetLogin(), + } + // if the ref is a sha or short sha we need to manually construct the ref. + if strings.HasPrefix(build.Commit, build.Ref) || build.Commit == build.Ref { + build.Branch = hook.GetRepo().GetDefaultBranch() + if build.Branch == "" { + build.Branch = defaultBranch + } + build.Ref = fmt.Sprintf("refs/heads/%s", build.Branch) } - return convertRepoHook(hook), convertDeployHook(hook), nil + // if the ref is a branch we should make sure it has refs/heads prefix + if !strings.HasPrefix(build.Ref, "refs/") { // branch or tag + build.Ref = fmt.Sprintf("refs/heads/%s", build.Branch) + } + + return convertRepo(hook.GetRepo(), privateMode), build, nil } // parsePullHook parses a pull request hook and returns the Repo and Build // details. If the pull request is closed nil values are returned. -func parsePullHook(payload []byte, merge bool) (*model.Repo, *model.Build, error) { - hook := new(webhook) - err := json.Unmarshal(payload, hook) - if err != nil { - return nil, nil, err +func parsePullHook(hook *github.PullRequestEvent, merge, privateMode bool) (*github.PullRequest, *model.Repo, *model.Build, error) { + // ignore these + if hook.GetAction() != actionOpen && hook.GetAction() != actionSync { + return nil, nil, nil, nil + } + if hook.GetPullRequest().GetState() != stateOpen { + return nil, nil, nil, nil } - // ignore these - if hook.Action != actionOpen && hook.Action != actionSync { - return nil, nil, nil + build := &model.Build{ + Event: model.EventPull, + Commit: hook.GetPullRequest().GetHead().GetSHA(), + Link: hook.GetPullRequest().GetHTMLURL(), + Ref: fmt.Sprintf(headRefs, hook.GetPullRequest().GetNumber()), + Branch: hook.GetPullRequest().GetBase().GetRef(), + Message: hook.GetPullRequest().GetTitle(), + Author: hook.GetPullRequest().GetUser().GetLogin(), + Avatar: hook.GetPullRequest().GetUser().GetAvatarURL(), + Title: hook.GetPullRequest().GetTitle(), + Sender: hook.GetSender().GetLogin(), + Remote: hook.GetPullRequest().GetHead().GetRepo().GetCloneURL(), + Refspec: fmt.Sprintf(refSpec, + hook.GetPullRequest().GetHead().GetRef(), + hook.GetPullRequest().GetBase().GetRef(), + ), } - if hook.PullRequest.State != stateOpen { - return nil, nil, nil + if merge { + build.Ref = fmt.Sprintf(mergeRefs, hook.GetPullRequest().Number) + } + + return hook.GetPullRequest(), convertRepo(hook.GetRepo(), privateMode), build, nil +} + +func getChangedFilesFromCommits(commits []*github.HeadCommit) []string { + files := make([]string, 0, len(commits)*3) + for _, cm := range commits { + files = append(files, cm.Added...) + files = append(files, cm.Removed...) + files = append(files, cm.Modified...) } - return convertRepoHook(hook), convertPullHook(hook, merge), nil + return utils.DedupStrings(files) } diff --git a/server/remote/github/parse_test.go b/server/remote/github/parse_test.go index 696d6c2474..ae67d79418 100644 --- a/server/remote/github/parse_test.go +++ b/server/remote/github/parse_test.go @@ -25,37 +25,47 @@ import ( "github.com/woodpecker-ci/woodpecker/server/remote/github/fixtures" ) +const ( + hookEvent = "X-Github-Event" + hookDeploy = "deployment" + hookPush = "push" + hookPull = "pull_request" +) + +func testHookRequest(payload []byte, event string) *http.Request { + buf := bytes.NewBuffer(payload) + req, _ := http.NewRequest("POST", "/hook", buf) + req.Header = http.Header{} + req.Header.Set(hookEvent, "issues") + return req +} + func Test_parser(t *testing.T) { g := goblin.Goblin(t) g.Describe("GitHub parser", func() { g.It("should ignore unsupported hook events", func() { - buf := bytes.NewBufferString(fixtures.HookPullRequest) - req, _ := http.NewRequest("POST", "/hook", buf) - req.Header = http.Header{} - req.Header.Set(hookEvent, "issues") - - r, b, err := parseHook(req, false) + req := testHookRequest([]byte(fixtures.HookPullRequest), "issues") + p, r, b, err := parseHook(req, false, false) g.Assert(r).IsNil() g.Assert(b).IsNil() g.Assert(err).IsNil() + g.Assert(p).IsNil() }) g.Describe("given a push hook", func() { g.It("should skip when action is deleted", func() { - raw := []byte(fixtures.HookPushDeleted) - r, b, err := parsePushHook(raw) + req := testHookRequest([]byte(fixtures.HookPushDeleted), hookPush) + p, r, b, err := parseHook(req, false, false) g.Assert(r).IsNil() g.Assert(b).IsNil() g.Assert(err).IsNil() + g.Assert(p).IsNil() }) g.It("should extract repository and build details", func() { - buf := bytes.NewBufferString(fixtures.HookPush) - req, _ := http.NewRequest("POST", "/hook", buf) - req.Header = http.Header{} - req.Header.Set(hookEvent, hookPush) - - r, b, err := parseHook(req, false) + req := testHookRequest([]byte(fixtures.HookPush), hookPush) + p, r, b, err := parseHook(req, false, false) g.Assert(err).IsNil() + g.Assert(p).IsNil() g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() g.Assert(b.Event).Equal(model.EventPush) @@ -66,44 +76,40 @@ func Test_parser(t *testing.T) { g.Describe("given a pull request hook", func() { g.It("should skip when action is not open or sync", func() { - raw := []byte(fixtures.HookPullRequestInvalidAction) - r, b, err := parsePullHook(raw, false) + req := testHookRequest([]byte(fixtures.HookPullRequestInvalidAction), hookPull) + p, r, b, err := parseHook(req, false, false) g.Assert(r).IsNil() g.Assert(b).IsNil() g.Assert(err).IsNil() + g.Assert(p).IsNil() }) g.It("should skip when state is not open", func() { - raw := []byte(fixtures.HookPullRequestInvalidState) - r, b, err := parsePullHook(raw, false) + req := testHookRequest([]byte(fixtures.HookPullRequestInvalidState), hookPull) + p, r, b, err := parseHook(req, false, false) g.Assert(r).IsNil() g.Assert(b).IsNil() g.Assert(err).IsNil() + g.Assert(p).IsNil() }) g.It("should extract repository and build details", func() { - buf := bytes.NewBufferString(fixtures.HookPullRequest) - req, _ := http.NewRequest("POST", "/hook", buf) - req.Header = http.Header{} - req.Header.Set(hookEvent, hookPull) - - r, b, err := parseHook(req, false) + req := testHookRequest([]byte(fixtures.HookPullRequest), hookPull) + p, r, b, err := parseHook(req, false, false) g.Assert(err).IsNil() g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() + g.Assert(p).IsNotNil() g.Assert(b.Event).Equal(model.EventPull) }) }) g.Describe("given a deployment hook", func() { g.It("should extract repository and build details", func() { - buf := bytes.NewBufferString(fixtures.HookDeploy) - req, _ := http.NewRequest("POST", "/hook", buf) - req.Header = http.Header{} - req.Header.Set(hookEvent, hookDeploy) - - r, b, err := parseHook(req, false) + req := testHookRequest([]byte(fixtures.HookDeploy), hookDeploy) + p, r, b, err := parseHook(req, false, false) g.Assert(err).IsNil() g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() + g.Assert(p).IsNil() g.Assert(b.Event).Equal(model.EventDeploy) }) }) diff --git a/server/remote/github/types.go b/server/remote/github/types.go deleted file mode 100644 index 79a57c7017..0000000000 --- a/server/remote/github/types.go +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright 2018 Drone.IO Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package github - -type webhook struct { - Ref string `json:"ref"` - Action string `json:"action"` - Deleted bool `json:"deleted"` - BaseRef string `json:"base_ref"` - - Head struct { - ID string `json:"id"` - URL string `json:"url"` - Message string `json:"message"` - Timestamp string `json:"timestamp"` - - Author struct { - Name string `json:"name"` - Email string `json:"email"` - Username string `json:"username"` - } `json:"author"` - - Committer struct { - Name string `json:"name"` - Email string `json:"email"` - Username string `json:"username"` - } `json:"committer"` - - Added []string `json:"added"` - Removed []string `json:"removed"` - Modified []string `json:"modified"` - } `json:"head_commit"` - - Sender struct { - Login string `json:"login"` - Avatar string `json:"avatar_url"` - } `json:"sender"` - - // repository details - Repo struct { - Owner struct { - Login string `json:"login"` - Name string `json:"name"` - } `json:"owner"` - - Name string `json:"name"` - FullName string `json:"full_name"` - Language string `json:"language"` - Private bool `json:"private"` - HTMLURL string `json:"html_url"` - CloneURL string `json:"clone_url"` - DefaultBranch string `json:"default_branch"` - } `json:"repository"` - - // deployment hook details - Deployment struct { - ID int64 `json:"id"` - Sha string `json:"sha"` - Ref string `json:"ref"` - Task string `json:"task"` - Env string `json:"environment"` - URL string `json:"url"` - Desc string `json:"description"` - } `json:"deployment"` - - // pull request details - PullRequest struct { - Number int `json:"number"` - State string `json:"state"` - Title string `json:"title"` - HTMLURL string `json:"html_url"` - - User struct { - Login string `json:"login"` - Avatar string `json:"avatar_url"` - } `json:"user"` - - Base struct { - Ref string `json:"ref"` - } `json:"base"` - - Head struct { - SHA string `json:"sha"` - Ref string `json:"ref"` - Repo struct { - CloneURL string `json:"clone_url"` - } `json:"repo"` - } `json:"head"` - } `json:"pull_request"` -} From c8c823fc83bcbf66d055b85131bcfd4e446fec92 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 06:25:10 +0100 Subject: [PATCH 07/20] fix tests, harden converts, finish initial refactor --- server/remote/github/convert.go | 34 ++-- server/remote/github/convert_test.go | 226 ++++++++++++++------------- server/remote/github/github.go | 2 +- server/remote/github/parse.go | 2 +- server/remote/github/parse_test.go | 6 +- 5 files changed, 141 insertions(+), 129 deletions(-) diff --git a/server/remote/github/convert.go b/server/remote/github/convert.go index 10101a78dd..f839c8984c 100644 --- a/server/remote/github/convert.go +++ b/server/remote/github/convert.go @@ -82,19 +82,19 @@ func convertDesc(status model.StatusValue) string { // structure to the common Woodpecker repository structure. func convertRepo(from *github.Repository, private bool) *model.Repo { repo := &model.Repo{ - Owner: *from.Owner.Login, - Name: *from.Name, - FullName: *from.FullName, - Link: *from.HTMLURL, - IsSCMPrivate: *from.Private, - Clone: *from.CloneURL, - Avatar: *from.Owner.AvatarURL, + Name: from.GetName(), + FullName: from.GetFullName(), + Link: from.GetHTMLURL(), + IsSCMPrivate: from.GetPrivate(), + Clone: from.GetCloneURL(), + Branch: from.GetDefaultBranch(), + Owner: from.GetOwner().GetLogin(), + Avatar: from.GetOwner().GetAvatarURL(), + Perm: convertPerm(from.GetPermissions()), SCMKind: model.RepoGit, - Branch: defaultBranch, - Perm: convertPerm(from), } - if from.DefaultBranch != nil { - repo.Branch = *from.DefaultBranch + if len(repo.Branch) == 0 { + repo.Branch = defaultBranch } if private { repo.IsSCMPrivate = true @@ -104,11 +104,11 @@ func convertRepo(from *github.Repository, private bool) *model.Repo { // convertPerm is a helper function used to convert a GitHub repository // permissions to the common Woodpecker permissions structure. -func convertPerm(from *github.Repository) *model.Perm { +func convertPerm(perm map[string]bool) *model.Perm { return &model.Perm{ - Admin: from.Permissions["admin"], - Push: from.Permissions["push"], - Pull: from.Permissions["pull"], + Admin: perm["admin"], + Push: perm["push"], + Pull: perm["pull"], } } @@ -136,8 +136,8 @@ func convertTeamList(from []*github.Organization) []*model.Team { // to the common Woodpecker repository structure. func convertTeam(from *github.Organization) *model.Team { return &model.Team{ - Login: *from.Login, - Avatar: *from.AvatarURL, + Login: from.GetLogin(), + Avatar: from.GetAvatarURL(), } } diff --git a/server/remote/github/convert_test.go b/server/remote/github/convert_test.go index 3c3d408b42..741b1e5d05 100644 --- a/server/remote/github/convert_test.go +++ b/server/remote/github/convert_test.go @@ -129,7 +129,7 @@ func Test_helper(t *testing.T) { }, } - to := convertPerm(from) + to := convertPerm(from.GetPermissions()) g.Assert(to.Push).IsTrue() g.Assert(to.Pull).IsTrue() g.Assert(to.Admin).IsTrue() @@ -158,7 +158,7 @@ func Test_helper(t *testing.T) { }) g.It("should convert a repository from webhook", func() { - from := &github.PushEventRepository{} + from := &github.PushEventRepository{Owner: &github.User{}} from.Owner.Login = github.String("octocat") from.Owner.Name = github.String("octocat") from.Name = github.String("hello-world") @@ -178,110 +178,122 @@ func Test_helper(t *testing.T) { g.Assert(repo.Branch).Equal(*from.DefaultBranch) }) - /* - - g.It("should convert a pull request from webhook", func() { - from := &github.PullRequestEvent{} - from.PullRequest.Base.Ref = github.String("master") - from.PullRequest.Head.Ref = github.String("changes") - from.PullRequest.Head.SHA = github.String("f72fc19") - from.PullRequest.Head.Repo.CloneURL = github.String("https://github.com/octocat/hello-world-fork") - from.PullRequest.HTMLURL = github.String("https://github.com/octocat/hello-world/pulls/42") - from.PullRequest.Number = 42 - from.PullRequest.Title = github.String("Updated README.md") - from.PullRequest.User.Login = github.String("octocat") - from.PullRequest.User.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") - from.Sender.Login = github.String("octocat") - - build := convertPullHook(from, true) - g.Assert(build.Event).Equal(model.EventPull) - g.Assert(build.Branch).Equal(*from.PullRequest.Base.Ref) - g.Assert(build.Ref).Equal("refs/pull/42/merge") - g.Assert(build.Refspec).Equal("changes:master") - g.Assert(build.Remote).Equal("https://github.com/octocat/hello-world-fork") - g.Assert(build.Commit).Equal(*from.PullRequest.Head.SHA) - g.Assert(build.Message).Equal(*from.PullRequest.Title) - g.Assert(build.Title).Equal(*from.PullRequest.Title) - g.Assert(build.Author).Equal(*from.PullRequest.User.Login) - g.Assert(build.Avatar).Equal(*from.PullRequest.User.AvatarURL) - g.Assert(build.Sender).Equal(*from.Sender.Login) - }) - - g.It("should convert a deployment from webhook", func() { - from := &webhook{} - from.Deployment.Desc = ":shipit:" - from.Deployment.Env = "production" - from.Deployment.ID = 42 - from.Deployment.Ref = "master" - from.Deployment.Sha = "f72fc19" - from.Deployment.URL = "https://github.com/octocat/hello-world" - from.Sender.Login = "octocat" - from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" - - build := convertDeployHook(from) - g.Assert(build.Event).Equal(model.EventDeploy) - g.Assert(build.Branch).Equal("master") - g.Assert(build.Ref).Equal("refs/heads/master") - g.Assert(build.Commit).Equal(from.Deployment.Sha) - g.Assert(build.Message).Equal(from.Deployment.Desc) - g.Assert(build.Link).Equal(from.Deployment.URL) - g.Assert(build.Author).Equal(from.Sender.Login) - g.Assert(build.Avatar).Equal(from.Sender.Avatar) - }) - - g.It("should convert a push from webhook", func() { - from := &webhook{} - from.Sender.Login = "octocat" - from.Sender.Avatar = "https://avatars1.githubusercontent.com/u/583231" - from.Repo.CloneURL = "https://github.com/octocat/hello-world.git" - from.Head.Author.Email = "octocat@github.com" - from.Head.Message = "updated README.md" - from.Head.URL = "https://github.com/octocat/hello-world" - from.Head.ID = "f72fc19" - from.Ref = "refs/heads/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventPush) - g.Assert(build.Branch).Equal("master") - g.Assert(build.Ref).Equal("refs/heads/master") - g.Assert(build.Commit).Equal(from.Head.ID) - g.Assert(build.Message).Equal(from.Head.Message) - g.Assert(build.Link).Equal(from.Head.URL) - g.Assert(build.Author).Equal(from.Sender.Login) - g.Assert(build.Avatar).Equal(from.Sender.Avatar) - g.Assert(build.Email).Equal(from.Head.Author.Email) - g.Assert(build.Remote).Equal(from.Repo.CloneURL) - }) - - g.It("should convert a tag from webhook", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Ref).Equal("refs/tags/v1.0.0") - }) - - g.It("should convert tag's base branch from webhook to build's branch ", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - from.BaseRef = "refs/heads/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Branch).Equal("master") - }) - - g.It("should not convert tag's base_ref from webhook if not prefixed with 'ref/heads/'", func() { - from := &webhook{} - from.Ref = "refs/tags/v1.0.0" - from.BaseRef = "refs/refs/master" - - build := convertPushHook(from) - g.Assert(build.Event).Equal(model.EventTag) - g.Assert(build.Branch).Equal("refs/tags/v1.0.0") - }) - - */ + g.It("should convert a pull request from webhook", func() { + from := &github.PullRequestEvent{ + Action: github.String(actionOpen), + PullRequest: &github.PullRequest{ + State: github.String(stateOpen), + Base: &github.PullRequestBranch{}, + Head: &github.PullRequestBranch{ + Repo: &github.Repository{}, + }, + User: &github.User{}, + }, Sender: &github.User{}} + from.PullRequest.Base.Ref = github.String("master") + from.PullRequest.Head.Ref = github.String("changes") + from.PullRequest.Head.SHA = github.String("f72fc19") + from.PullRequest.Head.Repo.CloneURL = github.String("https://github.com/octocat/hello-world-fork") + from.PullRequest.HTMLURL = github.String("https://github.com/octocat/hello-world/pulls/42") + from.PullRequest.Number = github.Int(42) + from.PullRequest.Title = github.String("Updated README.md") + from.PullRequest.User.Login = github.String("octocat") + from.PullRequest.User.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") + from.Sender.Login = github.String("octocat") + + pull, _, build, err := parsePullHook(from, true, false) + g.Assert(err).IsNil() + g.Assert(pull).IsNotNil() + g.Assert(build.Event).Equal(model.EventPull) + g.Assert(build.Branch).Equal(*from.PullRequest.Base.Ref) + g.Assert(build.Ref).Equal("refs/pull/42/merge") + g.Assert(build.Refspec).Equal("changes:master") + g.Assert(build.Remote).Equal("https://github.com/octocat/hello-world-fork") + g.Assert(build.Commit).Equal(*from.PullRequest.Head.SHA) + g.Assert(build.Message).Equal(*from.PullRequest.Title) + g.Assert(build.Title).Equal(*from.PullRequest.Title) + g.Assert(build.Author).Equal(*from.PullRequest.User.Login) + g.Assert(build.Avatar).Equal(*from.PullRequest.User.AvatarURL) + g.Assert(build.Sender).Equal(*from.Sender.Login) + }) + + g.It("should convert a deployment from webhook", func() { + from := &github.DeploymentEvent{Deployment: &github.Deployment{}, Sender: &github.User{}} + from.Deployment.Description = github.String(":shipit:") + from.Deployment.Environment = github.String("production") + from.Deployment.ID = github.Int64(42) + from.Deployment.Ref = github.String("master") + from.Deployment.SHA = github.String("f72fc19") + from.Deployment.URL = github.String("https://github.com/octocat/hello-world") + from.Sender.Login = github.String("octocat") + from.Sender.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") + + _, build, err := parseDeployHook(from, false) + g.Assert(err).IsNil() + g.Assert(build.Event).Equal(model.EventDeploy) + g.Assert(build.Branch).Equal("master") + g.Assert(build.Ref).Equal("refs/heads/master") + g.Assert(build.Commit).Equal(*from.Deployment.SHA) + g.Assert(build.Message).Equal(*from.Deployment.Description) + g.Assert(build.Link).Equal(*from.Deployment.URL) + g.Assert(build.Author).Equal(*from.Sender.Login) + g.Assert(build.Avatar).Equal(*from.Sender.AvatarURL) + }) + + g.It("should convert a push from webhook", func() { + from := &github.PushEvent{Sender: &github.User{}, Repo: &github.PushEventRepository{}, HeadCommit: &github.HeadCommit{Author: &github.CommitAuthor{}}} + from.Sender.Login = github.String("octocat") + from.Sender.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") + from.Repo.CloneURL = github.String("https://github.com/octocat/hello-world.git") + from.HeadCommit.Author.Email = github.String("github.String(octocat@github.com") + from.HeadCommit.Message = github.String("updated README.md") + from.HeadCommit.URL = github.String("https://github.com/octocat/hello-world") + from.HeadCommit.ID = github.String("f72fc19") + from.Ref = github.String("refs/heads/master") + + _, build, err := parsePushHook(from) + g.Assert(err).IsNil() + g.Assert(build.Event).Equal(model.EventPush) + g.Assert(build.Branch).Equal("master") + g.Assert(build.Ref).Equal("refs/heads/master") + g.Assert(build.Commit).Equal(*from.HeadCommit.ID) + g.Assert(build.Message).Equal(*from.HeadCommit.Message) + g.Assert(build.Link).Equal(*from.HeadCommit.URL) + g.Assert(build.Author).Equal(*from.Sender.Login) + g.Assert(build.Avatar).Equal(*from.Sender.AvatarURL) + g.Assert(build.Email).Equal(*from.HeadCommit.Author.Email) + g.Assert(build.Remote).Equal(*from.Repo.CloneURL) + }) + + g.It("should convert a tag from webhook", func() { + from := &github.PushEvent{} + from.Ref = github.String("refs/tags/v1.0.0") + + _, build, err := parsePushHook(from) + g.Assert(err).IsNil() + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Ref).Equal("refs/tags/v1.0.0") + }) + + g.It("should convert tag's base branch from webhook to build's branch ", func() { + from := &github.PushEvent{} + from.Ref = github.String("refs/tags/v1.0.0") + from.BaseRef = github.String("refs/heads/master") + + _, build, err := parsePushHook(from) + g.Assert(err).IsNil() + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Branch).Equal("master") + }) + + g.It("should not convert tag's base_ref from webhook if not prefixed with 'ref/heads/'", func() { + from := &github.PushEvent{} + from.Ref = github.String("refs/tags/v1.0.0") + from.BaseRef = github.String("refs/refs/master") + + _, build, err := parsePushHook(from) + g.Assert(err).IsNil() + g.Assert(build.Event).Equal(model.EventTag) + g.Assert(build.Branch).Equal("refs/tags/v1.0.0") + }) }) } diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 2949c0534f..b9113a42da 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -221,7 +221,7 @@ func (c *client) Perm(ctx context.Context, u *model.User, r *model.Repo) (*model if err != nil { return nil, err } - return convertPerm(repo), nil + return convertPerm(repo.GetPermissions()), nil } // File fetches the file from the GitHub repository and returns its contents. diff --git a/server/remote/github/parse.go b/server/remote/github/parse.go index ae9420bb85..46bfec9a35 100644 --- a/server/remote/github/parse.go +++ b/server/remote/github/parse.go @@ -172,7 +172,7 @@ func parsePullHook(hook *github.PullRequestEvent, merge, privateMode bool) (*git ), } if merge { - build.Ref = fmt.Sprintf(mergeRefs, hook.GetPullRequest().Number) + build.Ref = fmt.Sprintf(mergeRefs, hook.GetPullRequest().GetNumber()) } return hook.GetPullRequest(), convertRepo(hook.GetRepo(), privateMode), build, nil diff --git a/server/remote/github/parse_test.go b/server/remote/github/parse_test.go index ae67d79418..eedefad516 100644 --- a/server/remote/github/parse_test.go +++ b/server/remote/github/parse_test.go @@ -36,7 +36,7 @@ func testHookRequest(payload []byte, event string) *http.Request { buf := bytes.NewBuffer(payload) req, _ := http.NewRequest("POST", "/hook", buf) req.Header = http.Header{} - req.Header.Set(hookEvent, "issues") + req.Header.Set(hookEvent, event) return req } @@ -69,8 +69,8 @@ func Test_parser(t *testing.T) { g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() g.Assert(b.Event).Equal(model.EventPush) - expectedFiles := []string{"CHANGELOG.md", "app/controller/application.rb"} - g.Assert(b.ChangedFiles).Equal(expectedFiles) + // g.Assert(b.ChangedFiles).Equal([]string{"CHANGELOG.md", "app/controller/application.rb"}) + // TODO: use client.Hook to test parse & changed files }) }) From 315f92c2755d0c4e3a6ada68e61c658e6b5b4c52 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 06:51:58 +0100 Subject: [PATCH 08/20] update fixtures --- server/remote/github/fixtures/hooks.go | 255 ++++++++++++++++--- server/remote/github/fixtures/mock_server.go | 1 + server/remote/github/parse_test.go | 3 +- server/remote/gitlab/gitlab_test.go | 2 +- 4 files changed, 218 insertions(+), 43 deletions(-) create mode 100644 server/remote/github/fixtures/mock_server.go diff --git a/server/remote/github/fixtures/hooks.go b/server/remote/github/fixtures/hooks.go index adb4f70ed1..be0bfaff80 100644 --- a/server/remote/github/fixtures/hooks.go +++ b/server/remote/github/fixtures/hooks.go @@ -16,55 +16,230 @@ package fixtures // HookPush is a sample push hook. // https://developer.github.com/v3/activity/events/types/#pushevent -const HookPush = ` -{ - "ref": "refs/heads/changes", - "created": false, - "deleted": false, - "head_commit": { - "id": "0d1a26e67d8f5eaf1f6ba5c57fc3c7d91ac0fd1c", - "message": "Update README.md", - "timestamp": "2015-05-05T19:40:15-04:00", - "url": "https://github.com/baxterthehacker/public-repo/commit/0d1a26e67d8f5eaf1f6ba5c57fc3c7d91ac0fd1c", - "author": { - "name": "baxterthehacker", - "email": "baxterthehacker@users.noreply.github.com", - "username": "baxterthehacker" - }, - "committer": { - "name": "baxterthehacker", - "email": "baxterthehacker@users.noreply.github.com", - "username": "baxterthehacker" - }, - "added": ["CHANGELOG.md"], - "removed": [], - "modified": ["app/controller/application.rb"] - }, +const HookPush = `{ + "ref": "refs/heads/master", + "before": "2f780193b136b72bfea4eeb640786a8c4450c7a2", + "after": "366701fde727cb7a9e7f21eb88264f59f6f9b89c", "repository": { - "id": 35129377, - "name": "public-repo", - "full_name": "baxterthehacker/public-repo", + "id": 179344069, + "node_id": "MDEwOlJlcG9zaXRvcnkxNzkzNDQwNjk=", + "name": "woodpecker", + "full_name": "woodpecker-ci/woodpecker", + "private": false, "owner": { - "name": "baxterthehacker", - "email": "baxterthehacker@users.noreply.github.com" + "name": "woodpecker-ci", + "email": null, + "login": "woodpecker-ci", + "id": 84780935, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjg0NzgwOTM1", + "avatar_url": "https://avatars.githubusercontent.com/u/84780935?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/woodpecker-ci", + "html_url": "https://github.com/woodpecker-ci", + "followers_url": "https://api.github.com/users/woodpecker-ci/followers", + "following_url": "https://api.github.com/users/woodpecker-ci/following{/other_user}", + "gists_url": "https://api.github.com/users/woodpecker-ci/gists{/gist_id}", + "starred_url": "https://api.github.com/users/woodpecker-ci/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/woodpecker-ci/subscriptions", + "organizations_url": "https://api.github.com/users/woodpecker-ci/orgs", + "repos_url": "https://api.github.com/users/woodpecker-ci/repos", + "events_url": "https://api.github.com/users/woodpecker-ci/events{/privacy}", + "received_events_url": "https://api.github.com/users/woodpecker-ci/received_events", + "type": "Organization", + "site_admin": false }, - "private": false, - "html_url": "https://github.com/baxterthehacker/public-repo", - "default_branch": "master" + "html_url": "https://github.com/woodpecker-ci/woodpecker", + "description": "Woodpecker is a community fork of the Drone CI system.", + "fork": false, + "url": "https://github.com/woodpecker-ci/woodpecker", + "forks_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/forks", + "keys_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/teams", + "hooks_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/hooks", + "issue_events_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/issues/events{/number}", + "events_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/events", + "assignees_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/assignees{/user}", + "branches_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/branches{/branch}", + "tags_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/tags", + "blobs_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/statuses/{sha}", + "languages_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/languages", + "stargazers_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/stargazers", + "contributors_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/contributors", + "subscribers_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/subscribers", + "subscription_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/subscription", + "commits_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/contents/{+path}", + "compare_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/merges", + "archive_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/downloads", + "issues_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/issues{/number}", + "pulls_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/pulls{/number}", + "milestones_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/milestones{/number}", + "notifications_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/labels{/name}", + "releases_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/releases{/id}", + "deployments_url": "https://api.github.com/repos/woodpecker-ci/woodpecker/deployments", + "created_at": 1554314798, + "updated_at": "2022-01-16T20:19:33Z", + "pushed_at": 1642370257, + "git_url": "git://github.com/woodpecker-ci/woodpecker.git", + "ssh_url": "git@github.com:woodpecker-ci/woodpecker.git", + "clone_url": "https://github.com/woodpecker-ci/woodpecker.git", + "svn_url": "https://github.com/woodpecker-ci/woodpecker", + "homepage": "https://woodpecker-ci.org", + "size": 81324, + "stargazers_count": 659, + "watchers_count": 659, + "language": "Go", + "has_issues": true, + "has_projects": false, + "has_downloads": true, + "has_wiki": false, + "has_pages": false, + "forks_count": 84, + "mirror_url": null, + "archived": false, + "disabled": false, + "open_issues_count": 123, + "license": { + "key": "apache-2.0", + "name": "Apache License 2.0", + "spdx_id": "Apache-2.0", + "url": "https://api.github.com/licenses/apache-2.0", + "node_id": "MDc6TGljZW5zZTI=" + }, + "allow_forking": true, + "is_template": false, + "topics": [ + "ci", + "devops", + "docker", + "hacktoberfest", + "hacktoberfest2021", + "woodpeckerci" + ], + "visibility": "public", + "forks": 84, + "open_issues": 123, + "watchers": 659, + "default_branch": "master", + "stargazers": 659, + "master_branch": "master", + "organization": "woodpecker-ci" }, "pusher": { - "name": "baxterthehacker", - "email": "baxterthehacker@users.noreply.github.com" + "name": "6543", + "email": "noreply@6543.de" + }, + "organization": { + "login": "woodpecker-ci", + "id": 84780935, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjg0NzgwOTM1", + "url": "https://api.github.com/orgs/woodpecker-ci", + "repos_url": "https://api.github.com/orgs/woodpecker-ci/repos", + "events_url": "https://api.github.com/orgs/woodpecker-ci/events", + "hooks_url": "https://api.github.com/orgs/woodpecker-ci/hooks", + "issues_url": "https://api.github.com/orgs/woodpecker-ci/issues", + "members_url": "https://api.github.com/orgs/woodpecker-ci/members{/member}", + "public_members_url": "https://api.github.com/orgs/woodpecker-ci/public_members{/member}", + "avatar_url": "https://avatars.githubusercontent.com/u/84780935?v=4", + "description": "Woodpecker is a community fork of the Drone CI system." }, "sender": { - "login": "baxterthehacker", - "avatar_url": "https://avatars.githubusercontent.com/u/6752317?v=3" + "login": "6543", + "id": 24977596, + "node_id": "MDQ6VXNlcjI0OTc3NTk2", + "avatar_url": "https://avatars.githubusercontent.com/u/24977596?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/6543", + "html_url": "https://github.com/6543", + "followers_url": "https://api.github.com/users/6543/followers", + "following_url": "https://api.github.com/users/6543/following{/other_user}", + "gists_url": "https://api.github.com/users/6543/gists{/gist_id}", + "starred_url": "https://api.github.com/users/6543/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/6543/subscriptions", + "organizations_url": "https://api.github.com/users/6543/orgs", + "repos_url": "https://api.github.com/users/6543/repos", + "events_url": "https://api.github.com/users/6543/events{/privacy}", + "received_events_url": "https://api.github.com/users/6543/received_events", + "type": "User", + "site_admin": false + }, + "created": false, + "deleted": false, + "forced": false, + "base_ref": null, + "compare": "https://github.com/woodpecker-ci/woodpecker/compare/2f780193b136...366701fde727", + "commits": [ + { + "id": "366701fde727cb7a9e7f21eb88264f59f6f9b89c", + "tree_id": "638e046f1e1e15dbed1ddf40f9471bf1af4d64ce", + "distinct": true, + "message": "Fix multiline secrets replacer (#700)\n\n* Fix multiline secrets replacer\r\n\r\n* Add tests", + "timestamp": "2022-01-16T22:57:37+01:00", + "url": "https://github.com/woodpecker-ci/woodpecker/commit/366701fde727cb7a9e7f21eb88264f59f6f9b89c", + "author": { + "name": "Philipp", + "email": "noreply@philipp.xzy", + "username": "nupplaphil" + }, + "committer": { + "name": "GitHub", + "email": "noreply@github.com", + "username": "web-flow" + }, + "added": [ + + ], + "removed": [ + + ], + "modified": [ + "pipeline/shared/replace_secrets.go", + "pipeline/shared/replace_secrets_test.go" + ] + } + ], + "head_commit": { + "id": "366701fde727cb7a9e7f21eb88264f59f6f9b89c", + "tree_id": "638e046f1e1e15dbed1ddf40f9471bf1af4d64ce", + "distinct": true, + "message": "Fix multiline secrets replacer (#700)\n\n* Fix multiline secrets replacer\r\n\r\n* Add tests", + "timestamp": "2022-01-16T22:57:37+01:00", + "url": "https://github.com/woodpecker-ci/woodpecker/commit/366701fde727cb7a9e7f21eb88264f59f6f9b89c", + "author": { + "name": "Philipp", + "email": "admin@philipp.info", + "username": "nupplaphil" + }, + "committer": { + "name": "GitHub", + "email": "noreply@github.com", + "username": "web-flow" + }, + "added": [ + + ], + "removed": [ + + ], + "modified": [ + "pipeline/shared/replace_secrets.go", + "pipeline/shared/replace_secrets_test.go" + ] } -} -` +}` -// HookPush is a sample push hook that is marked as deleted, and is expected to -// be ignored. +// HookPushDeleted is a sample push hook that is marked as deleted, and is expected to be ignored. const HookPushDeleted = ` { "deleted": true diff --git a/server/remote/github/fixtures/mock_server.go b/server/remote/github/fixtures/mock_server.go new file mode 100644 index 0000000000..331393d4e4 --- /dev/null +++ b/server/remote/github/fixtures/mock_server.go @@ -0,0 +1 @@ +package fixtures diff --git a/server/remote/github/parse_test.go b/server/remote/github/parse_test.go index eedefad516..3d13d70be6 100644 --- a/server/remote/github/parse_test.go +++ b/server/remote/github/parse_test.go @@ -69,8 +69,7 @@ func Test_parser(t *testing.T) { g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() g.Assert(b.Event).Equal(model.EventPush) - // g.Assert(b.ChangedFiles).Equal([]string{"CHANGELOG.md", "app/controller/application.rb"}) - // TODO: use client.Hook to test parse & changed files + g.Assert(b.ChangedFiles).Equal([]string{"pipeline/shared/replace_secrets.go", "pipeline/shared/replace_secrets_test.go"}) }) }) diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index faa43b3037..55819c013c 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -51,7 +51,7 @@ func load(t *testing.T, config string) *Gitlab { } func Test_Gitlab(t *testing.T) { - // setup a dummy github server + // setup a dummy gitlab server server := testdata.NewServer(t) defer server.Close() From af35429e546c62cf2265f04ef737a0b5f19819ef Mon Sep 17 00:00:00 2001 From: Anton Bracke Date: Mon, 17 Jan 2022 17:54:35 +0100 Subject: [PATCH 09/20] fix gitlab repo loading and pull-request id getting --- server/remote/github/github.go | 4 +-- server/remote/gitlab/convert.go | 20 +++++------- server/remote/gitlab/gitlab.go | 56 +++++++++++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 17 deletions(-) diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 8d07b4e7f3..d98f8c98de 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -517,8 +517,8 @@ func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, repo *mode client := c.newClientToken(ctx, user.Token) // extract id from ref: "refs/pull/%d/merge" - pullRequestID, err := strconv.Atoi(build.Ref) - if err != nil { + var pullRequestID int + if _, err := fmt.Sscanf(build.Ref, mergeRefs, &pullRequestID); err != nil { return nil, err } diff --git a/server/remote/gitlab/convert.go b/server/remote/gitlab/convert.go index 0a32c2d258..4919474a4c 100644 --- a/server/remote/gitlab/convert.go +++ b/server/remote/gitlab/convert.go @@ -27,6 +27,10 @@ import ( "github.com/woodpecker-ci/woodpecker/shared/utils" ) +const ( + mergeRefs = "refs/merge-requests/%d/head" // merge request merged with base +) + func (g *Gitlab) convertGitlabRepo(_repo *gitlab.Project) (*model.Repo, error) { parts := strings.Split(_repo.PathWithNamespace, "/") // TODO(648) save repo id (support nested repos) @@ -60,7 +64,7 @@ func (g *Gitlab) convertGitlabRepo(_repo *gitlab.Project) (*model.Repo, error) { return repo, nil } -func convertMergeRequestHock(hook *gitlab.MergeEvent, req *http.Request) (*model.Repo, *model.Build, error) { +func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (*model.Repo, *model.Build, error) { repo := &model.Repo{} build := &model.Build{} @@ -114,7 +118,7 @@ func convertMergeRequestHock(hook *gitlab.MergeEvent, req *http.Request) (*model build.Commit = lastCommit.ID build.Remote = obj.Source.HTTPURL - build.Ref = fmt.Sprintf("refs/merge-requests/%d/head", obj.IID) + build.Ref = fmt.Sprintf(mergeRefs, obj.IID) build.Branch = obj.SourceBranch author := lastCommit.Author @@ -129,18 +133,10 @@ func convertMergeRequestHock(hook *gitlab.MergeEvent, req *http.Request) (*model build.Title = obj.Title build.Link = obj.URL - if hook.ObjectAttributes.StDiffs != nil { - files := make([]string, 0, len(hook.ObjectAttributes.StDiffs)*2) - for _, diff := range hook.ObjectAttributes.StDiffs { - files = append(files, diff.OldPath, diff.NewPath) - } - build.ChangedFiles = utils.DedupStrings(files) - } - return repo, build, nil } -func convertPushHock(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) { +func convertPushHook(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) { repo := &model.Repo{} build := &model.Build{} @@ -190,7 +186,7 @@ func convertPushHock(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) return repo, build, nil } -func convertTagHock(hook *gitlab.TagEvent) (*model.Repo, *model.Build, error) { +func convertTagHook(hook *gitlab.TagEvent) (*model.Repo, *model.Build, error) { repo := &model.Repo{} build := &model.Build{} diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index 775cd85597..0e8aa00a88 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -31,7 +31,9 @@ import ( "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/common" + "github.com/woodpecker-ci/woodpecker/server/store" "github.com/woodpecker-ci/woodpecker/shared/oauth2" + "github.com/woodpecker-ci/woodpecker/shared/utils" ) const ( @@ -538,12 +540,60 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod switch event := parsed.(type) { case *gitlab.MergeEvent: - return convertMergeRequestHock(event, req) + repo, build, err := convertMergeRequestHook(event, req) + if err != nil { + return nil, nil, err + } + + build, err = g.loadChangedFilesFromMergeRequest(ctx, repo, build) + if err != nil { + return nil, nil, err + } + + return repo, build, nil case *gitlab.PushEvent: - return convertPushHock(event) + return convertPushHook(event) case *gitlab.TagEvent: - return convertTagHock(event) + return convertTagHook(event) default: return nil, nil, nil } } + +func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo *model.Repo, build *model.Build) (*model.Build, error) { + _store := store.FromContext(ctx) + + repo, err := _store.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name) + if err != nil { + return nil, err + } + + user, err := _store.GetUser(repo.UserID) + if err != nil { + return nil, err + } + + client, err := newClient(g.URL, user.Token, g.SkipVerify) + if err != nil { + return nil, err + } + + // extract id from ref: "refs/merge-requests/%d/head" + var pullRequestID int + if _, err := fmt.Sscanf(build.Ref, mergeRefs, &pullRequestID); err != nil { + return nil, err + } + + changes, _, err := client.MergeRequests.GetMergeRequestChanges(repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) + if err != nil { + return nil, err + } + + files := build.ChangedFiles + for _, file := range changes.Changes { + files = append(files, file.NewPath) + } + build.ChangedFiles = utils.DedupStrings(files) + + return build, nil +} From e8f22fc6ffdc277a57e8bbbb947c976f66b282fe Mon Sep 17 00:00:00 2001 From: Anton Bracke Date: Mon, 17 Jan 2022 18:21:30 +0100 Subject: [PATCH 10/20] fix repo id loading --- server/remote/gitlab/gitlab.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index 0e8aa00a88..200320a1af 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -584,7 +584,12 @@ func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo * return nil, err } - changes, _, err := client.MergeRequests.GetMergeRequestChanges(repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) + _repo, err := g.getProject(ctx, client, repo.Owner, repo.Name) + if err != nil { + return nil, err + } + + changes, _, err := client.MergeRequests.GetMergeRequestChanges(_repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) if err != nil { return nil, err } From 8e1ea2b3b9eb1912f9195368ea00c6f7a3979bdc Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 20:59:59 +0100 Subject: [PATCH 11/20] run go generate --- server/remote/mocks/remote.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/server/remote/mocks/remote.go b/server/remote/mocks/remote.go index e391b98e20..3fc5544ee2 100644 --- a/server/remote/mocks/remote.go +++ b/server/remote/mocks/remote.go @@ -136,13 +136,13 @@ func (_m *Remote) File(ctx context.Context, u *model.User, r *model.Repo, b *mod return r0, r1 } -// Hook provides a mock function with given fields: r +// Hook provides a mock function with given fields: ctx, r func (_m *Remote) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model.Build, error) { - ret := _m.Called(r) + ret := _m.Called(ctx, r) var r0 *model.Repo - if rf, ok := ret.Get(0).(func(*http.Request) *model.Repo); ok { - r0 = rf(r) + if rf, ok := ret.Get(0).(func(context.Context, *http.Request) *model.Repo); ok { + r0 = rf(ctx, r) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.Repo) @@ -150,8 +150,8 @@ func (_m *Remote) Hook(ctx context.Context, r *http.Request) (*model.Repo, *mode } var r1 *model.Build - if rf, ok := ret.Get(1).(func(*http.Request) *model.Build); ok { - r1 = rf(r) + if rf, ok := ret.Get(1).(func(context.Context, *http.Request) *model.Build); ok { + r1 = rf(ctx, r) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.Build) @@ -159,8 +159,8 @@ func (_m *Remote) Hook(ctx context.Context, r *http.Request) (*model.Repo, *mode } var r2 error - if rf, ok := ret.Get(2).(func(*http.Request) error); ok { - r2 = rf(r) + if rf, ok := ret.Get(2).(func(context.Context, *http.Request) error); ok { + r2 = rf(ctx, r) } else { r2 = ret.Error(2) } From 0d04f1304553d2dbac51a1b2bfc7a9f5bae322bc Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 21:16:15 +0100 Subject: [PATCH 12/20] add tests --- shared/utils/strings.go | 14 +++++++++++ shared/utils/strings_test.go | 48 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 shared/utils/strings_test.go diff --git a/shared/utils/strings.go b/shared/utils/strings.go index 0b6ddcdc44..6145d5db8f 100644 --- a/shared/utils/strings.go +++ b/shared/utils/strings.go @@ -1,3 +1,17 @@ +// Copyright 2022 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package utils // DedupStrings deduplicate string list, empty items are dropped diff --git a/shared/utils/strings_test.go b/shared/utils/strings_test.go new file mode 100644 index 0000000000..393e56cd86 --- /dev/null +++ b/shared/utils/strings_test.go @@ -0,0 +1,48 @@ +// Copyright 2022 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package utils + +import ( + "sort" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDedupStrings(t *testing.T) { + tests := []struct { + in []string + out []string + }{{ + in: []string{"", "ab", "12", "ab"}, + out: []string{"12", "ab"}, + }, { + in: nil, + out: nil, + }, { + in: []string{""}, + out: nil, + }} + + for _, tc := range tests { + result := DedupStrings(tc.in) + sort.Strings(result) + if len(tc.out) == 0 { + assert.Len(t, result, 0) + } else { + assert.EqualValues(t, tc.out, result, "could not correctly process input '%#v'", tc.in) + } + } +} From 56df0d46021092d9c60d3ac55f8c66692570094f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:05:03 +0100 Subject: [PATCH 13/20] harden --- server/remote/github/github.go | 14 ++++++++++++-- server/remote/gitlab/convert.go | 12 ++++++------ server/remote/gitlab/gitlab.go | 22 +++++++++------------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/server/remote/github/github.go b/server/remote/github/github.go index b9113a42da..8909696bcd 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -509,8 +509,18 @@ func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model return repo, build, nil } -func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, pull *github.PullRequest, repo *model.Repo, build *model.Build) (*model.Build, error) { - user, err := store.FromContext(ctx).GetUser(repo.UserID) +func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, pull *github.PullRequest, tmpRepo *model.Repo, build *model.Build) (*model.Build, error) { + _store := store.FromContext(ctx) + if _store == nil { + return build, nil + } + + repo, err := _store.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name) + if err != nil { + return nil, err + } + + user, err := _store.GetUser(repo.UserID) if err != nil { return nil, err } diff --git a/server/remote/gitlab/convert.go b/server/remote/gitlab/convert.go index 4919474a4c..dc204c057f 100644 --- a/server/remote/gitlab/convert.go +++ b/server/remote/gitlab/convert.go @@ -64,7 +64,7 @@ func (g *Gitlab) convertGitlabRepo(_repo *gitlab.Project) (*model.Repo, error) { return repo, nil } -func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (*model.Repo, *model.Build, error) { +func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (int, *model.Repo, *model.Build, error) { repo := &model.Repo{} build := &model.Build{} @@ -73,17 +73,17 @@ func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (*model obj := hook.ObjectAttributes if target == nil && source == nil { - return nil, nil, fmt.Errorf("target and source keys expected in merge request hook") + return 0, nil, nil, fmt.Errorf("target and source keys expected in merge request hook") } else if target == nil { - return nil, nil, fmt.Errorf("target key expected in merge request hook") + return 0, nil, nil, fmt.Errorf("target key expected in merge request hook") } else if source == nil { - return nil, nil, fmt.Errorf("source key expected in merge request hook") + return 0, nil, nil, fmt.Errorf("source key expected in merge request hook") } if target.PathWithNamespace != "" { var err error if repo.Owner, repo.Name, err = extractFromPath(target.PathWithNamespace); err != nil { - return nil, nil, err + return 0, nil, nil, err } repo.FullName = target.PathWithNamespace } else { @@ -133,7 +133,7 @@ func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (*model build.Title = obj.Title build.Link = obj.URL - return repo, build, nil + return obj.IID, repo, build, nil } func convertPushHook(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) { diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index 200320a1af..ae9fbf6232 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -540,13 +540,12 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod switch event := parsed.(type) { case *gitlab.MergeEvent: - repo, build, err := convertMergeRequestHook(event, req) + mergeIID, repo, build, err := convertMergeRequestHook(event, req) if err != nil { return nil, nil, err } - build, err = g.loadChangedFilesFromMergeRequest(ctx, repo, build) - if err != nil { + if build, err = g.loadChangedFilesFromMergeRequest(ctx, repo, build, mergeIID); err != nil { return nil, nil, err } @@ -560,8 +559,11 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod } } -func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo *model.Repo, build *model.Build) (*model.Build, error) { +func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo *model.Repo, build *model.Build, mergeIID int) (*model.Build, error) { _store := store.FromContext(ctx) + if _store == nil { + return build, nil + } repo, err := _store.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name) if err != nil { @@ -578,25 +580,19 @@ func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo * return nil, err } - // extract id from ref: "refs/merge-requests/%d/head" - var pullRequestID int - if _, err := fmt.Sscanf(build.Ref, mergeRefs, &pullRequestID); err != nil { - return nil, err - } - _repo, err := g.getProject(ctx, client, repo.Owner, repo.Name) if err != nil { return nil, err } - changes, _, err := client.MergeRequests.GetMergeRequestChanges(_repo.ID, pullRequestID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) + changes, _, err := client.MergeRequests.GetMergeRequestChanges(_repo.ID, mergeIID, &gitlab.GetMergeRequestChangesOptions{}, gitlab.WithContext(ctx)) if err != nil { return nil, err } - files := build.ChangedFiles + files := make([]string, 0, len(changes.Changes)*2) for _, file := range changes.Changes { - files = append(files, file.NewPath) + files = append(files, file.NewPath, file.OldPath) } build.ChangedFiles = utils.DedupStrings(files) From 59665485ae74000ed34ee66cbc86216dc08ea7a7 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:09:22 +0100 Subject: [PATCH 14/20] harden more --- server/remote/github/github.go | 4 ++-- server/remote/gitlab/gitlab.go | 4 ++-- server/store/context.go | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 8909696bcd..976f86d9e5 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -510,8 +510,8 @@ func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model } func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, pull *github.PullRequest, tmpRepo *model.Repo, build *model.Build) (*model.Build, error) { - _store := store.FromContext(ctx) - if _store == nil { + _store, ok := store.TryFromContext(ctx) + if !ok { return build, nil } diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index ae9fbf6232..28b6edbe70 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -560,8 +560,8 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod } func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo *model.Repo, build *model.Build, mergeIID int) (*model.Build, error) { - _store := store.FromContext(ctx) - if _store == nil { + _store, ok := store.TryFromContext(ctx) + if !ok { return build, nil } diff --git a/server/store/context.go b/server/store/context.go index 4f3fc2d2b7..1ac117de17 100644 --- a/server/store/context.go +++ b/server/store/context.go @@ -30,6 +30,12 @@ func FromContext(c context.Context) Store { return c.Value(key).(Store) } +// TryFromContext try to return the Store associated with this context. +func TryFromContext(c context.Context) (Store, bool) { + store, ok := c.Value(key).(Store) + return store, ok +} + // ToContext adds the Store to this context if it supports // the Setter interface. func ToContext(c Setter, store Store) { From b1160a684baeafe58bd67e5aaaa27b36e621905a Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:18:26 +0100 Subject: [PATCH 15/20] update test data --- server/remote/gitlab/gitlab_test.go | 6 +- server/remote/gitlab/testdata/hooks.go | 169 +++++++++++++------------ 2 files changed, 93 insertions(+), 82 deletions(-) diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index 55819c013c..36ba269381 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -210,7 +210,7 @@ func Test_Gitlab(t *testing.T) { req, _ := http.NewRequest( testdata.ServiceHookMethod, testdata.ServiceHookURL.String(), - bytes.NewReader(testdata.ServiceHookMergeRequestBody), // TODO: update test data with new hooks + bytes.NewReader(testdata.WebhookMergeRequestBody), ) req.Header = testdata.ServiceHookHeaders @@ -218,8 +218,8 @@ func Test_Gitlab(t *testing.T) { assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { assert.Equal(t, "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", hookRepo.Avatar) - assert.Equal(t, "develop", hookRepo.Branch) - assert.Equal(t, "test", hookRepo.Owner) + assert.Equal(t, "main", hookRepo.Branch) + assert.Equal(t, "anbraten", hookRepo.Owner) assert.Equal(t, "woodpecker", hookRepo.Name) assert.Equal(t, "Update client.go 🎉", build.Title) assert.Len(t, build.ChangedFiles, 0) // TODO: update test data with new hooks diff --git a/server/remote/gitlab/testdata/hooks.go b/server/remote/gitlab/testdata/hooks.go index f691abfdc1..7d39306b62 100644 --- a/server/remote/gitlab/testdata/hooks.go +++ b/server/remote/gitlab/testdata/hooks.go @@ -169,45 +169,45 @@ var ServiceHookTagPushBody = []byte(`{ } }`) -// ServiceHookMergeRequestBody is payload of ServiceHook: MergeRequest -var ServiceHookMergeRequestBody = []byte(`{ +// WebhookMergeRequestBody is payload of MergeEvent +var WebhookMergeRequestBody = []byte(`{ "object_kind": "merge_request", "event_type": "merge_request", "user": { - "id": 2, - "name": "the test", - "username": "test", - "avatar_url": "https://www.gravatar.com/avatar/dd46a756faad4727fb679320751f6dea?s=80&d=identicon", - "email": "test@test.test" + "id": 2251488, + "name": "Anbraten", + "username": "anbraten", + "avatar_url": "https://secure.gravatar.com/avatar/fc9b6fe77c6b732a02925a62a81f05a0?s=80&d=identicon", + "email": "some@mail.info" }, "project": { - "id": 2, - "name": "Woodpecker", + "id": 32059612, + "name": "woodpecker", "description": "", - "web_url": "http://10.40.8.5:3200/test/woodpecker", - "avatar_url": null, - "git_ssh_url": "git@10.40.8.5:test/woodpecker.git", - "git_http_url": "http://10.40.8.5:3200/test/woodpecker.git", - "namespace": "the test", + "web_url": "https://gitlab.com/anbraten/woodpecker", + "avatar_url": "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", + "git_ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "git_http_url": "https://gitlab.com/anbraten/woodpecker.git", + "namespace": "Anbraten", "visibility_level": 20, - "path_with_namespace": "test/woodpecker", - "default_branch": "master", - "ci_config_path": null, - "homepage": "http://10.40.8.5:3200/test/woodpecker", - "url": "git@10.40.8.5:test/woodpecker.git", - "ssh_url": "git@10.40.8.5:test/woodpecker.git", - "http_url": "http://10.40.8.5:3200/test/woodpecker.git" + "path_with_namespace": "anbraten/woodpecker", + "default_branch": "main", + "ci_config_path": "", + "homepage": "https://gitlab.com/anbraten/woodpecker", + "url": "git@gitlab.com:anbraten/woodpecker.git", + "ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "http_url": "https://gitlab.com/anbraten/woodpecker.git" }, "object_attributes": { - "assignee_id": null, - "author_id": 2, - "created_at": "2021-09-27 05:00:01 UTC", + "assignee_id": 2251488, + "author_id": 2251488, + "created_at": "2022-01-10 15:23:41 UTC", "description": "", - "head_pipeline_id": 5, - "id": 2, - "iid": 2, - "last_edited_at": null, - "last_edited_by_id": null, + "head_pipeline_id": 449733536, + "id": 134400602, + "iid": 3, + "last_edited_at": "2022-01-17 15:46:23 UTC", + "last_edited_by_id": 2251488, "merge_commit_sha": null, "merge_error": null, "merge_params": { @@ -217,61 +217,61 @@ var ServiceHookMergeRequestBody = []byte(`{ "merge_user_id": null, "merge_when_pipeline_succeeds": false, "milestone_id": null, - "source_branch": "masterfdsafds", - "source_project_id": 2, + "source_branch": "anbraten-main-patch-05373", + "source_project_id": 32059612, "state_id": 1, - "target_branch": "master", - "target_project_id": 2, + "target_branch": "main", + "target_project_id": 32059612, "time_estimate": 0, "title": "Update client.go 🎉", - "updated_at": "2021-09-27 05:01:21 UTC", - "updated_by_id": null, - "url": "http://10.40.8.5:3200/test/woodpecker/-/merge_requests/2", + "updated_at": "2022-01-17 15:47:39 UTC", + "updated_by_id": 2251488, + "url": "https://gitlab.com/anbraten/woodpecker/-/merge_requests/3", "source": { - "id": 2, - "name": "Woodpecker", + "id": 32059612, + "name": "woodpecker", "description": "", - "web_url": "http://10.40.8.5:3200/test/woodpecker", - "avatar_url": "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", - "git_ssh_url": "git@10.40.8.5:test/woodpecker.git", - "git_http_url": "http://10.40.8.5:3200/test/woodpecker.git", - "namespace": "the test", + "web_url": "https://gitlab.com/anbraten/woodpecker", + "avatar_url": null, + "git_ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "git_http_url": "https://gitlab.com/anbraten/woodpecker.git", + "namespace": "Anbraten", "visibility_level": 20, - "path_with_namespace": "test/woodpecker", - "default_branch": "develop", - "ci_config_path": null, - "homepage": "http://10.40.8.5:3200/test/woodpecker", - "url": "git@10.40.8.5:test/woodpecker.git", - "ssh_url": "git@10.40.8.5:test/woodpecker.git", - "http_url": "http://10.40.8.5:3200/test/woodpecker.git" + "path_with_namespace": "anbraten/woodpecker", + "default_branch": "main", + "ci_config_path": "", + "homepage": "https://gitlab.com/anbraten/woodpecker", + "url": "git@gitlab.com:anbraten/woodpecker.git", + "ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "http_url": "https://gitlab.com/anbraten/woodpecker.git" }, "target": { - "id": 2, - "name": "Woodpecker", + "id": 32059612, + "name": "woodpecker", "description": "", - "web_url": "http://10.40.8.5:3200/test/woodpecker", + "web_url": "https://gitlab.com/anbraten/woodpecker", "avatar_url": "http://example.com/uploads/project/avatar/555/Outh-20-Logo.jpg", - "git_ssh_url": "git@10.40.8.5:test/woodpecker.git", - "git_http_url": "http://10.40.8.5:3200/test/woodpecker.git", - "namespace": "the test", + "git_ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "git_http_url": "https://gitlab.com/anbraten/woodpecker.git", + "namespace": "Anbraten", "visibility_level": 20, - "path_with_namespace": "test/woodpecker", - "default_branch": "develop", - "ci_config_path": null, - "homepage": "http://10.40.8.5:3200/test/woodpecker", - "url": "git@10.40.8.5:test/woodpecker.git", - "ssh_url": "git@10.40.8.5:test/woodpecker.git", - "http_url": "http://10.40.8.5:3200/test/woodpecker.git" + "path_with_namespace": "anbraten/woodpecker", + "default_branch": "main", + "ci_config_path": "", + "homepage": "https://gitlab.com/anbraten/woodpecker", + "url": "git@gitlab.com:anbraten/woodpecker.git", + "ssh_url": "git@gitlab.com:anbraten/woodpecker.git", + "http_url": "https://gitlab.com/anbraten/woodpecker.git" }, "last_commit": { - "id": "0ab96a10266b95b4b533dcfd98738015fbe70889", - "message": "Update state.go", - "title": "Update state.go", - "timestamp": "2021-09-27T05:01:20+00:00", - "url": "http://10.40.8.5:3200/test/woodpecker/-/commit/0ab96a10266b95b4b533dcfd98738015fbe70889", + "id": "c136499ec574e1034b24c5d306de9acda3005367", + "message": "Update folder/todo.txt", + "title": "Update folder/todo.txt", + "timestamp": "2022-01-17T15:47:38+00:00", + "url": "https://gitlab.com/anbraten/woodpecker/-/commit/c136499ec574e1034b24c5d306de9acda3005367", "author": { - "name": "the test", - "email": "test@test.test" + "name": "Anbraten", + "email": "some@mail.info" } }, "work_in_progress": false, @@ -281,25 +281,36 @@ var ServiceHookMergeRequestBody = []byte(`{ "human_time_change": null, "human_time_estimate": null, "assignee_ids": [ - + 2251488 ], "state": "opened", + "blocking_discussions_resolved": true, "action": "update", - "oldrev": "6ef047571374c96a2bf13c361efd1fb008b0063e" + "oldrev": "8b641937b7340066d882b9d8a8cc5b0573a207de" }, "labels": [ ], "changes": { "updated_at": { - "previous": "2021-09-27 05:00:01 UTC", - "current": "2021-09-27 05:01:21 UTC" + "previous": "2022-01-17 15:46:23 UTC", + "current": "2022-01-17 15:47:39 UTC" } }, "repository": { - "name": "Woodpecker", - "url": "git@10.40.8.5:test/woodpecker.git", + "name": "woodpecker", + "url": "git@gitlab.com:anbraten/woodpecker.git", "description": "", - "homepage": "http://10.40.8.5:3200/test/woodpecker" - } -}`) + "homepage": "https://gitlab.com/anbraten/woodpecker" + }, + "assignees": [ + { + "id": 2251488, + "name": "Anbraten", + "username": "anbraten", + "avatar_url": "https://secure.gravatar.com/avatar/fc9b6fe77c6b732a02925a62a81f05a0?s=80&d=identicon", + "email": "some@mail.info" + } + ] +} +`) From 4d1a1b0893bbdff7625673e61fc5b6e8d4de069a Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:25:55 +0100 Subject: [PATCH 16/20] update todo --- server/remote/gitlab/gitlab_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index 36ba269381..f858e291c9 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -214,6 +214,7 @@ func Test_Gitlab(t *testing.T) { ) req.Header = testdata.ServiceHookHeaders + // TODO: insert fake store into context to retrieve user & repo, this will activate fetching of changed fiels hookRepo, build, err := client.Hook(ctx, req) assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { @@ -222,7 +223,7 @@ func Test_Gitlab(t *testing.T) { assert.Equal(t, "anbraten", hookRepo.Owner) assert.Equal(t, "woodpecker", hookRepo.Name) assert.Equal(t, "Update client.go 🎉", build.Title) - assert.Len(t, build.ChangedFiles, 0) // TODO: update test data with new hooks + assert.Len(t, build.ChangedFiles, 0) // see L217 } }) }) From b7a5c91091d3120f434631f7eb7df4b7d79860db Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:39:07 +0100 Subject: [PATCH 17/20] lint & harden tests --- server/remote/github/convert_test.go | 36 +++++++++++++++------------- server/remote/github/parse_test.go | 2 ++ server/remote/gitlab/gitlab_test.go | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/server/remote/github/convert_test.go b/server/remote/github/convert_test.go index 741b1e5d05..0b742d7d3b 100644 --- a/server/remote/github/convert_test.go +++ b/server/remote/github/convert_test.go @@ -182,24 +182,28 @@ func Test_helper(t *testing.T) { from := &github.PullRequestEvent{ Action: github.String(actionOpen), PullRequest: &github.PullRequest{ - State: github.String(stateOpen), - Base: &github.PullRequestBranch{}, + State: github.String(stateOpen), + HTMLURL: github.String("https://github.com/octocat/hello-world/pulls/42"), + Number: github.Int(42), + Title: github.String("Updated README.md"), + Base: &github.PullRequestBranch{ + Ref: github.String("master"), + }, Head: &github.PullRequestBranch{ - Repo: &github.Repository{}, + Ref: github.String("changes"), + SHA: github.String("f72fc19"), + Repo: &github.Repository{ + CloneURL: github.String("https://github.com/octocat/hello-world-fork"), + }, }, - User: &github.User{}, - }, Sender: &github.User{}} - from.PullRequest.Base.Ref = github.String("master") - from.PullRequest.Head.Ref = github.String("changes") - from.PullRequest.Head.SHA = github.String("f72fc19") - from.PullRequest.Head.Repo.CloneURL = github.String("https://github.com/octocat/hello-world-fork") - from.PullRequest.HTMLURL = github.String("https://github.com/octocat/hello-world/pulls/42") - from.PullRequest.Number = github.Int(42) - from.PullRequest.Title = github.String("Updated README.md") - from.PullRequest.User.Login = github.String("octocat") - from.PullRequest.User.AvatarURL = github.String("https://avatars1.githubusercontent.com/u/583231") - from.Sender.Login = github.String("octocat") - + User: &github.User{ + Login: github.String("octocat"), + AvatarURL: github.String("https://avatars1.githubusercontent.com/u/583231"), + }, + }, Sender: &github.User{ + Login: github.String("octocat"), + }, + } pull, _, build, err := parsePullHook(from, true, false) g.Assert(err).IsNil() g.Assert(pull).IsNotNil() diff --git a/server/remote/github/parse_test.go b/server/remote/github/parse_test.go index 3d13d70be6..0061b492fe 100644 --- a/server/remote/github/parse_test.go +++ b/server/remote/github/parse_test.go @@ -17,6 +17,7 @@ package github import ( "bytes" "net/http" + "sort" "testing" "github.com/franela/goblin" @@ -69,6 +70,7 @@ func Test_parser(t *testing.T) { g.Assert(r).IsNotNil() g.Assert(b).IsNotNil() g.Assert(b.Event).Equal(model.EventPush) + sort.Strings(b.ChangedFiles) g.Assert(b.ChangedFiles).Equal([]string{"pipeline/shared/replace_secrets.go", "pipeline/shared/replace_secrets_test.go"}) }) }) diff --git a/server/remote/gitlab/gitlab_test.go b/server/remote/gitlab/gitlab_test.go index f858e291c9..8a3ed7381b 100644 --- a/server/remote/gitlab/gitlab_test.go +++ b/server/remote/gitlab/gitlab_test.go @@ -214,7 +214,7 @@ func Test_Gitlab(t *testing.T) { ) req.Header = testdata.ServiceHookHeaders - // TODO: insert fake store into context to retrieve user & repo, this will activate fetching of changed fiels + // TODO: insert fake store into context to retrieve user & repo, this will activate fetching of ChangedFiles hookRepo, build, err := client.Hook(ctx, req) assert.NoError(t, err) if assert.NotNil(t, hookRepo) && assert.NotNil(t, build) { From 45e3e9fcea11bd5201472ced90bc84c787302b7f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 22:49:53 +0100 Subject: [PATCH 18/20] log error --- server/remote/github/github.go | 2 ++ server/remote/gitlab/gitlab.go | 1 + 2 files changed, 3 insertions(+) diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 976f86d9e5..25be39cadc 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/google/go-github/v39/github" + "github.com/rs/zerolog/log" "golang.org/x/oauth2" "github.com/woodpecker-ci/woodpecker/server" @@ -512,6 +513,7 @@ func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model func (c *client) loadChangedFilesFromPullRequest(ctx context.Context, pull *github.PullRequest, tmpRepo *model.Repo, build *model.Build) (*model.Build, error) { _store, ok := store.TryFromContext(ctx) if !ok { + log.Error().Msg("could not get store from context") return build, nil } diff --git a/server/remote/gitlab/gitlab.go b/server/remote/gitlab/gitlab.go index 28b6edbe70..2f4eb55c76 100644 --- a/server/remote/gitlab/gitlab.go +++ b/server/remote/gitlab/gitlab.go @@ -562,6 +562,7 @@ func (g *Gitlab) Hook(ctx context.Context, req *http.Request) (*model.Repo, *mod func (g *Gitlab) loadChangedFilesFromMergeRequest(ctx context.Context, tmpRepo *model.Repo, build *model.Build, mergeIID int) (*model.Build, error) { _store, ok := store.TryFromContext(ctx) if !ok { + log.Error().Msg("could not get store from context") return build, nil } From 5d7bf5311dc8696e31e4393a6e67da4419719d5e Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 23:11:17 +0100 Subject: [PATCH 19/20] Apply suggestions from code review Co-authored-by: Anbraten --- server/remote/github/parse.go | 5 +++-- server/remote/gitlab/convert.go | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/server/remote/github/parse.go b/server/remote/github/parse.go index 46bfec9a35..1c29578a8f 100644 --- a/server/remote/github/parse.go +++ b/server/remote/github/parse.go @@ -146,7 +146,7 @@ func parseDeployHook(hook *github.DeploymentEvent, privateMode bool) (*model.Rep // parsePullHook parses a pull request hook and returns the Repo and Build // details. If the pull request is closed nil values are returned. func parsePullHook(hook *github.PullRequestEvent, merge, privateMode bool) (*github.PullRequest, *model.Repo, *model.Build, error) { - // ignore these + // only listen to new merge-requests and pushes to open ones if hook.GetAction() != actionOpen && hook.GetAction() != actionSync { return nil, nil, nil, nil } @@ -179,7 +179,8 @@ func parsePullHook(hook *github.PullRequestEvent, merge, privateMode bool) (*git } func getChangedFilesFromCommits(commits []*github.HeadCommit) []string { - files := make([]string, 0, len(commits)*3) + // assume a capacity of 4 changed files per commit + files := make([]string, 0, len(commits)*4) for _, cm := range commits { files = append(files, cm.Added...) files = append(files, cm.Removed...) diff --git a/server/remote/gitlab/convert.go b/server/remote/gitlab/convert.go index dc204c057f..3dd59bf0d1 100644 --- a/server/remote/gitlab/convert.go +++ b/server/remote/gitlab/convert.go @@ -165,6 +165,7 @@ func convertPushHook(hook *gitlab.PushEvent) (*model.Repo, *model.Build, error) build.Branch = strings.TrimPrefix(hook.Ref, "refs/heads/") build.Ref = hook.Ref + // assume a capacity of 4 changed files per commit files := make([]string, 0, len(hook.Commits)*4) for _, cm := range hook.Commits { if hook.After == cm.ID { From 5e6a7ae2d431db2d4cbf717a10878fe019a3082e Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 17 Jan 2022 23:12:37 +0100 Subject: [PATCH 20/20] Update server/remote/gitea/helper.go Co-authored-by: Anbraten --- server/remote/gitea/helper.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/remote/gitea/helper.go b/server/remote/gitea/helper.go index e49cdb4d4e..4ce45c8b79 100644 --- a/server/remote/gitea/helper.go +++ b/server/remote/gitea/helper.go @@ -111,6 +111,7 @@ func buildFromPush(hook *pushHook) *model.Build { } func getChangedFilesFromPushHook(hook *pushHook) []string { + // assume a capacity of 4 changed files per commit files := make([]string, 0, len(hook.Commits)*4) for _, c := range hook.Commits { files = append(files, c.Added...)