From fdc00608ae210e46885f1f4a425419bda1378079 Mon Sep 17 00:00:00 2001 From: chhsia0 Date: Fri, 21 Aug 2020 15:00:39 -0700 Subject: [PATCH 1/2] Supported `RepositoryService.UpdatedHook` in GitLab driver. Note that the returned hook events are changed to reflect the actual parameter names used to create or update the webhook. Specifically, the original `convertEvents` in the GitLab driver sets the value of `Hook.Events` with the following mapping: ``` issues_events=true -> "issue" tag_push_events=true -> "tag" push_events=true -> "push" note_events=true -> "comment" merge_requests_events=true -> "merge" other options -> none ``` It seems the event name strings are arbitrary and not consistent across different drivers, so the mapping itself doesn't provide much abstraction (if any). More importantly, for all other drivers, `Hook.Events` contains native events unrecognized by go-scm, but GitLab driver completely discards them. This is important to implement logics to "reconcile" webhooks (i.e., update only if needed) to sync the list of watched events. Therefore, this patch proposes `Hook.Events` returned by the hook-related functions in the GitLab driver to store the option names if their values are true, e.g., `["issues_events", "push_events", "confidential_issues_events"]`. This is a breaking change. --- scm/driver/gitlab/repo.go | 136 +++++++++-------- scm/driver/gitlab/repo_test.go | 140 +++++++++++++++--- scm/driver/gitlab/testdata/hook.json.golden | 13 +- .../hook_skip_verification.json.golden | 13 +- scm/driver/gitlab/testdata/hooks.json.golden | 13 +- 5 files changed, 223 insertions(+), 92 deletions(-) diff --git a/scm/driver/gitlab/repo.go b/scm/driver/gitlab/repo.go index ed3152a25..5368ea572 100644 --- a/scm/driver/gitlab/repo.go +++ b/scm/driver/gitlab/repo.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "net/url" + "sort" "strconv" "strings" "time" @@ -46,19 +47,22 @@ type access struct { } type hook struct { - ID int `json:"id"` - URL string `json:"url"` - ProjectID int `json:"project_id"` - PushEvents bool `json:"push_events"` - IssuesEvents bool `json:"issues_events"` - MergeRequestsEvents bool `json:"merge_requests_events"` - TagPushEvents bool `json:"tag_push_events"` - NoteEvents bool `json:"note_events"` - JobEvents bool `json:"job_events"` - PipelineEvents bool `json:"pipeline_events"` - WikiPageEvents bool `json:"wiki_page_events"` - EnableSslVerification bool `json:"enable_ssl_verification"` - CreatedAt time.Time `json:"created_at"` + ID int `json:"id"` + URL string `json:"url"` + ProjectID int `json:"project_id"` + PushEvents bool `json:"push_events"` + IssuesEvents bool `json:"issues_events"` + ConfidentialIssuesEvents bool `json:"confidential_issues_events"` + MergeRequestsEvents bool `json:"merge_requests_events"` + TagPushEvents bool `json:"tag_push_events"` + NoteEvents bool `json:"note_events"` + ConfidentialNoteEvents bool `json:"confidential_note_events"` + JobEvents bool `json:"job_events"` + PipelineEvents bool `json:"pipeline_events"` + WikiPageEvents bool `json:"wiki_page_events"` + DeploymentEvents bool `json:"deployment_events"` + EnableSslVerification bool `json:"enable_ssl_verification"` + CreatedAt time.Time `json:"created_at"` } type repositoryService struct { @@ -110,30 +114,13 @@ func (s *repositoryService) ListStatus(ctx context.Context, repo, ref string, op func (s *repositoryService) CreateHook(ctx context.Context, repo string, input *scm.HookInput) (*scm.Hook, *scm.Response, error) { params := url.Values{} params.Set("url", input.Target) - if input.Secret != "" { - params.Set("token", input.Secret) + params.Set("token", input.Secret) + params.Set("enable_ssl_verification", strconv.FormatBool(!input.SkipVerify)) + for k, v := range convertFromHookEvents(input.Events) { + params.Set(k, strconv.FormatBool(v)) } - if input.SkipVerify { - params.Set("enable_ssl_verification", "false") - } - if input.Events.Branch { - // no-op - } - if input.Events.Issue { - params.Set("issues_events", "true") - } - if input.Events.IssueComment || - input.Events.PullRequestComment { - params.Set("note_events", "true") - } - if input.Events.PullRequest { - params.Set("merge_requests_events", "true") - } - if input.Events.Push || input.Events.Branch { - params.Set("push_events", "true") - } - if input.Events.Tag { - params.Set("tag_push_events", "true") + for _, k := range input.NativeEvents { + params.Set(k, "true") } path := fmt.Sprintf("api/v4/projects/%s/hooks?%s", encode(repo), params.Encode()) @@ -153,8 +140,22 @@ func (s *repositoryService) CreateStatus(ctx context.Context, repo, ref string, return convertStatus(out), res, err } -func (s *repositoryService) UpdateHook(ctx context.Context, repo string, id string, input *scm.HookInput) (*scm.Hook, *scm.Response, error) { - return nil, nil, scm.ErrNotSupported +func (s *repositoryService) UpdateHook(ctx context.Context, repo, id string, input *scm.HookInput) (*scm.Hook, *scm.Response, error) { + params := url.Values{} + params.Set("url", input.Target) + params.Set("token", input.Secret) + params.Set("enable_ssl_verification", strconv.FormatBool(!input.SkipVerify)) + for k, v := range convertFromHookEvents(input.Events) { + params.Set(k, strconv.FormatBool(v)) + } + for _, k := range input.NativeEvents { + params.Set(k, "true") + } + + path := fmt.Sprintf("api/v4/projects/%s/hooks/%s?%s", encode(repo), id, params.Encode()) + out := new(hook) + res, err := s.client.do(ctx, "PUT", path, nil, out) + return convertHook(out), res, err } func (s *repositoryService) DeleteHook(ctx context.Context, repo string, id string) (*scm.Response, error) { @@ -219,6 +220,45 @@ func convertHook(from *hook) *scm.Hook { } } +func convertFromHookEvents(from scm.HookEvents) map[string]bool { + return map[string]bool{ + "push_events": from.Push || from.Branch, + "issues_events": from.Issue, + "merge_requests_events": from.PullRequest, + "confidential_issues_events": false, + "tag_push_events": from.Tag, + "note_events": from.IssueComment || from.PullRequestComment || from.ReviewComment, + "confidential_note_events": false, + "job_events": false, + "pipeline_events": false, + "wiki_page_events": false, + "deployment_events": from.Deployment, + } +} + +func convertEvents(from *hook) []string { + var events []string + for k, v := range map[string]bool{ + "push_events": from.PushEvents, + "issues_events": from.IssuesEvents, + "merge_requests_events": from.MergeRequestsEvents, + "confidential_issues_events": from.ConfidentialIssuesEvents, + "tag_push_events": from.TagPushEvents, + "note_events": from.NoteEvents, + "confidential_note_events": from.ConfidentialNoteEvents, + "job_events": from.JobEvents, + "pipeline_events": from.PipelineEvents, + "wiki_page_events": from.WikiPageEvents, + "deployment_events": from.DeploymentEvents, + } { + if v { + events = append(events, k) + } + } + sort.Strings(events) + return events +} + type status struct { Name string `json:"name"` Desc null.String `json:"description"` @@ -247,26 +287,6 @@ func convertStatus(from *status) *scm.Status { } } -func convertEvents(from *hook) []string { - var events []string - if from.IssuesEvents { - events = append(events, "issues") - } - if from.TagPushEvents { - events = append(events, "tag") - } - if from.PushEvents { - events = append(events, "push") - } - if from.NoteEvents { - events = append(events, "comment") - } - if from.MergeRequestsEvents { - events = append(events, "merge") - } - return events -} - func convertState(from string) scm.State { switch from { case "canceled": diff --git a/scm/driver/gitlab/repo_test.go b/scm/driver/gitlab/repo_test.go index 48e21db6b..396448a67 100644 --- a/scm/driver/gitlab/repo_test.go +++ b/scm/driver/gitlab/repo_test.go @@ -7,7 +7,9 @@ package gitlab import ( "context" "encoding/json" + "fmt" "io/ioutil" + "regexp" "testing" "github.com/drone/go-scm/scm" @@ -301,47 +303,63 @@ func TestRepositoryHookList(t *testing.T) { t.Run("Page", testPage(res)) } -func TestRepositoryHookDelete(t *testing.T) { +func TestRepositoryHookCreate(t *testing.T) { defer gock.Off() gock.New("https://gitlab.com"). - Delete("/api/v4/projects/diaspora/diaspora/hooks/1"). - Reply(204). + Post("/api/v4/projects/diaspora/diaspora/hooks"). + MatchParam("token", "topsecret"). + MatchParam("url", "https://ci.example.com/hook"). + Reply(201). Type("application/json"). - SetHeaders(mockHeaders) + SetHeaders(mockHeaders). + File("testdata/hook.json") + + in := &scm.HookInput{ + Name: "drone", + Target: "https://ci.example.com/hook", + Secret: "topsecret", + SkipVerify: false, + } client := NewDefault() - res, err := client.Repositories.DeleteHook(context.Background(), "diaspora/diaspora", "1") + got, res, err := client.Repositories.CreateHook(context.Background(), "diaspora/diaspora", in) if err != nil { t.Error(err) return } - if got, want := res.Status, 204; got != want { - t.Errorf("Want response status %d, got %d", want, got) + want := new(scm.Hook) + raw, _ := ioutil.ReadFile("testdata/hook.json.golden") + json.Unmarshal(raw, want) + + if diff := cmp.Diff(got, want); diff != "" { + t.Errorf("Unexpected Results") + t.Log(diff) } t.Run("Request", testRequest(res)) t.Run("Rate", testRate(res)) } -func TestRepositoryHookCreate(t *testing.T) { +func TestRepositoryHookCreate_SkipVerification(t *testing.T) { defer gock.Off() gock.New("https://gitlab.com"). Post("/api/v4/projects/diaspora/diaspora/hooks"). + MatchParam("enable_ssl_verification", "false"). MatchParam("token", "topsecret"). MatchParam("url", "https://ci.example.com/hook"). Reply(201). Type("application/json"). SetHeaders(mockHeaders). - File("testdata/hook.json") + File("testdata/hook_skip_verification.json") in := &scm.HookInput{ Name: "drone", Target: "https://ci.example.com/hook", Secret: "topsecret", - SkipVerify: false, + SkipVerify: true, } client := NewDefault() @@ -352,7 +370,7 @@ func TestRepositoryHookCreate(t *testing.T) { } want := new(scm.Hook) - raw, _ := ioutil.ReadFile("testdata/hook.json.golden") + raw, _ := ioutil.ReadFile("testdata/hook_skip_verification.json.golden") json.Unmarshal(raw, want) if diff := cmp.Diff(got, want); diff != "" { @@ -364,35 +382,34 @@ func TestRepositoryHookCreate(t *testing.T) { t.Run("Rate", testRate(res)) } -func TestRepositoryHookCreate_SkipVerification(t *testing.T) { +func TestRepositoryHookUpdate(t *testing.T) { defer gock.Off() gock.New("https://gitlab.com"). - Post("/api/v4/projects/diaspora/diaspora/hooks"). - MatchParam("enable_ssl_verification", "false"). + Put("/api/v4/projects/diaspora/diaspora/hooks/1"). MatchParam("token", "topsecret"). MatchParam("url", "https://ci.example.com/hook"). - Reply(201). + Reply(200). Type("application/json"). SetHeaders(mockHeaders). - File("testdata/hook_skip_verification.json") + File("testdata/hook.json") in := &scm.HookInput{ Name: "drone", Target: "https://ci.example.com/hook", Secret: "topsecret", - SkipVerify: true, + SkipVerify: false, } client := NewDefault() - got, res, err := client.Repositories.CreateHook(context.Background(), "diaspora/diaspora", in) + got, res, err := client.Repositories.UpdateHook(context.Background(), "diaspora/diaspora", "1", in) if err != nil { t.Error(err) return } want := new(scm.Hook) - raw, _ := ioutil.ReadFile("testdata/hook_skip_verification.json.golden") + raw, _ := ioutil.ReadFile("testdata/hook.json.golden") json.Unmarshal(raw, want) if diff := cmp.Diff(got, want); diff != "" { @@ -404,6 +421,91 @@ func TestRepositoryHookCreate_SkipVerification(t *testing.T) { t.Run("Rate", testRate(res)) } +func TestRepositoryHookDelete(t *testing.T) { + defer gock.Off() + + gock.New("https://gitlab.com"). + Delete("/api/v4/projects/diaspora/diaspora/hooks/1"). + Reply(204). + Type("application/json"). + SetHeaders(mockHeaders) + + client := NewDefault() + res, err := client.Repositories.DeleteHook(context.Background(), "diaspora/diaspora", "1") + if err != nil { + t.Error(err) + return + } + + if got, want := res.Status, 204; got != want { + t.Errorf("Want response status %d, got %d", want, got) + } + + t.Run("Request", testRequest(res)) + t.Run("Rate", testRate(res)) +} + +func TestConvertFromHookEvents(t *testing.T) { + str := func(v scm.HookEvents) string { + s := fmt.Sprintf("%+v", v) + return regexp.MustCompile(" *[A-Za-z]+:false *").ReplaceAllString(s, "") + } + for _, test := range []struct { + src scm.HookEvents + dst string + }{{ + src: scm.HookEvents{Push: true}, + dst: "push_events", + }, { + src: scm.HookEvents{Branch: true}, + dst: "push_events", + }, { + src: scm.HookEvents{Issue: true}, + dst: "issues_events", + }, { + src: scm.HookEvents{PullRequest: true}, + dst: "merge_requests_events", + }, { + src: scm.HookEvents{Tag: true}, + dst: "tag_push_events", + }, { + src: scm.HookEvents{IssueComment: true}, + dst: "note_events", + }, { + src: scm.HookEvents{PullRequestComment: true}, + dst: "note_events", + }, { + src: scm.HookEvents{ReviewComment: true}, + dst: "note_events", + }, { + src: scm.HookEvents{Deployment: true}, + dst: "deployment_events", + }} { + events := convertFromHookEvents(test.src) + for _, k := range []string{ + "push_events", + "issues_events", + "merge_requests_events", + "confidential_issues_events", + "tag_push_events", + "note_events", + "confidential_note_events", + "job_events", + "pipeline_events", + "wiki_page_events", + "deployment_events", + } { + if v, ok := events[k]; !ok || v != (k == test.dst) { + got, want := fmt.Sprintf("%s=%t", k, v), fmt.Sprintf("%s=%t", k, k == test.dst) + if !ok { + got = "none" + } + t.Errorf("Want %s converted to %s, got %s", str(test.src), want, got) + } + } + } +} + func TestConvertState(t *testing.T) { tests := []struct { src string diff --git a/scm/driver/gitlab/testdata/hook.json.golden b/scm/driver/gitlab/testdata/hook.json.golden index d11f4b7c6..e320db0d5 100644 --- a/scm/driver/gitlab/testdata/hook.json.golden +++ b/scm/driver/gitlab/testdata/hook.json.golden @@ -3,11 +3,14 @@ "Name": "", "Target": "http://example.com/hook", "Events": [ - "issues", - "tag", - "push", - "comment", - "merge" + "issues_events", + "job_events", + "merge_requests_events", + "note_events", + "pipeline_events", + "push_events", + "tag_push_events", + "wiki_page_events" ], "Active": true, "SkipVerify": false diff --git a/scm/driver/gitlab/testdata/hook_skip_verification.json.golden b/scm/driver/gitlab/testdata/hook_skip_verification.json.golden index 746342595..b4ad9ca7c 100644 --- a/scm/driver/gitlab/testdata/hook_skip_verification.json.golden +++ b/scm/driver/gitlab/testdata/hook_skip_verification.json.golden @@ -3,11 +3,14 @@ "Name": "", "Target": "http://example.com/hook", "Events": [ - "issues", - "tag", - "push", - "comment", - "merge" + "issues_events", + "job_events", + "merge_requests_events", + "note_events", + "pipeline_events", + "push_events", + "tag_push_events", + "wiki_page_events" ], "Active": true, "SkipVerify": true diff --git a/scm/driver/gitlab/testdata/hooks.json.golden b/scm/driver/gitlab/testdata/hooks.json.golden index a90d275c3..741696194 100644 --- a/scm/driver/gitlab/testdata/hooks.json.golden +++ b/scm/driver/gitlab/testdata/hooks.json.golden @@ -4,11 +4,14 @@ "Name": "", "Target": "http://example.com/hook", "Events": [ - "issues", - "tag", - "push", - "comment", - "merge" + "issues_events", + "job_events", + "merge_requests_events", + "note_events", + "pipeline_events", + "push_events", + "tag_push_events", + "wiki_page_events" ], "Active": true, "SkipVerify": false From 22d9f7638b0c41e9bd229b2f07bdf3e624f847b6 Mon Sep 17 00:00:00 2001 From: chhsia0 Date: Tue, 25 Aug 2020 11:00:12 -0700 Subject: [PATCH 2/2] Set secret token in `CreateHook` only if non-empty. --- scm/driver/gitlab/repo.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scm/driver/gitlab/repo.go b/scm/driver/gitlab/repo.go index 5368ea572..520d7bf93 100644 --- a/scm/driver/gitlab/repo.go +++ b/scm/driver/gitlab/repo.go @@ -114,7 +114,11 @@ func (s *repositoryService) ListStatus(ctx context.Context, repo, ref string, op func (s *repositoryService) CreateHook(ctx context.Context, repo string, input *scm.HookInput) (*scm.Hook, *scm.Response, error) { params := url.Values{} params.Set("url", input.Target) - params.Set("token", input.Secret) + // Earlier versions of gitlab returned an error if the token was an empty string, so we only set + // the token value if non-empty. + if input.Secret != "" { + params.Set("token", input.Secret) + } params.Set("enable_ssl_verification", strconv.FormatBool(!input.SkipVerify)) for k, v := range convertFromHookEvents(input.Events) { params.Set(k, strconv.FormatBool(v))