From 3cdee7a636e510db47858edf37e5eccf916ed32c Mon Sep 17 00:00:00 2001 From: Ryo Kitagawa Date: Thu, 11 Apr 2024 09:42:21 +0900 Subject: [PATCH 1/2] fix: get MR IID when MR number is 0 --- pkg/notifier/gitlab/comment_test.go | 18 +++++ pkg/notifier/gitlab/commits.go | 32 -------- pkg/notifier/gitlab/commits_test.go | 119 ---------------------------- pkg/notifier/gitlab/notify.go | 11 --- pkg/notifier/gitlab/notify_test.go | 5 +- 5 files changed, 20 insertions(+), 165 deletions(-) delete mode 100644 pkg/notifier/gitlab/commits_test.go diff --git a/pkg/notifier/gitlab/comment_test.go b/pkg/notifier/gitlab/comment_test.go index c8ce98a..626f752 100644 --- a/pkg/notifier/gitlab/comment_test.go +++ b/pkg/notifier/gitlab/comment_test.go @@ -1,6 +1,7 @@ package gitlab import ( + "errors" "reflect" "testing" @@ -81,6 +82,23 @@ func TestCommentPost(t *testing.T) { }, ok: false, }, + { + name: "should postForRevision when listMergeRequestIIDs is failed", + config: newFakeConfig(), + createMockGitLabAPI: func(ctrl *gomock.Controller) *gitlabmock.MockAPI { + api := gitlabmock.NewMockAPI(ctrl) + api.EXPECT().ListMergeRequestsByCommit("revision").Return(nil, nil, errors.New("error")) + // PostCommitComment should be called + api.EXPECT().PostCommitComment("revision", &gitlab.PostCommitCommentOptions{Note: gitlab.String(body)}).Return(&gitlab.CommitComment{}, nil, nil) + return api + }, + body: body, + opt: PostOptions{ + Number: 0, + Revision: "revision", + }, + ok: true, + }, } for _, testCase := range testCases { diff --git a/pkg/notifier/gitlab/commits.go b/pkg/notifier/gitlab/commits.go index e188e57..2650686 100644 --- a/pkg/notifier/gitlab/commits.go +++ b/pkg/notifier/gitlab/commits.go @@ -2,32 +2,12 @@ package gitlab import ( "errors" - - gitlab "github.com/xanzy/go-gitlab" ) // CommitsService handles communication with the commits related // methods of GitLab API type CommitsService service -// List lists commits on a repository -func (g *CommitsService) List(revision string) ([]string, error) { - if revision == "" { - return []string{}, errors.New("no revision specified") - } - commits, _, err := g.client.API.ListCommits( - &gitlab.ListCommitsOptions{}, - ) - if err != nil { - return nil, err - } - s := make([]string, len(commits)) - for i, commit := range commits { - s[i] = commit.ID - } - return s, nil -} - func (g *CommitsService) ListMergeRequestIIDsByRevision(revision string) ([]int, error) { if revision == "" { return nil, errors.New("no revision specified") @@ -43,15 +23,3 @@ func (g *CommitsService) ListMergeRequestIIDsByRevision(revision string) ([]int, } return result, nil } - -// lastOne returns the hash of the previous commit of the given commit -func (g *CommitsService) lastOne(commits []string, revision string) (string, error) { - if revision == "" { - return "", errors.New("no revision specified") - } - if len(commits) == 0 { - return "", errors.New("no commits") - } - - return commits[1], nil -} diff --git a/pkg/notifier/gitlab/commits_test.go b/pkg/notifier/gitlab/commits_test.go deleted file mode 100644 index 3bf23e4..0000000 --- a/pkg/notifier/gitlab/commits_test.go +++ /dev/null @@ -1,119 +0,0 @@ -package gitlab - -import ( - "testing" - - gitlabmock "github.com/hirosassa/tfcmt-gitlab/pkg/notifier/gitlab/gen" - gitlab "github.com/xanzy/go-gitlab" - "go.uber.org/mock/gomock" -) - -func TestCommitsList(t *testing.T) { - t.Parallel() - testCases := []struct { - name string - createMockGitLabAPI func(ctrl *gomock.Controller) *gitlabmock.MockAPI - revision string - ok bool - }{ - { - name: "should list commits", - createMockGitLabAPI: func(ctrl *gomock.Controller) *gitlabmock.MockAPI { - api := gitlabmock.NewMockAPI(ctrl) - api.EXPECT().ListCommits(gomock.Cond(func(x any) bool { - _, ok := x.(*gitlab.ListCommitsOptions) - return ok - })).Return([]*gitlab.Commit{}, nil, nil) - return api - }, - revision: "04e0917e448b662c2b16330fad50e97af16ff27a", - ok: true, - }, - { - name: "should return error when revision is empty", - createMockGitLabAPI: func(ctrl *gomock.Controller) *gitlabmock.MockAPI { - api := gitlabmock.NewMockAPI(ctrl) - return api - }, - revision: "", - ok: false, - }, - } - - for _, testCase := range testCases { - testCase := testCase - t.Run(testCase.name, func(t *testing.T) { - t.Parallel() - - cfg := newFakeConfig() - client, err := NewClient(cfg) - if err != nil { - t.Fatal(err) - } - - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - client.API = testCase.createMockGitLabAPI(mockCtrl) - - _, err = client.Commits.List(testCase.revision) - if (err == nil) != testCase.ok { - t.Errorf("got error %q", err) - } - }) - } -} - -func TestCommitsLastOne(t *testing.T) { - t.Parallel() - testCases := []struct { - commits []string - revision string - lastRev string - ok bool - }{ - { - // ok - commits: []string{ - "04e0917e448b662c2b16330fad50e97af16ff27a", - "04e0917e448b662c2b16330fad50e97af16ff27b", - "04e0917e448b662c2b16330fad50e97af16ff27c", - }, - revision: "04e0917e448b662c2b16330fad50e97af16ff27a", - lastRev: "04e0917e448b662c2b16330fad50e97af16ff27b", - ok: true, - }, - { - // no revision - commits: []string{ - "04e0917e448b662c2b16330fad50e97af16ff27a", - "04e0917e448b662c2b16330fad50e97af16ff27b", - "04e0917e448b662c2b16330fad50e97af16ff27c", - }, - revision: "", - lastRev: "", - ok: false, - }, - { - // no commits - commits: []string{}, - revision: "04e0917e448b662c2b16330fad50e97af16ff27a", - lastRev: "", - ok: false, - }, - } - - for _, testCase := range testCases { - cfg := newFakeConfig() - client, err := NewClient(cfg) - if err != nil { - t.Fatal(err) - } - commit, err := client.Commits.lastOne(testCase.commits, testCase.revision) - if (err == nil) != testCase.ok { - t.Errorf("got error %q", err) - } - if commit != testCase.lastRev { - t.Errorf("got %q but want %q", commit, testCase.lastRev) - } - } -} diff --git a/pkg/notifier/gitlab/notify.go b/pkg/notifier/gitlab/notify.go index b02b62b..e066c59 100644 --- a/pkg/notifier/gitlab/notify.go +++ b/pkg/notifier/gitlab/notify.go @@ -64,17 +64,6 @@ func (g *NotifyService) Notify(param notifier.ParamExec) (int, error) { //nolint } _, isApply := parser.(*terraform.ApplyParser) - if isApply { - if !cfg.MR.IsNumber() { - commits, err := g.client.Commits.List(cfg.MR.Revision) - if err != nil { - return result.ExitCode, err - } - lastRevision, _ := g.client.Commits.lastOne(commits, cfg.MR.Revision) - cfg.MR.Revision = lastRevision - } - } - logE := logrus.WithFields(logrus.Fields{ "program": "tfcmt", }) diff --git a/pkg/notifier/gitlab/notify_test.go b/pkg/notifier/gitlab/notify_test.go index dfb1f90..e93d35d 100644 --- a/pkg/notifier/gitlab/notify_test.go +++ b/pkg/notifier/gitlab/notify_test.go @@ -234,11 +234,10 @@ func TestNotifyNotify(t *testing.T) { //nolint:maintidx exitCode: 2, }, { - name: "apply case", + name: "get MR IID when MR number is 0", createMockGitLabAPI: func(ctrl *gomock.Controller) *gitlabmock.MockAPI { api := gitlabmock.NewMockAPI(ctrl) - api.EXPECT().ListCommits(gomock.Any()).Return([]*gitlab.Commit{{ID: "1"}, {ID: "2"}}, nil, nil) - api.EXPECT().ListMergeRequestsByCommit("2").Return([]*gitlab.MergeRequest{{IID: 1}}, nil, nil) + api.EXPECT().ListMergeRequestsByCommit("revision").Return([]*gitlab.MergeRequest{{IID: 1}}, nil, nil) api.EXPECT().CreateMergeRequestNote(1, gomock.Any()).Return(nil, nil, nil) return api }, From bf399e9bfb3b63e6e1a13b8a2624edcb4fb4457b Mon Sep 17 00:00:00 2001 From: Ryo Kitagawa Date: Thu, 11 Apr 2024 09:48:39 +0900 Subject: [PATCH 2/2] refactor: delete unused method --- pkg/notifier/gitlab/gen/gitlab.go | 21 --------------------- pkg/notifier/gitlab/gitlab.go | 6 ------ 2 files changed, 27 deletions(-) diff --git a/pkg/notifier/gitlab/gen/gitlab.go b/pkg/notifier/gitlab/gen/gitlab.go index 7f44257..d15383e 100644 --- a/pkg/notifier/gitlab/gen/gitlab.go +++ b/pkg/notifier/gitlab/gen/gitlab.go @@ -138,27 +138,6 @@ func (mr *MockAPIMockRecorder) GetMergeRequest(mergeRequest, opt any, options .. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMergeRequest", reflect.TypeOf((*MockAPI)(nil).GetMergeRequest), varargs...) } -// ListCommits mocks base method. -func (m *MockAPI) ListCommits(opt *gitlab.ListCommitsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Commit, *gitlab.Response, error) { - m.ctrl.T.Helper() - varargs := []any{opt} - for _, a := range options { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "ListCommits", varargs...) - ret0, _ := ret[0].([]*gitlab.Commit) - ret1, _ := ret[1].(*gitlab.Response) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// ListCommits indicates an expected call of ListCommits. -func (mr *MockAPIMockRecorder) ListCommits(opt any, options ...any) *gomock.Call { - mr.mock.ctrl.T.Helper() - varargs := append([]any{opt}, options...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListCommits", reflect.TypeOf((*MockAPI)(nil).ListCommits), varargs...) -} - // ListMergeRequestLabels mocks base method. func (m *MockAPI) ListMergeRequestLabels(mergeRequest int, opt *gitlab.GetMergeRequestsOptions, options ...gitlab.RequestOptionFunc) (gitlab.Labels, error) { m.ctrl.T.Helper() diff --git a/pkg/notifier/gitlab/gitlab.go b/pkg/notifier/gitlab/gitlab.go index 19cc00e..2ef96b4 100644 --- a/pkg/notifier/gitlab/gitlab.go +++ b/pkg/notifier/gitlab/gitlab.go @@ -14,7 +14,6 @@ type API interface { GetMergeRequest(mergeRequest int, opt *gitlab.GetMergeRequestsOptions, options ...gitlab.RequestOptionFunc) (*gitlab.MergeRequest, *gitlab.Response, error) UpdateMergeRequest(mergeRequest int, opt *gitlab.UpdateMergeRequestOptions, options ...gitlab.RequestOptionFunc) (*gitlab.MergeRequest, *gitlab.Response, error) PostCommitComment(sha string, opt *gitlab.PostCommitCommentOptions, options ...gitlab.RequestOptionFunc) (*gitlab.CommitComment, *gitlab.Response, error) - ListCommits(opt *gitlab.ListCommitsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Commit, *gitlab.Response, error) AddMergeRequestLabels(labels *[]string, mergeRequest int) (gitlab.Labels, error) RemoveMergeRequestLabels(labels *[]string, mergeRequest int) (gitlab.Labels, error) ListMergeRequestLabels(mergeRequest int, opt *gitlab.GetMergeRequestsOptions, options ...gitlab.RequestOptionFunc) (gitlab.Labels, error) @@ -60,11 +59,6 @@ func (g *GitLab) PostCommitComment(sha string, opt *gitlab.PostCommitCommentOpti return g.Client.Commits.PostCommitComment(fmt.Sprintf("%s/%s", g.namespace, g.project), sha, opt, options...) } -// ListCommits is a wrapper of https://godoc.org/github.com/xanzy/go-gitlab#CommitsService.ListCommits -func (g *GitLab) ListCommits(opt *gitlab.ListCommitsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Commit, *gitlab.Response, error) { - return g.Client.Commits.ListCommits(fmt.Sprintf("%s/%s", g.namespace, g.project), opt, options...) -} - // AddMergeRequestLabels adds labels on the merge request. func (g *GitLab) AddMergeRequestLabels(labels *[]string, mergeRequest int) (gitlab.Labels, error) { var addLabels gitlab.Labels