Merge "Add Gerrit pipeline trigger requirements"

9 files changed, 761 insertions, 197 deletions

diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst
index 4b9c93044..3c16f202a 100644
--- a/doc/source/drivers/gerrit.rst
+++ b/doc/source/drivers/gerrit.rst
@@ -239,6 +239,10 @@ be able to invoke the ``gerrit stream-events`` command over SSH.
 
    .. attr:: require-approval
 
+      .. warning:: This is deprecated and will be removed in a future
+                   version.  Use :attr:`pipeline.trigger.<gerrit
+                   source>.require` instead.
+
       This may be used for any event.  It requires that a certain kind
       of approval be present for the current patchset of the change
       (the approval could be added by the event in question).  It
@@ -246,13 +250,40 @@ be able to invoke the ``gerrit stream-events`` command over SSH.
       source>.approval`. For each specified criteria there must exist
       a matching approval.
 
+      This is ignored if the :attr:`pipeline.trigger.<gerrit
+      source>.require` attribute is present.
+
    .. attr:: reject-approval
 
+      .. warning:: This is deprecated and will be removed in a future
+                   version.  Use :attr:`pipeline.trigger.<gerrit
+                   source>.reject` instead.
+
       This takes a list of approvals in the same format as
       :attr:`pipeline.trigger.<gerrit source>.require-approval` but
       the item will fail to enter the pipeline if there is a matching
       approval.
 
+      This is ignored if the :attr:`pipeline.trigger.<gerrit
+      source>.reject` attribute is present.
+
+   .. attr:: require
+
+      This may be used for any event.  It describes conditions that
+      must be met by the change in order for the trigger event to
+      match.  Those conditions may be satisfied by the event in
+      question.  It follows the same syntax as
+      :ref:`gerrit_requirements`.
+
+   .. attr:: reject
+
+      This may be used for any event and is the mirror of
+      :attr:`pipeline.trigger.<gerrit source>.require`.  It describes
+      conditions that when met by the change cause the trigger event
+      not to match.  Those conditions may be satisfied by the event in
+      question.  It follows the same syntax as
+      :ref:`gerrit_requirements`.
+
 Reporter Configuration
 ----------------------
 
@@ -284,6 +315,8 @@ with an HTTP password, in which case the HTTP API is used.
 A :ref:`connection<connections>` that uses the gerrit driver must be
 supplied to the trigger.
 
+.. _gerrit_requirements:
+
 Requirements Configuration
 --------------------------
 
@@ -366,7 +399,7 @@ order to be enqueued into the pipeline.
    .. attr:: status
 
       A string value that corresponds with the status of the change
-      reported by the trigger.
+      reported by Gerrit.
 
 .. attr:: pipeline.reject.<gerrit source>
 
@@ -376,10 +409,12 @@ order to be enqueued into the pipeline.
 
    .. attr:: approval
 
-      This takes an approval or a list of approvals. If an approval 
-      matches the provided criteria the change can not be entered 
-      into the pipeline. It follows the same syntax as
-      :attr:`pipeline.require.<gerrit source>.approval`.
+      This requires that a certain kind of approval not be present for the
+      current patchset of the change (the approval could be added by
+      the event in question). Approval is a dictionary or a list of
+      dictionaries with attributes listed below, all of which are
+      optional and are combined together so that there must be no approvals
+      matching all specified requirements.
 
       Example to reject a change with any negative vote:
 
@@ -390,6 +425,55 @@ order to be enqueued into the pipeline.
              approval:
                - Code-Review: [-1, -2]
 
+      .. attr:: username
+
+         If present, an approval from this username is required.  It is
+         treated as a regular expression.
+
+      .. attr:: email
+
+         If present, an approval with this email address is required.  It is
+         treated as a regular expression.
+
+      .. attr:: older-than
+
+         If present, the approval must be older than this amount of time
+         to match.  Provide a time interval as a number with a suffix of
+         "w" (weeks), "d" (days), "h" (hours), "m" (minutes), "s"
+         (seconds).  Example ``48h`` or ``2d``.
+
+      .. attr:: newer-than
+
+         If present, the approval must be newer than this amount
+         of time to match.  Same format as "older-than".
+
+      Any other field is interpreted as a review category and value
+      pair.  For example ``Verified: 1`` would require that the
+      approval be for a +1 vote in the "Verified" column.  The value
+      may either be a single value or a list: ``Verified: [1, 2]``
+      would match either a +1 or +2 vote.
+
+   .. attr:: open
+
+      A boolean value (``true`` or ``false``) that indicates whether
+      the change must be open or closed in order to be rejected.
+
+   .. attr:: current-patchset
+
+      A boolean value (``true`` or ``false``) that indicates whether the
+      change must be the current patchset in order to be rejected.
+
+   .. attr:: wip
+
+      A boolean value (``true`` or ``false``) that indicates whether the
+      change must be wip or not wip in order to be rejected.
+
+   .. attr:: status
+
+      A string value that corresponds with the status of the change
+      reported by Gerrit.
+
+
 Reference Pipelines Configuration
 ---------------------------------
 
diff --git a/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml b/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml
new file mode 100644
index 000000000..479d6c153
--- /dev/null
+++ b/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml
@@ -0,0 +1,22 @@
+---
+features:
+  - |
+    Gerrit pipeline triggers now support embedded require and reject
+    filters in order to match.  Any conditions set for the pipeline in
+    require or reject filters may also be set for event trigger
+    filters.
+
+    This can be used to construct pipelines which trigger based on
+    certain events but only if certain other conditions are met.  It
+    is distinct from pipeline requirements in that it only affects
+    items that are directly enqueued whereas pipeline requirements
+    affect dependencies as well.
+  - |
+    All Gerrit "requires" filters are now available as "reject"
+    filters as well.
+deprecations:
+  - |
+    The `require-approval` and `reject-approval` Gerrit trigger
+    attributes are deprecated.  Use :attr:`pipeline.trigger.<gerrit
+    source>.require` and :attr:`pipeline.trigger.<gerrit
+    source>.reject` instead.
diff --git a/tests/base.py b/tests/base.py
index c031497a2..af10ebe96 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -950,6 +950,7 @@ class FakeGerritChange(object):
         if self.fail_merge:
             return
         self.data['status'] = 'MERGED'
+        self.data['open'] = False
         self.open = False
 
         path = os.path.join(self.upstream_root, self.project)
diff --git a/tests/fixtures/layouts/gerrit-trigger-requirements.yaml b/tests/fixtures/layouts/gerrit-trigger-requirements.yaml
new file mode 100644
index 000000000..72ad6b41e
--- /dev/null
+++ b/tests/fixtures/layouts/gerrit-trigger-requirements.yaml
@@ -0,0 +1,162 @@
+- pipeline:
+    name: require-open
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test require-open
+          require:
+            open: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: reject-open
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test reject-open
+          reject:
+            open: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: require-wip
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test require-wip
+          require:
+            wip: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: reject-wip
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test reject-wip
+          reject:
+            wip: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: require-current-patchset
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test require-current-patchset
+          require:
+            current-patchset: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: reject-current-patchset
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test reject-current-patchset
+          reject:
+            current-patchset: true
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: require-status
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test require-status
+          require:
+            status: MERGED
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: reject-status
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test reject-status
+          reject:
+            status: MERGED
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: require-approval
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test require-approval
+          require:
+            approval:
+              username: zuul
+              Verified: 1
+    success:
+      gerrit:
+        Verified: 1
+
+- pipeline:
+    name: reject-approval
+    manager: independent
+    trigger:
+      gerrit:
+        - event: comment-added
+          comment: test reject-approval
+          reject:
+            approval:
+              username: zuul
+              Verified: 1
+    success:
+      gerrit:
+        Verified: 1
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+
+- job: {name: require-open}
+- job: {name: reject-open}
+- job: {name: require-wip}
+- job: {name: reject-wip}
+- job: {name: require-current-patchset}
+- job: {name: reject-current-patchset}
+- job: {name: require-status}
+- job: {name: reject-status}
+- job: {name: require-approval}
+- job: {name: reject-approval}
+
+- project:
+    name: org/project
+    require-open: {jobs: [require-open]}
+    reject-open: {jobs: [reject-open]}
+    require-wip: {jobs: [require-wip]}
+    reject-wip: {jobs: [reject-wip]}
+    require-current-patchset: {jobs: [require-current-patchset]}
+    reject-current-patchset: {jobs: [reject-current-patchset]}
+    require-status: {jobs: [require-status]}
+    reject-status: {jobs: [reject-status]}
+    require-approval: {jobs: [require-approval]}
+    reject-approval: {jobs: [reject-approval]}
diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py
index 9f3b87187..c5dca56cd 100644
--- a/tests/unit/test_requirements.py
+++ b/tests/unit/test_requirements.py
@@ -14,7 +14,7 @@
 
 import time
 
-from tests.base import ZuulTestCase
+from tests.base import ZuulTestCase, simple_layout
 
 
 class TestRequirementsApprovalNewerThan(ZuulTestCase):
@@ -490,3 +490,222 @@ class TestRequirementsTrustedCheck(ZuulTestCase):
         self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
         self.waitUntilSettled()
         self.assertHistory([])
+
+
+class TestGerritTriggerRequirements(ZuulTestCase):
+    scheduler_count = 1
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_require_open(self):
+        # Test trigger require-open
+        jobname = 'require-open'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's open, so it should be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # Not open, so should be ignored
+        A.setMerged()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_reject_open(self):
+        # Test trigger reject-open
+        jobname = 'reject-open'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's open, so it should not be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Not open, so should be enqueued
+        A.setMerged()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_require_wip(self):
+        # Test trigger require-wip
+        jobname = 'require-wip'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's not WIP, so it should be ignored
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # WIP, so should be enqueued
+        A.setWorkInProgress(True)
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_reject_wip(self):
+        # Test trigger reject-wip
+        jobname = 'reject-wip'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's not WIP, so it should be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # WIP, so should be ignored
+        A.setWorkInProgress(True)
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_require_current_patchset(self):
+        # Test trigger require-current_patchset
+        jobname = 'require-current-patchset'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's current, so it should be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # Not current, so should be ignored
+        A.addPatchset()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_reject_current_patchset(self):
+        # Test trigger reject-current_patchset
+        jobname = 'reject-current-patchset'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's current, so it should be ignored
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Not current, so should be enqueued
+        A.addPatchset()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_require_status(self):
+        # Test trigger require-status
+        jobname = 'require-status'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's not merged, so it should be ignored
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Merged, so should be enqueued
+        A.setMerged()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_reject_status(self):
+        # Test trigger reject-status
+        jobname = 'reject-status'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # It's not merged, so it should be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # Merged, so should be ignored
+        A.setMerged()
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_require_approval(self):
+        # Test trigger require-approval
+        jobname = 'require-approval'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # Missing approval, so it should be ignored
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Has approval, so it should be enqueued
+        A.addApproval('Verified', 1, username='zuul')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+    def test_reject_approval(self):
+        # Test trigger reject-approval
+        jobname = 'reject-approval'
+        project = 'org/project'
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+        # Missing approval, so it should be enqueued
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # Has approval, so it should be ignored
+        A.addApproval('Verified', 1, username='zuul')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
diff --git a/zuul/driver/gerrit/gerritmodel.py b/zuul/driver/gerrit/gerritmodel.py
index 4ac291f2b..7b57ec934 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
@@ -247,110 +247,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
@@ -388,18 +310,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
@@ -414,7 +334,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:
@@ -428,7 +349,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
@@ -437,11 +359,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
@@ -455,7 +378,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.
@@ -468,7 +392,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')
@@ -478,7 +403,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():
@@ -488,29 +414,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'
@@ -518,10 +488,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))
@@ -533,37 +507,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 b0bd3c448..4e7a32b83 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
 
@@ -209,21 +209,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):
@@ -247,11 +241,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