Fix issue with method defaults (#447)

palantir · Jul 6, 2022 · 30d816f · 30d816f
1 parent 5998aa9
commit 30d816f
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 18 deletions.
diff --git 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/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
@@ -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.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/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,
 		}
 

diff --git 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
 		}
 	}