Skip to content

Commit

Permalink
Improve commenting logic to not produce repetitive comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@HappyTetrahedron
HappyTetrahedron committed Jul 15, 2024
1 parent 2d5c464 commit dc5729c
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 28 deletions.
29 changes: 25 additions & 4 deletions client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package client

import (
"fmt"
"strings"

"github.com/xanzy/go-gitlab"
)
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
22 changes: 11 additions & 11 deletions client/mock/client.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 15 additions & 8 deletions task/task.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down Expand Up @@ -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()
Expand All @@ -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)
Expand All @@ -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),
)
Expand All @@ -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
Expand Down
10 changes: 5 additions & 5 deletions task/task_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down

0 comments on commit dc5729c

Please sign in to comment.