diff --git a/client/client.go b/client/client.go index 67094eb..39f8704 100644 --- a/client/client.go +++ b/client/client.go @@ -2,6 +2,7 @@ package client import ( "fmt" + "strings" "github.com/xanzy/go-gitlab" ) @@ -18,7 +19,7 @@ type GitlabClient interface { ListMrsWithLabel(label string) ([]*gitlab.MergeRequest, error) RefreshMr(mr *gitlab.MergeRequest) (*gitlab.MergeRequest, error) MergeMr(mr *gitlab.MergeRequest) error - Comment(mr *gitlab.MergeRequest, comment string) error + Comment(mr *gitlab.MergeRequest, title string, comment string) error } type gitlabClientImpl struct { @@ -100,24 +101,36 @@ func (g *gitlabClientImpl) MergeMr(mr *gitlab.MergeRequest) error { return nil } -func (g *gitlabClientImpl) Comment(mr *gitlab.MergeRequest, comment string) error { +func (g *gitlabClientImpl) Comment(mr *gitlab.MergeRequest, title string, comment string) error { + full_comment := fmt.Sprintf("**%s**: %s", title, comment) nopts := &gitlab.ListMergeRequestNotesOptions{} notes, _, err := g.client.Notes.ListMergeRequestNotes(mr.ProjectID, mr.IID, nopts) + if err != nil { return fmt.Errorf("failed to get comments on MR: %w", err) } for _, n := range notes { if n.Author.ID == g.me.ID { - if n.Body == comment { + if title == extractTitleFromComment(n.Body) { + // Update old comment with the same title: + opts := &gitlab.UpdateMergeRequestNoteOptions{ + Body: gitlab.Ptr(full_comment), + } + _, _, err = g.client.Notes.UpdateMergeRequestNote(mr.ProjectID, mr.IID, n.ID, opts) + if err != nil { + return fmt.Errorf("failed to update comment on MR: %w", err) + } return nil } + // Only check the newest own comment for title match. break } } + // We don't have the possibility to update a previous comment of the same type - make a new one: opts := &gitlab.CreateMergeRequestNoteOptions{ - Body: gitlab.Ptr(comment), + Body: gitlab.Ptr(full_comment), } _, _, err = g.client.Notes.CreateMergeRequestNote(mr.ProjectID, mr.IID, opts) if err != nil { @@ -126,6 +139,14 @@ func (g *gitlabClientImpl) Comment(mr *gitlab.MergeRequest, comment string) erro return nil } +func extractTitleFromComment(comment string) string { + parts := strings.Split(comment, "**") + if len(parts) >= 2 { + return parts[1] + } + return "" +} + func IsMergeable(mr *gitlab.MergeRequest) bool { return mr.DetailedMergeStatus == MR_MERGE_STATUS_MERGEABLE } diff --git a/client/mock/client.go b/client/mock/client.go index 6380ec6..cbb429b 100644 --- a/client/mock/client.go +++ b/client/mock/client.go @@ -12,7 +12,7 @@ package mock_client import ( reflect "reflect" - go_gitlab "github.com/xanzy/go-gitlab" + gitlab "github.com/xanzy/go-gitlab" gomock "go.uber.org/mock/gomock" ) @@ -40,21 +40,21 @@ func (m *MockGitlabClient) EXPECT() *MockGitlabClientMockRecorder { } // Comment mocks base method. -func (m *MockGitlabClient) Comment(mr *go_gitlab.MergeRequest, comment string) error { +func (m *MockGitlabClient) Comment(mr *gitlab.MergeRequest, title, comment string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Comment", mr, comment) + ret := m.ctrl.Call(m, "Comment", mr, title, comment) ret0, _ := ret[0].(error) return ret0 } // Comment indicates an expected call of Comment. -func (mr_2 *MockGitlabClientMockRecorder) Comment(mr, comment any) *gomock.Call { +func (mr_2 *MockGitlabClientMockRecorder) Comment(mr, title, comment any) *gomock.Call { mr_2.mock.ctrl.T.Helper() - return mr_2.mock.ctrl.RecordCallWithMethodType(mr_2.mock, "Comment", reflect.TypeOf((*MockGitlabClient)(nil).Comment), mr, comment) + return mr_2.mock.ctrl.RecordCallWithMethodType(mr_2.mock, "Comment", reflect.TypeOf((*MockGitlabClient)(nil).Comment), mr, title, comment) } // GetConfigFileForMR mocks base method. -func (m *MockGitlabClient) GetConfigFileForMR(mr *go_gitlab.MergeRequest, filePath string) (*[]byte, error) { +func (m *MockGitlabClient) GetConfigFileForMR(mr *gitlab.MergeRequest, filePath string) (*[]byte, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetConfigFileForMR", mr, filePath) ret0, _ := ret[0].(*[]byte) @@ -69,10 +69,10 @@ func (mr_2 *MockGitlabClientMockRecorder) GetConfigFileForMR(mr, filePath any) * } // ListMrsWithLabel mocks base method. -func (m *MockGitlabClient) ListMrsWithLabel(label string) ([]*go_gitlab.MergeRequest, error) { +func (m *MockGitlabClient) ListMrsWithLabel(label string) ([]*gitlab.MergeRequest, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListMrsWithLabel", label) - ret0, _ := ret[0].([]*go_gitlab.MergeRequest) + ret0, _ := ret[0].([]*gitlab.MergeRequest) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -84,7 +84,7 @@ func (mr *MockGitlabClientMockRecorder) ListMrsWithLabel(label any) *gomock.Call } // MergeMr mocks base method. -func (m *MockGitlabClient) MergeMr(mr *go_gitlab.MergeRequest) error { +func (m *MockGitlabClient) MergeMr(mr *gitlab.MergeRequest) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "MergeMr", mr) ret0, _ := ret[0].(error) @@ -98,10 +98,10 @@ func (mr_2 *MockGitlabClientMockRecorder) MergeMr(mr any) *gomock.Call { } // RefreshMr mocks base method. -func (m *MockGitlabClient) RefreshMr(mr *go_gitlab.MergeRequest) (*go_gitlab.MergeRequest, error) { +func (m *MockGitlabClient) RefreshMr(mr *gitlab.MergeRequest) (*gitlab.MergeRequest, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "RefreshMr", mr) - ret0, _ := ret[0].(*go_gitlab.MergeRequest) + ret0, _ := ret[0].(*gitlab.MergeRequest) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/task/task.go b/task/task.go index aa5da9e..878a113 100644 --- a/task/task.go +++ b/task/task.go @@ -43,6 +43,13 @@ type Clock interface { type realClock struct{} +const ( + COMMENT_MERGE_FAILED = "Failed to merge" + COMMENT_MERGE_SKIPPED = "Not merging automatically" + COMMENT_MERGE_SCHEDULING_FAILED = "Failed to schedule merge" + COMMENT_MERGE_SCHEDULED = "Merge scheduled" +) + func (realClock) Now() time.Time { return time.Now() } @@ -85,14 +92,14 @@ func (t Task) processMR(mr *gitlab.MergeRequest) error { file, err := t.client.GetConfigFileForMR(mr, t.config.ConfigFilePath) if err != nil { - return t.client.Comment(mr, "Failed to schedule merge: missing config file.") + return t.client.Comment(mr, COMMENT_MERGE_SCHEDULING_FAILED, "Missing config file.") } config := RepositoryConfig{} err = yaml.Unmarshal(*file, &config) if err != nil { - return t.client.Comment(mr, fmt.Sprintf("Failed to schedule merge: Error while parsing config file.\n\n%s", err.Error())) + return t.client.Comment(mr, COMMENT_MERGE_SCHEDULING_FAILED, fmt.Sprintf("Error while parsing config file.\n\n%s", err.Error())) } now := t.clock.Now() @@ -101,7 +108,7 @@ func (t Task) processMR(mr *gitlab.MergeRequest) error { for _, w := range config.MergeWindows { nextActiveStartTime, err := w.getNextActiveWindowStartTime(now) if err != nil { - return t.client.Comment(mr, fmt.Sprintf("Failed to schedule merge: Error while parsing merge windows.\n\n%s", err.Error())) + return t.client.Comment(mr, COMMENT_MERGE_SCHEDULING_FAILED, fmt.Sprintf("Error while parsing merge windows.\n\n%s", err.Error())) } if nextActiveStartTime.Before(now) { return t.mergeMR(mr) @@ -114,7 +121,7 @@ func (t Task) processMR(mr *gitlab.MergeRequest) error { nextActiveEndTime := earliestMergeWindowTime.Add(earliestMergeWindow.MaxDelay) msg := fmt.Sprintf( - "Merge scheduled. This MR will be merged between %s and %s.", + "This MR will be merged between %s and %s.", earliestMergeWindowTime.Format(time.UnixDate), nextActiveEndTime.Format(time.UnixDate), ) @@ -127,23 +134,23 @@ func (t Task) processMR(mr *gitlab.MergeRequest) error { ) } - return t.client.Comment(mr, msg) + return t.client.Comment(mr, COMMENT_MERGE_SCHEDULED, msg) } func (t Task) mergeMR(mr *gitlab.MergeRequest) error { // We need to recheck MRs - we might in the interim have merged other things that led to conflicts rmr, err := t.client.RefreshMr(mr) if err != nil { - return t.client.Comment(mr, fmt.Sprintf("Failed to merge: error while refreshing merge request data.\n\n%s", err.Error())) + return t.client.Comment(mr, COMMENT_MERGE_FAILED, fmt.Sprintf("Error while refreshing merge request data.\n\n%s", err.Error())) } if !client.IsMergeable(rmr) { - return t.client.Comment(mr, fmt.Sprintf("Not merging automatically: MR is not mergeable. Current status: %s", rmr.DetailedMergeStatus)) + return t.client.Comment(mr, COMMENT_MERGE_SKIPPED, fmt.Sprintf("MR is not mergeable. Current status: %s", rmr.DetailedMergeStatus)) } err = t.client.MergeMr(rmr) if err != nil { - return t.client.Comment(mr, fmt.Sprintf("Failed to merge: Error while merging.\n\n%s", err.Error())) + return t.client.Comment(mr, COMMENT_MERGE_FAILED, fmt.Sprintf("Error while merging.\n\n%s", err.Error())) } return nil diff --git a/task/task_test.go b/task/task_test.go index dc986b1..aeed0f2 100644 --- a/task/task_test.go +++ b/task/task_test.go @@ -54,7 +54,7 @@ func Test_RunTask(t *testing.T) { mock.EXPECT().RefreshMr(mrs[0]).Return(mrs[0], nil), mock.EXPECT().MergeMr(mrs[0]).Return(nil), mock.EXPECT().GetConfigFileForMR(mrs[1], ".config-file.yml").Return(inactiveMergeWindow(), nil), - mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}})).Return(nil), + mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}}), gomock.Any()).Return(nil), ) err := subject.Run() @@ -77,9 +77,9 @@ func Test_RunTask_TimeZoneShenanigans(t *testing.T) { gomock.InOrder( mock.EXPECT().ListMrsWithLabel(gomock.Any()).Return(mrs, nil), mock.EXPECT().GetConfigFileForMR(mrs[0], ".config-file.yml").Return(inactiveMergeWindowWithLocation(), nil), - mock.EXPECT().Comment(mrs[0], gomock.Not(hasSubstr{[]string{"Failed"}})).Return(nil), + mock.EXPECT().Comment(mrs[0], gomock.Not(hasSubstr{[]string{"Failed"}}), gomock.Any()).Return(nil), mock.EXPECT().GetConfigFileForMR(mrs[1], ".config-file.yml").Return(inactiveMergeWindowWithWeek(), nil), - mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}})).Return(nil), + mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}}), gomock.Any()).Return(nil), ) err := subject.Run() @@ -102,9 +102,9 @@ func Test_RunTask_WithError(t *testing.T) { gomock.InOrder( mock.EXPECT().ListMrsWithLabel(gomock.Any()).Return(mrs, nil), mock.EXPECT().GetConfigFileForMR(mrs[0], ".config-file.yml").Return(nil, errors.New("ERROR FAIL HALP")), - mock.EXPECT().Comment(mrs[0], hasSubstr{[]string{"Failed"}}).Return(nil), + mock.EXPECT().Comment(mrs[0], hasSubstr{[]string{"Failed"}}, gomock.Any()).Return(nil), mock.EXPECT().GetConfigFileForMR(mrs[1], ".config-file.yml").Return(inactiveMergeWindowWithWeek(), nil), - mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}})).Return(errors.New("COMMENT FAILED")), + mock.EXPECT().Comment(mrs[1], gomock.Not(hasSubstr{[]string{"Failed"}}), gomock.Any()).Return(errors.New("COMMENT FAILED")), ) err := subject.Run()