Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve commenting logic to not produce repetitive comments #11

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Improve commenting logic to not produce repetitive comments #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions client/client.go
View file
Open in desktop
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
Open in desktop

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
Open in desktop
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
Open in desktop
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