Repository: palantir/policy-bot (go)

Issue:
Overriding approval methods clears all defaults
We had a problem today where a policy added alternate comments for some rules, but expected the default behavior of counting GitHub reviews to still work. With the current implementation, setting any property of the `methods` block removes the defaults for all properties.

Consider setting defaults for fields that are not explicitly set by the policy file to reduce confusion, as the docs imply.

Pull request description:
Fix issue with method defaults
When certain attributes are defined under 'methods', the other undefined attributes don't line up with the promised defaults. This PR resolves this issue by checking whether certain attributes were not specified and then adds defaults for them.

Closes #434 

Patch:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 4ecc0d2..11bbce0 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -58,14 +58,18 @@ type RequestReview struct {
 func (opts *Options) GetMethods() *common.Methods {
 	methods := opts.Methods
 	if methods == nil {
-		methods = &common.Methods{
-			Comments: []string{
-				":+1:",
-				"👍",
-			},
-			GithubReview: true,
+		methods = &common.Methods{}
+	}
+	if methods.Comments == nil {
+		methods.Comments = []string{
+			":+1:",
+			"👍",
 		}
 	}
+	if methods.GithubReview == nil {
+		defaultGithubReview := true
+		methods.GithubReview = &defaultGithubReview
+	}
 
 	methods.GithubReviewState = pull.ReviewApproved
 	return methods
@@ -85,7 +89,7 @@ func (r *Rule) Trigger() common.Trigger {
 		if len(m.Comments) > 0 || len(m.CommentPatterns) > 0 {
 			t |= common.TriggerComment
 		}
-		if m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
+		if m.GithubReview != nil && *m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
 			t |= common.TriggerReview
 		}
 	}
diff --git a/policy/common/methods.go b/policy/common/methods.go
index df0bca8..4164ed2 100644
--- a/policy/common/methods.go
+++ b/policy/common/methods.go
@@ -25,7 +25,7 @@ import (
 type Methods struct {
 	Comments                    []string `yaml:"comments,omitempty"`
 	CommentPatterns             []Regexp `yaml:"comment_patterns,omitempty"`
-	GithubReview                bool     `yaml:"github_review,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
@@ -72,7 +72,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 		}
 	}
 
-	if m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
+	if m.GithubReview != nil && *m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
 		reviews, err := prctx.Reviews()
 		if err != nil {
 			return nil, err
diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go
index b874765..7c46a7c 100644
--- a/policy/disapproval/disapprove.go
+++ b/policy/disapproval/disapprove.go
@@ -44,12 +44,13 @@ type Methods struct {
 func (opts *Options) GetDisapproveMethods() *common.Methods {
 	m := opts.Methods.Disapprove
 	if m == nil {
+		githubReview := true
 		m = &common.Methods{
 			Comments: []string{
 				":-1:",
 				"👎",
 			},
-			GithubReview: true,
+			GithubReview: &githubReview,
 		}
 	}
 
@@ -60,12 +61,13 @@ func (opts *Options) GetDisapproveMethods() *common.Methods {
 func (opts *Options) GetRevokeMethods() *common.Methods {
 	m := opts.Methods.Revoke
 	if m == nil {
+		githubReview := true
 		m = &common.Methods{
 			Comments: []string{
 				":+1:",
 				"👍",
 			},
-			GithubReview: true,
+			GithubReview: &githubReview,
 		}
 	}
 
@@ -87,7 +89,7 @@ func (p *Policy) Trigger() common.Trigger {
 		if len(dm.Comments) > 0 || len(rm.Comments) > 0 {
 			t |= common.TriggerComment
 		}
-		if dm.GithubReview || rm.GithubReview {
+		if dm.GithubReview != nil && *dm.GithubReview || rm.GithubReview != nil && *rm.GithubReview {
 			t |= common.TriggerReview
 		}
 	}


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index dea24b0..311b4cd 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -542,10 +542,11 @@ func TestTrigger(t *testing.T) {
 	})
 
 	t.Run("triggerReviewForGithubReview", func(t *testing.T) {
+		defaultGithubReview := true
 		r := &Rule{
 			Options: Options{
 				Methods: &common.Methods{
-					GithubReview: true,
+					GithubReview: &defaultGithubReview,
 				},
 			},
 			Requires: Requires{
diff --git a/policy/approval/evaluate_test.go b/policy/approval/evaluate_test.go
index a1280e7..56647b6 100644
--- a/policy/approval/evaluate_test.go
+++ b/policy/approval/evaluate_test.go
@@ -63,6 +63,12 @@ func TestRules(t *testing.T) {
 	var rules []*Rule
 	require.NoError(t, yaml.UnmarshalStrict([]byte(ruleText), &rules))
 
+	defaultGithubReview := true
+	defaultComments := []string{
+		":+1:",
+		"👍",
+	}
+	comments := []string{"+1"}
 	expected := []*Rule{
 		{
 			Name: "rule1",
@@ -97,8 +103,8 @@ func TestRules(t *testing.T) {
 				AllowContributor: true,
 				// InvalidateOnPush: true,
 				Methods: &common.Methods{
-					Comments:     []string{"+1"},
-					GithubReview: true,
+					Comments:     comments,
+					GithubReview: &defaultGithubReview,
 				},
 			},
 			Requires: Requires{
@@ -113,6 +119,64 @@ func TestRules(t *testing.T) {
 	}
 
 	require.True(t, reflect.DeepEqual(expected, rules))
+
+	optionsText := `
+allow_author: true
+allow_contributor: true
+invalidate_on_push: true
+methods:
+  comments: ["+1"]
+`
+	expectedMethods := &common.Methods{
+		Comments:     comments,
+		GithubReview: &defaultGithubReview,
+	}
+	var options *Options
+	require.NoError(t, yaml.UnmarshalStrict([]byte(optionsText), &options))
+
+	methods := options.GetMethods()
+
+	require.True(t, reflect.DeepEqual(expectedMethods.Comments, methods.Comments))
+	require.True(t, reflect.DeepEqual(*expectedMethods.GithubReview, *methods.GithubReview))
+
+	optionsText = `
+allow_author: true
+allow_contributor: true
+invalidate_on_push: true
+methods:
+  github_review: false
+`
+	falseGithubReview := false
+	expectedMethods = &common.Methods{
+		Comments:     defaultComments,
+		GithubReview: &falseGithubReview,
+	}
+
+	var optionsTwo *Options
+	require.NoError(t, yaml.UnmarshalStrict([]byte(optionsText), &optionsTwo))
+
+	methods = optionsTwo.GetMethods()
+
+	require.True(t, reflect.DeepEqual(expectedMethods.Comments, methods.Comments))
+	require.True(t, reflect.DeepEqual(*expectedMethods.GithubReview, *methods.GithubReview))
+
+	optionsText = `
+allow_author: true
+allow_contributor: true
+invalidate_on_push: true
+`
+	expectedMethods = &common.Methods{
+		Comments:     defaultComments,
+		GithubReview: &defaultGithubReview,
+	}
+	var optionsThree *Options
+	require.NoError(t, yaml.UnmarshalStrict([]byte(optionsText), &optionsThree))
+
+	methods = optionsThree.GetMethods()
+
+	require.True(t, reflect.DeepEqual(expectedMethods.Comments, methods.Comments))
+	require.True(t, reflect.DeepEqual(*expectedMethods.GithubReview, *methods.GithubReview))
+
 }
 
 type mockRequirement struct {
diff --git a/policy/common/methods_test.go b/policy/common/methods_test.go
index 44edb56..b1918b8 100644
--- a/policy/common/methods_test.go
+++ b/policy/common/methods_test.go
@@ -118,8 +118,9 @@ func TestCandidates(t *testing.T) {
 	})
 
 	t.Run("githubReviewCommentPatterns", func(t *testing.T) {
+		githubReview := true
 		m := &Methods{
-			GithubReview:      true,
+			GithubReview:      &githubReview,
 			GithubReviewState: pull.ReviewApproved,
 			GithubReviewCommentPatterns: []Regexp{
 				NewCompiledRegexp(regexp.MustCompile("(?i)nice")),
@@ -137,8 +138,9 @@ func TestCandidates(t *testing.T) {
 	})
 
 	t.Run("reviews", func(t *testing.T) {
+		githubReview := true
 		m := &Methods{
-			GithubReview:      true,
+			GithubReview:      &githubReview,
 			GithubReviewState: pull.ReviewChangesRequested,
 		}
 
@@ -152,9 +154,10 @@ func TestCandidates(t *testing.T) {
 	})
 
 	t.Run("deduplicate", func(t *testing.T) {
+		githubReview := true
 		m := &Methods{
 			Comments:          []string{":+1:", ":lgtm:"},
-			GithubReview:      true,
+			GithubReview:      &githubReview,
 			GithubReviewState: pull.ReviewApproved,
 		}