Repository: palantir/policy-bot
Pull request #361 (2022-01-07 02:26:54)

feat: add comment matching to github review approval with new option
Closes https://github.com/palantir/policy-bot/issues/359

As discussed in the attached issue, this feature would support new method options for `github_review_comments:` and `github_review_comment_patterns:` creating parity with issue comment approval for github review approval comment bodies. This would help in use cases where github review approval alone isn't sufficient.

I went with nesting the logic in the existing reviews for loop but first checking to confirm the review state is approved. That would make this feature work if `github_review` is provided in the config and set to `true`, or not provided in the config defaulting to `true`. As written, this feature doesn't work if `github_review: false` explicitly in the config. That doesn't seem surprising behavior to me, but let me know if there's an approach that makes better sense to accomplish the same.


Patch:
diff --git a/README.md b/README.md
index 30c9acc..9172010 100644
--- a/README.md
+++ b/README.md
@@ -347,8 +347,14 @@ options:
     # See the Notes on YAML Syntax section of this README for more information.
     comment_patterns:
       - "^Signed-off by \\s+$"
-    # If true, GitHub reviews can be used for approval. Default is true.
+    # If true, GitHub reviews can be used for approval. All GitHub review approvals
+    # will be accepted as approval candidates. Default is true.
     github_review: true
+    # Just like the "comment_patterns" option, but for GitHub reviews. Only GitHub
+    # review approvals matching the included patterns will be accepted as
+    # approval candidates. Defaults to an empty list.
+    github_review_comment_patterns:
+      - '\b(?i)domain\s*lgtm\b'
 
 # "requires" specifies the approval requirements for the rule. If the block
 # does not exist, the rule is automatically approved.
diff --git a/policy/common/methods.go b/policy/common/methods.go
index 05baec4..df0bca8 100644
--- a/policy/common/methods.go
+++ b/policy/common/methods.go
@@ -23,9 +23,10 @@ import (
 )
 
 type Methods struct {
-	Comments        []string `yaml:"comments,omitempty"`
-	CommentPatterns []Regexp `yaml:"comment_patterns,omitempty"`
-	GithubReview    bool     `yaml:"github_review,omitempty"`
+	Comments                    []string `yaml:"comments,omitempty"`
+	CommentPatterns             []Regexp `yaml:"comment_patterns,omitempty"`
+	GithubReview                bool     `yaml:"github_review,omitempty"`
+	GithubReviewCommentPatterns []Regexp `yaml:"github_review_comment_patterns,omitempty"`
 
 	// If GithubReview is true, GithubReviewState is the state a review must
 	// have to be considered a candidated. It is currently excluded from
@@ -71,7 +72,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 		}
 	}
 
-	if m.GithubReview {
+	if m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
 		reviews, err := prctx.Reviews()
 		if err != nil {
 			return nil, err
@@ -79,11 +80,21 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 
 		for _, r := range reviews {
 			if r.State == m.GithubReviewState {
-				candidates = append(candidates, &Candidate{
-					User:      r.Author,
-					CreatedAt: r.CreatedAt,
-					UpdatedAt: r.UpdatedAt,
-				})
+				if len(m.GithubReviewCommentPatterns) > 0 {
+					if m.githubReviewCommentMatches(r.Body) {
+						candidates = append(candidates, &Candidate{
+							User:      r.Author,
+							CreatedAt: r.CreatedAt,
+							UpdatedAt: r.UpdatedAt,
+						})
+					}
+				} else {
+					candidates = append(candidates, &Candidate{
+						User:      r.Author,
+						CreatedAt: r.CreatedAt,
+						UpdatedAt: r.UpdatedAt,
+					})
+				}
 			}
 		}
 	}
@@ -121,3 +132,12 @@ func (m *Methods) CommentMatches(commentBody string) bool {
 	}
 	return false
 }
+
+func (m *Methods) githubReviewCommentMatches(commentBody string) bool {
+	for _, pattern := range m.GithubReviewCommentPatterns {
+		if pattern.Matches(commentBody) {
+			return true
+		}
+	}
+	return false
+}


Test patch:
diff --git a/policy/common/methods_test.go b/policy/common/methods_test.go
index 4a95179..44edb56 100644
--- a/policy/common/methods_test.go
+++ b/policy/common/methods_test.go
@@ -64,12 +64,25 @@ func TestCandidates(t *testing.T) {
 				CreatedAt: now.Add(3 * time.Minute),
 				Author:    "mhaypenny",
 				State:     pull.ReviewChangesRequested,
+				Body:      "pr needs work",
 			},
 			{
 				CreatedAt: now.Add(5 * time.Minute),
 				Author:    "ttest",
 				State:     pull.ReviewApproved,
 			},
+			{
+				CreatedAt: now.Add(7 * time.Minute),
+				Author:    "santaclaus",
+				Body:      "nice",
+				State:     pull.ReviewApproved,
+			},
+			{
+				CreatedAt: now.Add(9 * time.Minute),
+				Author:    "dasherdancer",
+				Body:      "nIcE",
+				State:     pull.ReviewApproved,
+			},
 		},
 	}
 
@@ -104,6 +117,25 @@ func TestCandidates(t *testing.T) {
 		assert.Equal(t, "mhaypenny", cs[0].User)
 	})
 
+	t.Run("githubReviewCommentPatterns", func(t *testing.T) {
+		m := &Methods{
+			GithubReview:      true,
+			GithubReviewState: pull.ReviewApproved,
+			GithubReviewCommentPatterns: []Regexp{
+				NewCompiledRegexp(regexp.MustCompile("(?i)nice")),
+			},
+		}
+
+		cs, err := m.Candidates(ctx, prctx)
+		require.NoError(t, err)
+
+		sort.Sort(CandidatesByCreationTime(cs))
+
+		require.Len(t, cs, 2, "incorrect number of candidates found")
+		assert.Equal(t, "santaclaus", cs[0].User)
+		assert.Equal(t, "dasherdancer", cs[1].User)
+	})
+
 	t.Run("reviews", func(t *testing.T) {
 		m := &Methods{
 			GithubReview:      true,
@@ -131,9 +163,11 @@ func TestCandidates(t *testing.T) {
 
 		sort.Sort(CandidatesByCreationTime(cs))
 
-		require.Len(t, cs, 2, "incorrect number of candidates found")
+		require.Len(t, cs, 4, "incorrect number of candidates found")
 		assert.Equal(t, "mhaypenny", cs[0].User)
 		assert.Equal(t, "ttest", cs[1].User)
+		assert.Equal(t, "santaclaus", cs[2].User)
+		assert.Equal(t, "dasherdancer", cs[3].User)
 	})
 }