diff --git a/bot/actions/automerge/automerge.go b/bot/actions/automerge/automerge.go index f18e597..b0bbcb4 100644 --- a/bot/actions/automerge/automerge.go +++ b/bot/actions/automerge/automerge.go @@ -18,6 +18,11 @@ type MergeableEventData interface { Merge(mergeMethod string) } +type HasReviewersAPIEventData interface { + GetReviewers() map[string]string + GetApprovals() []string +} + func (a *action) merge(meta bot.EventData) { if a.rule.Label == "" { mergeable, ok := meta.(MergeableEventData) @@ -32,12 +37,28 @@ func (a *action) merge(meta bot.EventData) { } } -func (a *action) Apply(config bot.Configuration, meta bot.EventData) { +func (a *action) getApprovalsFromAPI(meta bot.EventData) (int, bool) { assigneesList := meta.GetAssignees() - if len(assigneesList) == 0 { - util.Logger.Debug("No assignees to issue - skipping") - return + approvals := 0 + assignees := util.StringSet{} + assignees.AddAll(assigneesList) + reviewApi, ok := meta.(HasReviewersAPIEventData) + if !ok { + util.Logger.Warning("Event data does not support reviewers API. Check your configuration") + a.err = fmt.Errorf("Event data does not support reviewers API") + } else { + for _, approver := range reviewApi.GetApprovals() { + if assignees.Contains(approver) { + assignees.Remove(approver) + approvals++ + } + } } + return approvals, assignees.Len() == 0 +} + +func (a *action) getApprovalsFromComments(meta bot.EventData) (int, bool) { + assigneesList := meta.GetAssignees() approvals := 0 assignees := util.StringSet{} assignees.AddAll(assigneesList) @@ -51,12 +72,30 @@ func (a *action) Apply(config bot.Configuration, meta bot.EventData) { approvals++ } } - if a.rule.Require == 0 && assignees.Len() == 0 { - util.Logger.Debug("All assignees have approved the PR - merging") - a.merge(meta) - } else if a.rule.Require > 0 && approvals >= a.rule.Require { - util.Logger.Debug("Got %d required approvals for PR - merging", a.rule.Require) - a.merge(meta) + return approvals, assignees.Len() == 0 +} + +func (a *action) Apply(config bot.Configuration, meta bot.EventData) { + assigneesList := meta.GetAssignees() + if len(assigneesList) == 0 { + util.Logger.Debug("No assignees to issue - skipping") + return + } + calls := []func(bot.EventData) (int, bool){ + a.getApprovalsFromAPI, + a.getApprovalsFromComments, + } + for _, call := range calls { + approvals, all := call(meta) + if a.rule.Require == 0 && all { + util.Logger.Debug("All assignees have approved the PR - merging") + a.merge(meta) + return + } else if a.rule.Require > 0 && approvals >= a.rule.Require { + util.Logger.Debug("Got %d required approvals for PR - merging", a.rule.Require) + a.merge(meta) + return + } } } diff --git a/bot/actions/automerge/automerge.md b/bot/actions/automerge/automerge.md index a8d6573..3d18c90 100644 --- a/bot/actions/automerge/automerge.md +++ b/bot/actions/automerge/automerge.md @@ -8,6 +8,12 @@ Assignees must use one of the approval phrases (_Approve_, _Approved_, _LGTM_, _ **Note** If others comment with one of the approval phrases, it will not count as approval +### GitHub Users + +Since GitHub API support for pull request reviews API - the lookup will first search the API and only then for comments. + +**Note** Approvals can only be read by API or comments. Mixing both approvals from API and comments is not supported. + ## Requirements None diff --git a/bot/actions/automerge/automerge_test.go b/bot/actions/automerge/automerge_test.go index c05334e..6d4dd7c 100644 --- a/bot/actions/automerge/automerge_test.go +++ b/bot/actions/automerge/automerge_test.go @@ -1,16 +1,27 @@ package automerge import ( + "testing" + "github.com/bivas/rivi/bot" "github.com/bivas/rivi/bot/mock" "github.com/stretchr/testify/assert" - "testing" ) type mockMergableEventData struct { mock.MockEventData - merged bool - method string + merged bool + method string + reviewers map[string]string + approvals []string +} + +func (m *mockMergableEventData) GetReviewers() map[string]string { + return m.reviewers +} + +func (m *mockMergableEventData) GetApprovals() []string { + return m.approvals } func (m *mockMergableEventData) Merge(mergeMethod string) { @@ -18,6 +29,38 @@ func (m *mockMergableEventData) Merge(mergeMethod string) { m.method = mergeMethod } +func TestNoReviewersAPI(t *testing.T) { + action := action{rule: &rule{}} + action.rule.Defaults() + config := &mock.MockConfiguration{} + meta := &mock.MockEventData{Assignees: []string{"user1"}, Comments: []bot.Comment{ + {Commenter: "user1", Comment: "approved"}, + }} + action.Apply(config, meta) + assert.NotNil(t, action.err, "should be error on api") +} + +func TestMergeWithAPI(t *testing.T) { + action := action{rule: &rule{}} + action.rule.Defaults() + config := &mock.MockConfiguration{} + mockEventData := mock.MockEventData{Assignees: []string{"user1"}} + meta := &mockMergableEventData{MockEventData: mockEventData, approvals: []string{"user1"}} + action.Apply(config, meta) + assert.True(t, meta.merged, "should be merged") + assert.Equal(t, "merge", meta.method, "default should be merge") +} + +func TestMergeWithAPINoApprovals(t *testing.T) { + action := action{rule: &rule{}} + action.rule.Defaults() + config := &mock.MockConfiguration{} + mockEventData := mock.MockEventData{Assignees: []string{"user1"}} + meta := &mockMergableEventData{MockEventData: mockEventData, approvals: []string{"user2"}} + action.Apply(config, meta) + assert.False(t, meta.merged, "should be merged") +} + func TestNotCapableToMerge(t *testing.T) { action := action{rule: &rule{}} action.rule.Defaults() diff --git a/bot/connector/github/builder.go b/bot/connector/github/builder.go index 9abacf6..781d31f 100644 --- a/bot/connector/github/builder.go +++ b/bot/connector/github/builder.go @@ -86,6 +86,7 @@ func (builder *eventDataBuilder) readFromClient(context *builderContext) { context.data.fileNames = fileNames stringSet := util.StringSet{Transformer: filepath.Ext} context.data.fileExt = stringSet.AddAll(fileNames).Values() + context.data.reviewers = context.client.GetReviewers(id) context.data.locked = context.client.Locked(id) } diff --git a/bot/connector/github/client.go b/bot/connector/github/client.go index b3e2250..41f71c3 100644 --- a/bot/connector/github/client.go +++ b/bot/connector/github/client.go @@ -177,6 +177,21 @@ func (c *ghClient) AddComment(issue int, comment string) bot.Comment { } } +func (c *ghClient) GetReviewers(issue int) map[string]string { + result := make(map[string]string) + reviews, _, err := c.client.PullRequests.ListReviews(context.Background(), c.owner, c.repo, issue, nil) + if err != nil { + util.Logger.Error("Unable to get reviewers for issue %d. %s", issue, err) + return result + } + for _, review := range reviews { + user := strings.ToLower(*review.User.Login) + state := strings.ToLower(*review.State) + result[user] = state + } + return result +} + func (c *ghClient) Merge(issue int, mergeMethod string) { options := &github.PullRequestOptions{MergeMethod: mergeMethod} _, _, err := c.client.PullRequests.Merge(context.Background(), c.owner, c.repo, issue, "", options) diff --git a/bot/connector/github/event_data.go b/bot/connector/github/event_data.go index ee832f7..28c19e2 100644 --- a/bot/connector/github/event_data.go +++ b/bot/connector/github/event_data.go @@ -1,6 +1,9 @@ package github -import "github.com/bivas/rivi/bot" +import ( + "github.com/bivas/rivi/bot" + "github.com/bivas/rivi/util" +) type eventData struct { client *ghClient @@ -21,6 +24,21 @@ type eventData struct { assignees []string comments []bot.Comment payload []byte + reviewers map[string]string +} + +func (d *eventData) GetReviewers() map[string]string { + return d.reviewers +} + +func (d *eventData) GetApprovals() []string { + result := util.StringSet{} + for reviewer, state := range d.reviewers { + if state == "approve" { + result.Add(reviewer) + } + } + return result.Values() } func (d *eventData) Lock() {