Repository: palantir/policy-bot
Pull request #365 (2022-01-14 21:22:05)

fix: triggers should include all comments
Noticed in my testing of https://github.com/palantir/policy-bot/pull/361 the code responsible for triggering evaluation was incomplete. Rules for methods that were more recently added weren't being evaluated until a user clicked on the details page in the github status link instead of at the time of the webhook event. This attempts to solve that.

Also added some tests for the Trigger method so that we can catch it in the future.


Patch:
diff --git a/README.md b/README.md
index 9172010..7400a1d 100644
--- a/README.md
+++ b/README.md
@@ -126,7 +126,8 @@ define a `remote` key. Only 1 level of remote configuration is supported by desi
 
 ```yaml
 # The remote repository to read the policy file from. This is required, and must
-# be in the form of "org/repo-name". Must be a public repository.
+# be in the form of "org/repo-name". The policy bot github app must have read
+# access to this repository.
 remote: org/repo-name
 
 # The path to the policy config file in the remote repository. If none is
@@ -323,6 +324,7 @@ options:
   request_review:
     # False by default
     enabled: true
+
     # mode modifies how reviewers are selected. `all-users` will request all users
     # who are able to approve the pending rule. `random-users` selects a small
     # set of random users based on the required count of approvals. `teams` will 
@@ -340,6 +342,7 @@ options:
     comments:
       - ":+1:"
       - "👍"
+
     # If a comment matches a regular expression in this list, it counts as
     # approval. Defaults to an empty list.
     #
@@ -347,9 +350,11 @@ 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. 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.
@@ -564,12 +569,6 @@ ignored. If this happens, you will need to reapprove the pull request.
 
 This feature has [security implications](#update-merge-conflicts).
 
-#### Private Repositories
-
-`policy-bot` works with private repositories, but currently does not support
-pull requests from private _forks_ of private repositories due to GitHub API
-limitations. Please file an issue if this functionality is important to you.
-
 #### Automatically Requesting Reviewers
 
 `policy-bot` can automatically request reviewers for all pending rules
diff --git a/config/policy-bot.example.yml b/config/policy-bot.example.yml
index c00839a..1a07514 100644
--- a/config/policy-bot.example.yml
+++ b/config/policy-bot.example.yml
@@ -15,9 +15,11 @@ server:
 logging:
   # If true, logs are printed in human-readable form. We recommend using
   # "false" to output JSON-formatted logs in production
+  # Can also be set by the POLICYBOT_LOG_PRETTY environment variable.
   text: false
   # Set a minimum logging level threshold
   # Choose from: debug, info, warn, error
+  # Can also be set by the POLICYBOT_LOG_LEVEL environment variable.
   level: debug
 
 # Options for the GitHub response cache. When the cache reaches max_size, the
@@ -78,17 +80,21 @@ sessions:
 # Options for application behavior. The defaults are shown below.
 #
 # options:
-#   # The path to the policy file in a repository.
+#   # The path to the policy file in a repository. Can also be set by the
+#   # POLICYBOT_POLICY_PATH environment variable.
 #   policy_path: .policy.yml
 #
 #   # The name of an organization repository to look in for a shared policy if
-#   # a repository does not define a policy file.
+#   # a repository does not define a policy file. Can also be set by the
+#   # POLICYBOT_SHARED_REPOSITORY environment variable.
 #   shared_repository: .github
 #
 #   # The path to the policy file in the shared organization repository.
+#   # Can also be set by the POLICYBOT_SHARED_POLICY_PATH environment variable.
 #   shared_policy_path: policy.yml
 #
-#   # The context prefix for status checks created by the bot.
+#   # The context prefix for status checks created by the bot. Can also be set by the
+#   # POLICYBOT_STATUS_CHECK_CONTEXT environment variable.
 #   status_check_context: policy-bot
 
 # Options for locating the frontend files. By default, the server uses appropriate
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 1ac32da..d5fd050 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -82,10 +82,10 @@ func (r *Rule) Trigger() common.Trigger {
 
 	if r.Requires.Count > 0 {
 		m := r.Options.GetMethods()
-		if len(m.Comments) > 0 {
+		if len(m.Comments) > 0 || len(m.CommentPatterns) > 0 {
 			t |= common.TriggerComment
 		}
-		if m.GithubReview {
+		if m.GithubReview || len(m.GithubReviewCommentPatterns) > 0 {
 			t |= common.TriggerReview
 		}
 	}
diff --git a/server/config.go b/server/config.go
index aa0ebe7..9eab937 100644
--- a/server/config.go
+++ b/server/config.go
@@ -65,7 +65,7 @@ func ParseConfig(bytes []byte) (*Config, error) {
 		return nil, errors.Wrapf(err, "failed unmarshalling yaml")
 	}
 
-	c.Options.FillDefaults()
+	c.Options.SetValuesFromEnv("POLICYBOT_OPTIONS_")
 	c.Server.SetValuesFromEnv("POLICYBOT_")
 	c.Github.SetValuesFromEnv("")
 
diff --git a/server/handler/base.go b/server/handler/base.go
index 620895c..b9a1aa1 100644
--- a/server/handler/base.go
+++ b/server/handler/base.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
+	"os"
 	"strings"
 	"time"
 
@@ -75,7 +76,7 @@ type PullEvaluationOptions struct {
 	Deprecated_AppName string `yaml:"app_name"`
 }
 
-func (p *PullEvaluationOptions) FillDefaults() {
+func (p *PullEvaluationOptions) fillDefaults() {
 	if p.PolicyPath == "" {
 		p.PolicyPath = DefaultPolicyPath
 	}
@@ -91,6 +92,14 @@ func (p *PullEvaluationOptions) FillDefaults() {
 	}
 }
 
+func (p *PullEvaluationOptions) SetValuesFromEnv(prefix string) {
+	setStringFromEnv("POLICY_PATH", prefix, &p.PolicyPath)
+	setStringFromEnv("SHARED_REPOSITORY", prefix, &p.SharedRepository)
+	setStringFromEnv("SHARED_POLICY_PATH", prefix, &p.SharedPolicyPath)
+	setStringFromEnv("STATUS_CHECK_CONTEXT", prefix, &p.StatusCheckContext)
+	p.fillDefaults()
+}
+
 func (b *Base) PostStatus(ctx context.Context, prctx pull.Context, client *github.Client, state, message string) {
 	logger := zerolog.Ctx(ctx)
 
@@ -381,3 +390,11 @@ func selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest {
 
 	return req
 }
+
+func setStringFromEnv(key, prefix string, value *string) bool {
+	if v, ok := os.LookupEnv(prefix + key); ok {
+		*value = v
+		return true
+	}
+	return false
+}


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index d6ff78c..dea24b0 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -17,6 +17,7 @@ package approval
 import (
 	"context"
 	"os"
+	"regexp"
 	"testing"
 	"time"
 
@@ -497,6 +498,84 @@ func TestIsApproved(t *testing.T) {
 	})
 }
 
+func TestTrigger(t *testing.T) {
+	t.Run("triggerCommitOnRules", func(t *testing.T) {
+		r := &Rule{}
+
+		assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %", r.Trigger(), common.TriggerCommit)
+	})
+
+	t.Run("triggerCommentOnComments", func(t *testing.T) {
+		r := &Rule{
+			Options: Options{
+				Methods: &common.Methods{
+					Comments: []string{
+						"lgtm",
+					},
+				},
+			},
+			Requires: Requires{
+				Count: 1,
+			},
+		}
+
+		assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %s", r.Trigger(), common.TriggerCommit)
+		assert.True(t, r.Trigger().Matches(common.TriggerComment), "expected %s to match %s", r.Trigger(), common.TriggerComment)
+	})
+
+	t.Run("triggerCommentOnCommentPatterns", func(t *testing.T) {
+		r := &Rule{
+			Options: Options{
+				Methods: &common.Methods{
+					CommentPatterns: []common.Regexp{
+						common.NewCompiledRegexp(regexp.MustCompile("(?i)nice")),
+					},
+				},
+			},
+			Requires: Requires{
+				Count: 1,
+			},
+		}
+
+		assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %s", r.Trigger(), common.TriggerCommit)
+		assert.True(t, r.Trigger().Matches(common.TriggerComment), "expected %s to match %s", r.Trigger(), common.TriggerComment)
+	})
+
+	t.Run("triggerReviewForGithubReview", func(t *testing.T) {
+		r := &Rule{
+			Options: Options{
+				Methods: &common.Methods{
+					GithubReview: true,
+				},
+			},
+			Requires: Requires{
+				Count: 1,
+			},
+		}
+
+		assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %s", r.Trigger(), common.TriggerCommit)
+		assert.True(t, r.Trigger().Matches(common.TriggerReview), "expected %s to match %s", r.Trigger(), common.TriggerReview)
+	})
+
+	t.Run("triggerReviewForGithubReviewCommentPatterns", func(t *testing.T) {
+		r := &Rule{
+			Options: Options{
+				Methods: &common.Methods{
+					GithubReviewCommentPatterns: []common.Regexp{
+						common.NewCompiledRegexp(regexp.MustCompile("(?i)nice")),
+					},
+				},
+			},
+			Requires: Requires{
+				Count: 1,
+			},
+		}
+
+		assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %s", r.Trigger(), common.TriggerCommit)
+		assert.True(t, r.Trigger().Matches(common.TriggerReview), "expected %s to match %s", r.Trigger(), common.TriggerReview)
+	})
+}
+
 func newTime(t time.Time) *time.Time {
 	return &t
 }