Add support for enforcing regex rules on PR Title (#256)

This PR includes changes which add functionality for disapproving pull requests which do not comply by defined title formatting requirements as mentioned in #244.

- A new predicate `title` is added for defining allowed (`not_match`) and disallowed (`match`) regex patterns on a pull request title.
- The `disapproval` policy is extended to allow predicates just as individual `approval_rules` do. However, whereas an approval rule may only allow approvals subject to passing predicates, the `disapproval` policy will only allow disapprovals subject to its own predicates all failing. Passing predicates on the `disapproval` policy will trigger a default disapproval, just as failing predicates on an approval rule will implicitly approve (pass).

README.md

@@ -24,7 +24,7 @@ UI to view the detailed approval status of any pull request.
   + [policy.yml Specification](#policyyml-specification)
   + [Approval Rules](#approval-rules)
   + [Approval Policies](#approval-policies)
-  + [Disapproval](#disapproval)
+  + [Disapproval Policy](#disapproval-policy)
   + [Caveats and Notes](#caveats-and-notes)
     - [Disapproval is Disabled by Default](#disapproval-is-disabled-by-default)
     - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates)
@@ -199,6 +199,18 @@ if:
   has_labels:
     - "label-1"
     - "label-2"
+
+  # "title" is satisfied if the pull request title matches any one of the
+  # patterns within the "matches" list, and does not match all of the patterns
+  # within the "not_matches" list.
+  # e.g. this predicate triggers for titles including "BREAKING CHANGE" or titles
+  # that are not marked as docs/style/chore changes (using conventional commits 
+  # formatting)    
+  title:
+    matches:
+      - "^BREAKING CHANGE: (\\w| )+$"
+    not_matches:
+      - "^(docs|style|chore): (\\w| )+$"
 
 # "options" specifies a set of restrictions on approvals. If the block does not
 # exist, the default values are used.
@@ -325,19 +337,34 @@ Conjunctions can contain more conjunctions (up to a maximum depth of 5):
         - rule4
 ```
 
-### Disapproval
+### Disapproval Policy
 
 Disapproval allows users to explicitly block pull requests if certain changes
 must be made. Any member of in the set of allowed users can disapprove a change
 or revoke another user's disapproval.
 
-Unlike approval, all disapproval options are specified as part of the policy.
-Effectively, there is a single disapproval rule. The `disapproval` policy has
-the following specification:
+Unlike approval, all disapproval predicates and options are specified as part 
+of the policy. Effectively, there is a single disapproval rule. The `disapproval` 
+policy has the following specification:
 
 ```yaml
 # "disapproval" is the top-level key in the policy block.
 disapproval:
+  # "if" specifies a set of predicates which will cause disapproval if any are 
+  # true
+  #  
+  # This block, and every condition within it are optional. If the block does 
+  # not exist, a pull request is only disapproved if a user takes a disapproval 
+  # action.
+  if:
+    # All predicates from the approval rules section are valid here
+    title:
+      not_matches:
+        - "^(fix|feat|chore): (\\w| )+$"
+        - "^BREAKING CHANGE: (\\w| )+$"
+      matches:
+        - "^BLOCKED"
+        
   # "options" sets behavior related to disapproval. If it does not exist, the
   # defaults shown below are used.
   options:

policy/approval/approve.go

@@ -25,15 +25,16 @@ import (
 	"github.com/rs/zerolog"
 
 	"github.com/palantir/policy-bot/policy/common"
+	"github.com/palantir/policy-bot/policy/predicate"
 	"github.com/palantir/policy-bot/pull"
 )
 
 type Rule struct {
-	Name        string     `yaml:"name"`
-	Description string     `yaml:"description"`
-	Predicates  Predicates `yaml:"if"`
-	Options     Options    `yaml:"options"`
-	Requires    Requires   `yaml:"requires"`
+	Name        string               `yaml:"name"`
+	Description string               `yaml:"description"`
+	Predicates  predicate.Predicates `yaml:"if"`
+	Options     Options              `yaml:"options"`
+	Requires    Requires             `yaml:"requires"`
 }
 
 type Options struct {
@@ -115,7 +116,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 
 			res.StatusDescription = desc
 			if desc == "" {
-				res.StatusDescription = "The preconditions of this rule are not satisfied"
+				res.StatusDescription = "A precondition of this rule was satisfied"
 			}
 
 			return

policy/approval/evaluate_test.go

@@ -67,7 +67,7 @@ func TestRules(t *testing.T) {
 	expected := []*Rule{
 		{
 			Name: "rule1",
-			Predicates: Predicates{
+			Predicates: predicate.Predicates{
 				ChangedFiles: &predicate.ChangedFiles{
 					Paths: []common.Regexp{
 						common.NewCompiledRegexp(regexp.MustCompile("path1")),

policy/disapproval/disapprove.go

@@ -23,12 +23,14 @@ import (
 	"github.com/rs/zerolog"
 
 	"github.com/palantir/policy-bot/policy/common"
+	"github.com/palantir/policy-bot/policy/predicate"
 	"github.com/palantir/policy-bot/pull"
 )
 
 type Policy struct {
-	Options  Options  `yaml:"options"`
-	Requires Requires `yaml:"requires"`
+	Predicates predicate.Predicates `yaml:"if"`
+	Options    Options              `yaml:"options"`
+	Requires   Requires             `yaml:"requires"`
 }
 
 type Options struct {
@@ -77,16 +79,24 @@ type Requires struct {
 }
 
 func (p *Policy) Trigger() common.Trigger {
-	dm := p.Options.GetDisapproveMethods()
-	rm := p.Options.GetRevokeMethods()
-
 	t := common.TriggerCommit
-	if len(dm.Comments) > 0 || len(rm.Comments) > 0 {
-		t |= common.TriggerComment
+
+	if !p.Requires.IsEmpty() {
+		dm := p.Options.GetDisapproveMethods()
+		rm := p.Options.GetRevokeMethods()
+
+		if len(dm.Comments) > 0 || len(rm.Comments) > 0 {
+			t |= common.TriggerComment
+		}
+		if dm.GithubReview || rm.GithubReview {
+			t |= common.TriggerReview
+		}
 	}
-	if dm.GithubReview || rm.GithubReview {
-		t |= common.TriggerReview
+
+	for _, predicate := range p.Predicates.Predicates() {
+		t |= predicate.Trigger()
 	}
+
 	return t
 }
 
@@ -96,6 +106,26 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
 	res.Name = "disapproval"
 	res.Status = common.StatusSkipped
 
+	for _, p := range p.Predicates.Predicates() {
+		satisfied, desc, err := p.Evaluate(ctx, prctx)
+
+		if err != nil {
+			res.Error = errors.Wrap(err, "failed to evaluate predicate")
+			return
+		}
+
+		if satisfied {
+			log.Debug().Msgf("disapproving, predicate of type %T was satisfied", p)
+
+			res.Status = common.StatusDisapproved
+			res.StatusDescription = desc
+			if desc == "" {
+				res.StatusDescription = "A precondition of this rule was satisfied"
+			}
+			return
+		}
+	}
+
 	if p.Requires.IsEmpty() {
 		log.Debug().Msg("no users are allowed to disapprove; skipping")
 

policy/disapproval/disapprove_test.go

@@ -17,13 +17,16 @@ package disapproval
 import (
 	"context"
 	"os"
+	"regexp"
 	"testing"
 	"time"
 
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/palantir/policy-bot/policy/common"
+	"github.com/palantir/policy-bot/policy/predicate"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/palantir/policy-bot/pull/pulltest"
 )
@@ -33,6 +36,7 @@ func TestIsDisapproved(t *testing.T) {
 	ctx := logger.WithContext(context.Background())
 
 	prctx := &pulltest.Context{
+		TitleValue: "test: add disapproval predicate test",
 		CommentsValue: []*pull.Comment{
 			{
 				Author:    "disapprover-1",
@@ -75,26 +79,28 @@ func TestIsDisapproved(t *testing.T) {
 	}
 
 	assertDisapproved := func(t *testing.T, p *Policy, expected string) {
-		disapproved, msg, err := p.IsDisapproved(ctx, prctx)
-		require.NoError(t, err)
+		res := p.Evaluate(ctx, prctx)
 
-		if assert.True(t, disapproved, "pull request was not disapproved") {
-			assert.Equal(t, expected, msg)
+		require.NoError(t, res.Error)
+
+		if assert.Equal(t, common.StatusDisapproved, res.Status, "pull request was not disapproved") {
+			assert.Equal(t, expected, res.StatusDescription)
 		}
 	}
 
 	assertSkipped := func(t *testing.T, p *Policy, expected string) {
-		disapproved, msg, err := p.IsDisapproved(ctx, prctx)
-		require.NoError(t, err)
+		res := p.Evaluate(ctx, prctx)
+
+		require.NoError(t, res.Error)
 
-		if assert.False(t, disapproved, "pull request was incorrectly disapproved") {
-			assert.Equal(t, expected, msg)
+		if assert.Equal(t, common.StatusSkipped, res.Status, "pull request was incorrectly disapproved") {
+			assert.Equal(t, expected, res.StatusDescription)
 		}
 	}
 
 	t.Run("skippedWithNoRequires", func(t *testing.T) {
 		p := &Policy{}
-		assertSkipped(t, p, "No disapprovals")
+		assertSkipped(t, p, "No disapproval policy is specified or the policy is empty")
 	})
 
 	t.Run("singleUserDisapproves", func(t *testing.T) {
@@ -145,6 +151,32 @@ func TestIsDisapproved(t *testing.T) {
 
 		assertDisapproved(t, p, "Disapproved by disapprover-4")
 	})
+
+	t.Run("predicateDisapproves", func(t *testing.T) {
+		p := &Policy{}
+		p.Predicates = predicate.Predicates{
+			Title: &predicate.Title{
+				NotMatches: []common.Regexp{
+					common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|docs)")),
+				},
+			},
+		}
+
+		assertDisapproved(t, p, "Title doesn't match a NotMatch pattern")
+	})
+
+	t.Run("predicateDoesNotDisapprove", func(t *testing.T) {
+		p := &Policy{}
+		p.Predicates = predicate.Predicates{
+			Title: &predicate.Title{
+				NotMatches: []common.Regexp{
+					common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|docs|test)")),
+				},
+			},
+		}
+
+		assertSkipped(t, p, "No disapproval policy is specified or the policy is empty")
+	})
 }
 
 func date(hour int) time.Time {

policy/predicate/file.go

@@ -45,7 +45,7 @@ func (pred *ChangedFiles) Evaluate(ctx context.Context, prctx pull.Context) (boo
 		}
 
 		if anyMatches(pred.Paths, f.Filename) {
-			return true, "", nil
+			return true, f.Filename + " was changed", nil
 		}
 	}
 
@@ -163,12 +163,3 @@ func (pred *ModifiedLines) Trigger() common.Trigger {
 }
 
 var _ Predicate = &ModifiedLines{}
-
-func anyMatches(re []common.Regexp, s string) bool {
-	for _, r := range re {
-		if r.Matches(s) {
-			return true
-		}
-	}
-	return false
-}