Add Gerrit pipeline trigger requirements

This updates the Gerrit driver to match the pattern in the GitHub
driver where instead of specifying individual trigger
requirements such as "require-approvals", instead a complete ref
filter (a la "requirements") can be embedded in the trigger
filter.

The "require-approvals" and "reject-approvals" attributes are
deprecated in favor of the new approach.

Additionally, all require filters in Gerrit are now available as
reject filters.

And finally, the Gerrit filters are updated to return
FalseWithReason so that log messages are more useful, and the
Github filters are updated to improve the language, avoid
apostraphes for ease of grepping, and match the new Gerrit
filters.

Change-Id: Ia9c749f1c8e318fe01e84e52831a9d0d2c10b203

4 files changed, 267 insertions, 191 deletions

diff --git a/zuul/driver/gerrit/gerritmodel.py b/zuul/driver/gerrit/gerritmodel.py
index 0ac3e7f9d..093c04f25 100644
--- a/zuul/driver/gerrit/gerritmodel.py
+++ b/zuul/driver/gerrit/gerritmodel.py
@@ -19,8 +19,8 @@ import urllib.parse
 import dateutil.parser
 
 from zuul.model import EventFilter, RefFilter
-from zuul.model import Change, TriggerEvent
-from zuul.driver.util import time_to_seconds
+from zuul.model import Change, TriggerEvent, FalseWithReason
+from zuul.driver.util import time_to_seconds, to_list
 from zuul import exceptions
 
 EMPTY_GIT_REF = '0' * 40  # git sha of all zeros, used during creates/deletes
@@ -243,110 +243,32 @@ class GerritTriggerEvent(TriggerEvent):
         return 'change-abandoned' == self.type
 
 
-class GerritApprovalFilter(object):
-    def __init__(self, required_approvals=[], reject_approvals=[]):
-        self._required_approvals = copy.deepcopy(required_approvals)
-        self.required_approvals = self._tidy_approvals(
-            self._required_approvals)
-        self._reject_approvals = copy.deepcopy(reject_approvals)
-        self.reject_approvals = self._tidy_approvals(self._reject_approvals)
-
-    def _tidy_approvals(self, approvals):
-        for a in approvals:
-            for k, v in a.items():
-                if k == 'username':
-                    a['username'] = re.compile(v)
-                elif k == 'email':
-                    a['email'] = re.compile(v)
-                elif k == 'newer-than':
-                    a[k] = time_to_seconds(v)
-                elif k == 'older-than':
-                    a[k] = time_to_seconds(v)
-        return approvals
-
-    def _match_approval_required_approval(self, rapproval, approval):
-        # Check if the required approval and approval match
-        if 'description' not in approval:
-            return False
-        now = time.time()
-        by = approval.get('by', {})
-        for k, v in rapproval.items():
-            if k == 'username':
-                if (not v.search(by.get('username', ''))):
-                    return False
-            elif k == 'email':
-                if (not v.search(by.get('email', ''))):
-                    return False
-            elif k == 'newer-than':
-                t = now - v
-                if (approval['grantedOn'] < t):
-                    return False
-            elif k == 'older-than':
-                t = now - v
-                if (approval['grantedOn'] >= t):
-                    return False
-            else:
-                if not isinstance(v, list):
-                    v = [v]
-                if (approval['description'] != k or
-                        int(approval['value']) not in v):
-                    return False
-        return True
-
-    def matchesApprovals(self, change):
-        if self.required_approvals or self.reject_approvals:
-            if not hasattr(change, 'number'):
-                # Not a change, no reviews
-                return False
-        if self.required_approvals and not change.approvals:
-            # A change with no approvals can not match
-            return False
-
-        # TODO(jhesketh): If we wanted to optimise this slightly we could
-        # analyse both the REQUIRE and REJECT filters by looping over the
-        # approvals on the change and keeping track of what we have checked
-        # rather than needing to loop on the change approvals twice
-        return (self.matchesRequiredApprovals(change) and
-                self.matchesNoRejectApprovals(change))
-
-    def matchesRequiredApprovals(self, change):
-        # Check if any approvals match the requirements
-        for rapproval in self.required_approvals:
-            matches_rapproval = False
-            for approval in change.approvals:
-                if self._match_approval_required_approval(rapproval, approval):
-                    # We have a matching approval so this requirement is
-                    # fulfilled
-                    matches_rapproval = True
-                    break
-            if not matches_rapproval:
-                return False
-        return True
-
-    def matchesNoRejectApprovals(self, change):
-        # Check to make sure no approvals match a reject criteria
-        for rapproval in self.reject_approvals:
-            for approval in change.approvals:
-                if self._match_approval_required_approval(rapproval, approval):
-                    # A reject approval has been matched, so we reject
-                    # immediately
-                    return False
-        # To get here no rejects can have been matched so we should be good to
-        # queue
-        return True
-
-
-class GerritEventFilter(EventFilter, GerritApprovalFilter):
+class GerritEventFilter(EventFilter):
     def __init__(self, connection_name, trigger, types=[], branches=[],
                  refs=[], event_approvals={}, comments=[], emails=[],
                  usernames=[], required_approvals=[], reject_approvals=[],
-                 uuid=None, scheme=None, ignore_deletes=True):
+                 uuid=None, scheme=None, ignore_deletes=True,
+                 require=None, reject=None):
 
         EventFilter.__init__(self, connection_name, trigger)
 
-        GerritApprovalFilter.__init__(self,
-                                      required_approvals=required_approvals,
-                                      reject_approvals=reject_approvals)
+        # TODO: Backwards compat, remove after 9.x:
+        if required_approvals and require is None:
+            require = {'approval': required_approvals}
+        if reject_approvals and reject is None:
+            reject = {'approval': reject_approvals}
+
+        if require:
+            self.require_filter = GerritRefFilter.requiresFromConfig(
+                connection_name, require)
+        else:
+            self.require_filter = None
+
+        if reject:
+            self.reject_filter = GerritRefFilter.rejectFromConfig(
+                connection_name, reject)
+        else:
+            self.reject_filter = None
 
         self._types = types
         self._branches = branches
@@ -384,18 +306,16 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
         if self.event_approvals:
             ret += ' event_approvals: %s' % ', '.join(
                 ['%s:%s' % a for a in self.event_approvals.items()])
-        if self.required_approvals:
-            ret += ' required_approvals: %s' % ', '.join(
-                ['%s' % a for a in self._required_approvals])
-        if self.reject_approvals:
-            ret += ' reject_approvals: %s' % ', '.join(
-                ['%s' % a for a in self._reject_approvals])
         if self._comments:
             ret += ' comments: %s' % ', '.join(self._comments)
         if self._emails:
             ret += ' emails: %s' % ', '.join(self._emails)
         if self._usernames:
             ret += ' usernames: %s' % ', '.join(self._usernames)
+        if self.require_filter:
+            ret += ' require: %s' % repr(self.require_filter)
+        if self.reject_filter:
+            ret += ' reject: %s' % repr(self.reject_filter)
         ret += '>'
 
         return ret
@@ -410,7 +330,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
             if etype.match(event.type):
                 matches_type = True
         if self.types and not matches_type:
-            return False
+            return FalseWithReason("Types %s do not match %s" % (
+                self.types, event.type))
 
         if event.type == 'pending-check':
             if self.uuid and event.uuid != self.uuid:
@@ -424,7 +345,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
             if branch.match(event.branch):
                 matches_branch = True
         if self.branches and not matches_branch:
-            return False
+            return FalseWithReason("Branches %s do not match %s" % (
+                self.branches, event.branch))
 
         # refs are ORed
         matches_ref = False
@@ -433,11 +355,12 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
                 if ref.match(event.ref):
                     matches_ref = True
         if self.refs and not matches_ref:
-            return False
+            return FalseWithReason(
+                "Refs %s do not match %s" % (self.refs, event.ref))
         if self.ignore_deletes and event.newrev == EMPTY_GIT_REF:
             # If the updated ref has an empty git sha (all 0s),
             # then the ref is being deleted
-            return False
+            return FalseWithReason("Ref deletion events are ignored")
 
         # comments are ORed
         matches_comment_re = False
@@ -451,7 +374,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
                         comment_re.search(comment)):
                         matches_comment_re = True
         if self.comments and not matches_comment_re:
-            return False
+            return FalseWithReason("Comments %s do not match %s" % (
+                self.comments, event.patchsetcomments))
 
         # We better have an account provided by Gerrit to do
         # email filtering.
@@ -464,7 +388,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
                         email_re.search(account_email)):
                     matches_email_re = True
             if self.emails and not matches_email_re:
-                return False
+                return FalseWithReason("Username %s does not match %s" % (
+                    self.emails, account_email))
 
             # usernames are ORed
             account_username = event.account.get('username')
@@ -474,7 +399,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
                     username_re.search(account_username)):
                     matches_username_re = True
             if self.usernames and not matches_username_re:
-                return False
+                return FalseWithReason("Username %s does not match %s" % (
+                    self.usernames, account_username))
 
         # approvals are ANDed
         for category, value in self.event_approvals.items():
@@ -484,29 +410,73 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
                         int(eapp['value']) == int(value)):
                     matches_approval = True
             if not matches_approval:
-                return False
+                return FalseWithReason("Approvals %s do not match %s" % (
+                    self.event_approvals, event.approvals))
 
-        # required approvals are ANDed (reject approvals are ORed)
-        if not self.matchesApprovals(change):
-            return False
+        if self.require_filter:
+            require_filter_result = self.require_filter.matches(change)
+            if not require_filter_result:
+                return require_filter_result
+
+        if self.reject_filter:
+            reject_filter_result = self.reject_filter.matches(change)
+            if not reject_filter_result:
+                return reject_filter_result
 
         return True
 
 
-class GerritRefFilter(RefFilter, GerritApprovalFilter):
-    def __init__(self, connection_name, open=None, current_patchset=None,
-                 wip=None, statuses=[], required_approvals=[],
-                 reject_approvals=[]):
+class GerritRefFilter(RefFilter):
+    def __init__(self, connection_name,
+                 open=None, reject_open=None,
+                 current_patchset=None, reject_current_patchset=None,
+                 wip=None, reject_wip=None,
+                 statuses=[], reject_statuses=[],
+                 required_approvals=[], reject_approvals=[]):
         RefFilter.__init__(self, connection_name)
 
-        GerritApprovalFilter.__init__(self,
-                                      required_approvals=required_approvals,
-                                      reject_approvals=reject_approvals)
-
-        self.open = open
-        self.wip = wip
-        self.current_patchset = current_patchset
+        self._required_approvals = copy.deepcopy(required_approvals)
+        self.required_approvals = self._tidy_approvals(
+            self._required_approvals)
+        self._reject_approvals = copy.deepcopy(reject_approvals)
+        self.reject_approvals = self._tidy_approvals(self._reject_approvals)
         self.statuses = statuses
+        self.reject_statuses = reject_statuses
+
+        if reject_open is not None:
+            self.open = not reject_open
+        else:
+            self.open = open
+        if reject_wip is not None:
+            self.wip = not reject_wip
+        else:
+            self.wip = wip
+        if reject_current_patchset is not None:
+            self.current_patchset = not reject_current_patchset
+        else:
+            self.current_patchset = current_patchset
+
+    @classmethod
+    def requiresFromConfig(cls, connection_name, config):
+        return cls(
+            connection_name=connection_name,
+            open=config.get('open'),
+            current_patchset=config.get('current-patchset'),
+            wip=config.get('wip'),
+            statuses=to_list(config.get('status')),
+            required_approvals=to_list(config.get('approval')),
+        )
+
+    @classmethod
+    def rejectFromConfig(cls, connection_name, config):
+        return cls(
+            connection_name=connection_name,
+            reject_open=config.get('open'),
+            reject_current_patchset=config.get('current-patchset'),
+            reject_wip=config.get('wip'),
+            reject_statuses=to_list(config.get('status')),
+            reject_approvals=to_list(config.get('approval')),
+        )
 
     def __repr__(self):
         ret = '<GerritRefFilter'
@@ -514,10 +484,14 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
         ret += ' connection_name: %s' % self.connection_name
         if self.open is not None:
             ret += ' open: %s' % self.open
+        if self.wip is not None:
+            ret += ' wip: %s' % self.wip
         if self.current_patchset is not None:
             ret += ' current-patchset: %s' % self.current_patchset
         if self.statuses:
             ret += ' statuses: %s' % ', '.join(self.statuses)
+        if self.reject_statuses:
+            ret += ' reject-statuses: %s' % ', '.join(self.reject_statuses)
         if self.required_approvals:
             ret += (' required-approvals: %s' %
                     str(self.required_approvals))
@@ -529,37 +503,138 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
         return ret
 
     def matches(self, change):
+        if self.open is not None:
+            # if a "change" has no number, it's not a change, but a push
+            # and cannot possibly pass this test.
+            if hasattr(change, 'number'):
+                if self.open != change.open:
+                    return FalseWithReason(
+                        "Change does not match open requirement")
+            else:
+                return FalseWithReason("Ref is not a Change")
 
-        filters = [
-            {
-                "required": self.open,
-                "value": change.open
-            },
-            {
-                "required": self.current_patchset,
-                "value": change.is_current_patchset
-            },
-            {
-                "required": self.wip,
-                "value": change.wip
-            },
-        ]
-        configured = filter(lambda x: x["required"] is not None, filters)
-
-        # if a "change" has no number, it's not a change, but a push
-        # and cannot possibly pass this test.
-        if hasattr(change, 'number'):
-            if any(map(lambda x: x["required"] != x["value"], configured)):
-                return False
-        elif configured:
-            return False
+        if self.current_patchset is not None:
+            # if a "change" has no number, it's not a change, but a push
+            # and cannot possibly pass this test.
+            if hasattr(change, 'number'):
+                if self.current_patchset != change.is_current_patchset:
+                    return FalseWithReason(
+                        "Change does not match current patchset requirement")
+            else:
+                return FalseWithReason("Ref is not a Change")
+
+        if self.wip is not None:
+            # if a "change" has no number, it's not a change, but a push
+            # and cannot possibly pass this test.
+            if hasattr(change, 'number'):
+                if self.wip != change.wip:
+                    return FalseWithReason(
+                        "Change does not match WIP requirement")
+            else:
+                return FalseWithReason("Ref is not a Change")
 
         if self.statuses:
             if change.status not in self.statuses:
-                return False
+                return FalseWithReason(
+                    "Required statuses %s do not match %s" % (
+                        self.statuses, change.status))
+        if self.reject_statuses:
+            if change.status in self.reject_statuses:
+                return FalseWithReason(
+                    "Reject statuses %s match %s" % (
+                        self.reject_statuses, change.status))
 
         # required approvals are ANDed (reject approvals are ORed)
-        if not self.matchesApprovals(change):
+        matches_approvals_result = self.matchesApprovals(change)
+        if not matches_approvals_result:
+            return matches_approvals_result
+
+        return True
+
+    def _tidy_approvals(self, approvals):
+        for a in approvals:
+            for k, v in a.items():
+                if k == 'username':
+                    a['username'] = re.compile(v)
+                elif k == 'email':
+                    a['email'] = re.compile(v)
+                elif k == 'newer-than':
+                    a[k] = time_to_seconds(v)
+                elif k == 'older-than':
+                    a[k] = time_to_seconds(v)
+        return approvals
+
+    def _match_approval_required_approval(self, rapproval, approval):
+        # Check if the required approval and approval match
+        if 'description' not in approval:
             return False
+        now = time.time()
+        by = approval.get('by', {})
+        for k, v in rapproval.items():
+            if k == 'username':
+                if (not v.search(by.get('username', ''))):
+                    return False
+            elif k == 'email':
+                if (not v.search(by.get('email', ''))):
+                    return False
+            elif k == 'newer-than':
+                t = now - v
+                if (approval['grantedOn'] < t):
+                    return False
+            elif k == 'older-than':
+                t = now - v
+                if (approval['grantedOn'] >= t):
+                    return False
+            else:
+                if not isinstance(v, list):
+                    v = [v]
+                if (approval['description'] != k or
+                        int(approval['value']) not in v):
+                    return False
+        return True
 
+    def matchesApprovals(self, change):
+        if self.required_approvals or self.reject_approvals:
+            if not hasattr(change, 'number'):
+                # Not a change, no reviews
+                return FalseWithReason("Ref is not a Change")
+        if self.required_approvals and not change.approvals:
+            # A change with no approvals can not match
+            return FalseWithReason("Approvals %s does not match %s" % (
+                self.required_approvals, change.approvals))
+
+        # TODO(jhesketh): If we wanted to optimise this slightly we could
+        # analyse both the REQUIRE and REJECT filters by looping over the
+        # approvals on the change and keeping track of what we have checked
+        # rather than needing to loop on the change approvals twice
+        return (self.matchesRequiredApprovals(change) and
+                self.matchesNoRejectApprovals(change))
+
+    def matchesRequiredApprovals(self, change):
+        # Check if any approvals match the requirements
+        for rapproval in self.required_approvals:
+            matches_rapproval = False
+            for approval in change.approvals:
+                if self._match_approval_required_approval(rapproval, approval):
+                    # We have a matching approval so this requirement is
+                    # fulfilled
+                    matches_rapproval = True
+                    break
+            if not matches_rapproval:
+                return FalseWithReason(
+                    "Required approvals %s do not match %s" % (
+                        self.required_approvals, change.approvals))
+        return True
+
+    def matchesNoRejectApprovals(self, change):
+        # Check to make sure no approvals match a reject criteria
+        for rapproval in self.reject_approvals:
+            for approval in change.approvals:
+                if self._match_approval_required_approval(rapproval, approval):
+                    # A reject approval has been matched, so we reject
+                    # immediately
+                    return FalseWithReason("Reject approvals %s match %s" % (
+                        self.reject_approvals, change.approvals))
+        # To get here no rejects can have been matched so we should be good to
+        # queue
         return True
diff --git a/zuul/driver/gerrit/gerritsource.py b/zuul/driver/gerrit/gerritsource.py
index f42e93254..305d15109 100644
--- a/zuul/driver/gerrit/gerritsource.py
+++ b/zuul/driver/gerrit/gerritsource.py
@@ -20,7 +20,7 @@ from urllib.parse import urlparse
 from zuul.source import BaseSource
 from zuul.model import Project
 from zuul.driver.gerrit.gerritmodel import GerritRefFilter
-from zuul.driver.util import scalar_or_list, to_list
+from zuul.driver.util import scalar_or_list
 from zuul.lib.dependson import find_dependency_headers
 from zuul.zk.change_cache import ChangeKey
 
@@ -206,21 +206,15 @@ class GerritSource(BaseSource):
         return self.connection._getGitwebUrl(project, sha)
 
     def getRequireFilters(self, config):
-        f = GerritRefFilter(
-            connection_name=self.connection.connection_name,
-            open=config.get('open'),
-            current_patchset=config.get('current-patchset'),
-            wip=config.get('wip'),
-            statuses=to_list(config.get('status')),
-            required_approvals=to_list(config.get('approval')),
-        )
+        f = GerritRefFilter.requiresFromConfig(
+            self.connection.connection_name,
+            config)
         return [f]
 
     def getRejectFilters(self, config):
-        f = GerritRefFilter(
-            connection_name=self.connection.connection_name,
-            reject_approvals=to_list(config.get('approval')),
-        )
+        f = GerritRefFilter.rejectFromConfig(
+            self.connection.connection_name,
+            config)
         return [f]
 
     def getRefForChange(self, change):
@@ -244,11 +238,13 @@ def getRequireSchema():
                'current-patchset': bool,
                'wip': bool,
                'status': scalar_or_list(str)}
-
     return require
 
 
 def getRejectSchema():
-    reject = {'approval': scalar_or_list(approval)}
-
+    reject = {'approval': scalar_or_list(approval),
+              'open': bool,
+              'current-patchset': bool,
+              'wip': bool,
+              'status': scalar_or_list(str)}
     return reject
diff --git a/zuul/driver/gerrit/gerrittrigger.py b/zuul/driver/gerrit/gerrittrigger.py
index dc8a7db68..dff5dc32c 100644
--- a/zuul/driver/gerrit/gerrittrigger.py
+++ b/zuul/driver/gerrit/gerrittrigger.py
@@ -16,6 +16,7 @@ import logging
 import voluptuous as v
 from zuul.trigger import BaseTrigger
 from zuul.driver.gerrit.gerritmodel import GerritEventFilter
+from zuul.driver.gerrit import gerritsource
 from zuul.driver.util import scalar_or_list, to_list
 
 
@@ -59,7 +60,9 @@ class GerritTrigger(BaseTrigger):
                 ),
                 uuid=trigger.get('uuid'),
                 scheme=trigger.get('scheme'),
-                ignore_deletes=ignore_deletes
+                ignore_deletes=ignore_deletes,
+                require=trigger.get('require'),
+                reject=trigger.get('reject'),
             )
             efilters.append(f)
 
@@ -101,6 +104,8 @@ def getSchema():
         'approval': scalar_or_list(variable_dict),
         'require-approval': scalar_or_list(approval),
         'reject-approval': scalar_or_list(approval),
+        'require': gerritsource.getRequireSchema(),
+        'reject': gerritsource.getRejectSchema(),
     }
 
     return gerrit_trigger
diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py
index 536cce903..71238d070 100644
--- a/zuul/driver/github/githubmodel.py
+++ b/zuul/driver/github/githubmodel.py
@@ -255,7 +255,7 @@ class GithubEventFilter(EventFilter):
             if etype.match(event.type):
                 matches_type = True
         if self.types and not matches_type:
-            return FalseWithReason("Types %s doesn't match %s" % (
+            return FalseWithReason("Types %s do not match %s" % (
                 self.types, event.type))
 
         # branches are ORed
@@ -264,7 +264,7 @@ class GithubEventFilter(EventFilter):
             if branch.match(event.branch):
                 matches_branch = True
         if self.branches and not matches_branch:
-            return FalseWithReason("Branches %s doesn't match %s" % (
+            return FalseWithReason("Branches %s do not match %s" % (
                 self.branches, event.branch))
 
         # refs are ORed
@@ -275,11 +275,11 @@ class GithubEventFilter(EventFilter):
                     matches_ref = True
         if self.refs and not matches_ref:
             return FalseWithReason(
-                "Refs %s doesn't match %s" % (self.refs, event.ref))
+                "Refs %s do not match %s" % (self.refs, event.ref))
         if self.ignore_deletes and event.newrev == EMPTY_GIT_REF:
             # If the updated ref has an empty git sha (all 0s),
             # then the ref is being deleted
-            return FalseWithReason("Ref deletion are ignored")
+            return FalseWithReason("Ref deletion events are ignored")
 
         # comments are ORed
         matches_comment_re = False
@@ -288,7 +288,7 @@ class GithubEventFilter(EventFilter):
                 comment_re.search(event.comment)):
                 matches_comment_re = True
         if self.comments and not matches_comment_re:
-            return FalseWithReason("Comments %s doesn't match %s" % (
+            return FalseWithReason("Comments %s do not match %s" % (
                 self.comments, event.comment))
 
         # actions are ORed
@@ -297,7 +297,7 @@ class GithubEventFilter(EventFilter):
             if (event.action == action):
                 matches_action = True
         if self.actions and not matches_action:
-            return FalseWithReason("Actions %s doesn't match %s" % (
+            return FalseWithReason("Actions %s do not match %s" % (
                 self.actions, event.action))
 
         # check_runs are ORed
@@ -308,22 +308,22 @@ class GithubEventFilter(EventFilter):
                     check_run_found = True
                     break
             if not check_run_found:
-                return FalseWithReason("Check_runs %s doesn't match %s" % (
+                return FalseWithReason("Check runs %s do not match %s" % (
                     self.check_runs, event.check_run))
 
         # labels are ORed
         if self.labels and event.label not in self.labels:
-            return FalseWithReason("Labels %s doesn't match %s" % (
+            return FalseWithReason("Labels %s do not match %s" % (
                 self.labels, event.label))
 
         # unlabels are ORed
         if self.unlabels and event.unlabel not in self.unlabels:
-            return FalseWithReason("Unlabels %s doesn't match %s" % (
+            return FalseWithReason("Unlabels %s do not match %s" % (
                 self.unlabels, event.unlabel))
 
         # states are ORed
         if self.states and event.state not in self.states:
-            return FalseWithReason("States %s doesn't match %s" % (
+            return FalseWithReason("States %s do not match %s" % (
                 self.states, event.state))
 
         # statuses are ORed
@@ -334,7 +334,7 @@ class GithubEventFilter(EventFilter):
                     status_found = True
                     break
             if not status_found:
-                return FalseWithReason("Statuses %s doesn't match %s" % (
+                return FalseWithReason("Statuses %s do not match %s" % (
                     self.statuses, event.status))
 
         if self.require_filter:
@@ -498,7 +498,7 @@ class GithubRefFilter(RefFilter):
             if self.required_reviews and not change.reviews:
                 # No reviews means no matching of required bits
                 # having reject reviews but no reviews on the change is okay
-                return FalseWithReason("Reviews %s does not match %s" % (
+                return FalseWithReason("Reviews %s do not match %s" % (
                     self.required_reviews, change.reviews))
 
         return (self.matchesRequiredReviews(change) and
@@ -514,7 +514,7 @@ class GithubRefFilter(RefFilter):
                     break
             if not matches_review:
                 return FalseWithReason(
-                    "Required reviews %s does not match %s" % (
+                    "Required reviews %s do not match %s" % (
                         self.required_reviews, change.reviews))
         return True
 
@@ -523,7 +523,7 @@ class GithubRefFilter(RefFilter):
             for review in change.reviews:
                 if self._match_review_required_review(rreview, review):
                     # A review matched, we can reject right away
-                    return FalseWithReason("Reject reviews %s matches %s" % (
+                    return FalseWithReason("Reject reviews %s match %s" % (
                         self.reject_reviews, change.reviews))
         return True
 
@@ -531,10 +531,10 @@ class GithubRefFilter(RefFilter):
         if self.required_statuses or self.reject_statuses:
             if not hasattr(change, 'number'):
                 # not a PR, no status
-                return FalseWithReason("Can't match statuses without PR")
+                return FalseWithReason("Can not match statuses without PR")
             if self.required_statuses and not change.status:
                 return FalseWithReason(
-                    "Required statuses %s does not match %s" % (
+                    "Required statuses %s do not match %s" % (
                         self.required_statuses, change.status))
         required_statuses_results = self.matchesRequiredStatuses(change)
         if not required_statuses_results:
@@ -551,7 +551,7 @@ class GithubRefFilter(RefFilter):
                 for status in change.status:
                     if re2.fullmatch(required_status, status):
                         return True
-            return FalseWithReason("RequiredStatuses %s does not match %s" % (
+            return FalseWithReason("Required statuses %s do not match %s" % (
                 self.required_statuses, change.status))
         return True
 
@@ -561,7 +561,7 @@ class GithubRefFilter(RefFilter):
         for rstatus in self.reject_statuses:
             for status in change.status:
                 if re2.fullmatch(rstatus, status):
-                    return FalseWithReason("NoRejectStatuses %s matches %s" % (
+                    return FalseWithReason("Reject statuses %s match %s" % (
                         self.reject_statuses, change.status))
         return True
 
@@ -569,7 +569,7 @@ class GithubRefFilter(RefFilter):
         if self.required_labels or self.reject_labels:
             if not hasattr(change, 'number'):
                 # not a PR, no label
-                return FalseWithReason("Can't match labels without PR")
+                return FalseWithReason("Can not match labels without PR")
             if self.required_labels and not change.labels:
                 # No labels means no matching of required bits
                 # having reject labels but no labels on the change is okay
@@ -582,14 +582,14 @@ class GithubRefFilter(RefFilter):
     def matchesRequiredLabels(self, change):
         for label in self.required_labels:
             if label not in change.labels:
-                return FalseWithReason("Labels %s does not match %s" % (
+                return FalseWithReason("Labels %s do not match %s" % (
                     self.required_labels, change.labels))
         return True
 
     def matchesNoRejectLabels(self, change):
         for label in self.reject_labels:
             if label in change.labels:
-                return FalseWithReason("NoRejectLabels %s matches %s" % (
+                return FalseWithReason("Reject labels %s match %s" % (
                     self.reject_labels, change.labels))
         return True