Add GitHub pipeline trigger requirements

This mimics a useful feature of the Gerrit driver and allows users
to configure pipelines that trigger on events but only if certain
conditions of the PR are met.

Unlike the Gerrit driver, this embeds the entire require/reject
filter within the trigger filter (the trigger filter has-a require
or reject filter).  This makes the code simpler and is easier for
users to configure.  If we like this approach, we should migrate the
gerrit driver as well, and perhaps the other drivers.

The "require-status" attribute already existed, but was undocumented.
This documents it, adds backwards-compat handling for it, and
deprecates it.

Some documentation typos are also corrected.

Change-Id: I4b6dd8c970691b1e74ffd5a96c2be4b8075f1a87

8 files changed, 598 insertions, 202 deletions

diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst
index 4e7fc7cea..4b9c93044 100644
--- a/doc/source/drivers/gerrit.rst
+++ b/doc/source/drivers/gerrit.rst
@@ -250,7 +250,8 @@ be able to invoke the ``gerrit stream-events`` command over SSH.
 
       This takes a list of approvals in the same format as
       :attr:`pipeline.trigger.<gerrit source>.require-approval` but
-      will fail to enter the pipeline if there is a matching approval.
+      the item will fail to enter the pipeline if there is a matching
+      approval.
 
 Reporter Configuration
 ----------------------
diff --git a/doc/source/drivers/github.rst b/doc/source/drivers/github.rst
index 148c6f976..7cacf45ac 100644
--- a/doc/source/drivers/github.rst
+++ b/doc/source/drivers/github.rst
@@ -339,7 +339,7 @@ the following options.
       format of ``user:context:status``.  For example,
       ``zuul_github_ci_bot:check_pipeline:success``.
 
-   .. attr: check
+   .. attr:: check
 
       This is only used for ``check_run`` events. It works similar to
       the ``status`` attribute and accepts a list of strings each of
@@ -363,6 +363,38 @@ the following options.
       always sends full ref name, eg. ``refs/tags/bar`` and this
       string is matched against the regular expression.
 
+   .. attr:: require-status
+
+      .. warning:: This is deprecated and will be removed in a future
+                   version.  Use :attr:`pipeline.trigger.<github
+                   source>.require` instead.
+
+      This may be used for any event.  It requires that a certain kind
+      of status be present for the PR (the status could be added by
+      the event in question).  It follows the same syntax as
+      :attr:`pipeline.require.<github source>.status`. For each
+      specified criteria there must exist a matching status.
+
+      This is ignored if the :attr:`pipeline.trigger.<github
+      source>.require` attribute is present.
+
+   .. attr:: require
+
+      This may be used for any event.  It describes conditions that
+      must be met by the PR 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:`github_requirements`.
+
+   .. attr:: reject
+
+      This may be used for any event and is the mirror of
+      :attr:`pipeline.trigger.<github source>.require`.  It describes
+      conditions that when met by the PR cause the trigger event not
+      to match.  Those conditions may be satisfied by the event in
+      question.  It follows the same syntax as
+      :ref:`github_requirements`.
+
+
 Reporter Configuration
 ----------------------
 Zuul reports back to GitHub via GitHub API. Available reports include a PR
@@ -462,6 +494,8 @@ itself. Status name, description, and context is taken from the pipeline.
 
 .. _Github App: https://developer.github.com/apps/
 
+.. _github_requirements:
+
 Requirements Configuration
 --------------------------
 
diff --git a/releasenotes/notes/github-trigger-status-948e81b9f45418f1.yaml b/releasenotes/notes/github-trigger-status-948e81b9f45418f1.yaml
new file mode 100644
index 000000000..19cac8642
--- /dev/null
+++ b/releasenotes/notes/github-trigger-status-948e81b9f45418f1.yaml
@@ -0,0 +1,17 @@
+---
+features:
+  - |
+    GitHub 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.
+deprecations:
+  - |
+    The `require-status` GitHub trigger attribute is deprecated.
+    Use :attr:`pipeline.trigger.<github source>.require` instead.
diff --git a/tests/fixtures/layouts/github-trigger-requirements.yaml b/tests/fixtures/layouts/github-trigger-requirements.yaml
new file mode 100644
index 000000000..5014df3bb
--- /dev/null
+++ b/tests/fixtures/layouts/github-trigger-requirements.yaml
@@ -0,0 +1,112 @@
+- pipeline:
+    name: require-status
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test require-status
+          require:
+            status:
+              - zuul:tenant-one/check:success
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: reject-status
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test reject-status
+          reject:
+            status:
+              - zuul:tenant-one/check:failure
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: require-review
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test require-review
+          require:
+            review:
+              - type: approved
+                permission: write
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: reject-review
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test reject-review
+          reject:
+            review:
+              - type: changes_requested
+                permission: write
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: require-label
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test require-label
+          require:
+            label:
+              - approved
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: reject-label
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: test reject-label
+          reject:
+            label:
+              - rejected
+    success:
+      github:
+        comment: true
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+
+- job: {name: require-status}
+- job: {name: reject-status}
+- job: {name: require-review}
+- job: {name: reject-review}
+- job: {name: require-label}
+- job: {name: reject-label}
+
+- project:
+    name: org/project
+    require-status: {jobs: [require-status]}
+    reject-status: {jobs: [reject-status]}
+    require-review: {jobs: [require-review]}
+    reject-review: {jobs: [reject-review]}
+    require-label: {jobs: [require-label]}
+    reject-label: {jobs: [reject-label]}
diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py
index ef1f75944..f3021d41d 100644
--- a/tests/unit/test_github_requirements.py
+++ b/tests/unit/test_github_requirements.py
@@ -678,3 +678,181 @@ class TestGithubAppRequirements(ZuulGithubAppTestCase):
         self.fake_github.emitEvent(comment)
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 1)
+
+
+class TestGithubTriggerRequirements(ZuulTestCase):
+    """Test pipeline and trigger requirements"""
+    config_file = 'zuul-github-driver.conf'
+    scheduler_count = 1
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_require_status(self):
+        # Test trigger require-status
+        jobname = 'require-status'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No status from zuul so should not be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # An error status should not cause it to be enqueued
+        self.fake_github.setCommitStatus(project, A.head_sha, 'error',
+                                         context='tenant-one/check')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A success status goes in
+        self.fake_github.setCommitStatus(project, A.head_sha, 'success',
+                                         context='tenant-one/check')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_reject_status(self):
+        # Test trigger reject-status
+        jobname = 'reject-status'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No status from zuul so should be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # A failure status should not cause it to be enqueued
+        self.fake_github.setCommitStatus(project, A.head_sha, 'failure',
+                                         context='tenant-one/check')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # A success status goes in
+        self.fake_github.setCommitStatus(project, A.head_sha, 'success',
+                                         context='tenant-one/check')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, jobname)
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_require_review(self):
+        # Test trigger require-review
+        jobname = 'require-review'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        A.writers.extend(('maintainer',))
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No review so should not be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # An changes requested review should not cause it to be enqueued
+        A.addReview('maintainer', 'CHANGES_REQUESTED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive review goes in
+        A.addReview('maintainer', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_reject_review(self):
+        # Test trigger reject-review
+        jobname = 'reject-review'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        A.writers.extend(('maintainer',))
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No review so should be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # An changes requested review should not cause it to be enqueued
+        A.addReview('maintainer', 'CHANGES_REQUESTED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # A positive review goes in
+        A.addReview('maintainer', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, jobname)
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_require_label(self):
+        # Test trigger require-label
+        jobname = 'require-label'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No label so should not be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A random should not cause it to be enqueued
+        A.addLabel('foobar')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # An approved label goes in
+        A.addLabel('approved')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+    @simple_layout('layouts/github-trigger-requirements.yaml', driver='github')
+    def test_reject_label(self):
+        # Test trigger reject-label
+        jobname = 'reject-label'
+        project = 'org/project'
+        A = self.fake_github.openFakePullRequest(project, 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent(f'test {jobname}')
+
+        # No label so should be enqueued
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, jobname)
+
+        # A rejected label should not cause it to be enqueued
+        A.addLabel('rejected')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # Any other label, it goes in
+        A.removeLabel('rejected')
+        A.addLabel('okay')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, jobname)
diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py
index 30610cf4e..536cce903 100644
--- a/zuul/driver/github/githubmodel.py
+++ b/zuul/driver/github/githubmodel.py
@@ -21,7 +21,7 @@ import time
 
 from zuul.model import Change, TriggerEvent, EventFilter, RefFilter
 from zuul.model import FalseWithReason
-from zuul.driver.util import time_to_seconds
+from zuul.driver.util import time_to_seconds, to_list
 
 
 EMPTY_GIT_REF = '0' * 40  # git sha of all zeros, used during creates/deletes
@@ -170,149 +170,31 @@ class GithubTriggerEvent(TriggerEvent):
         return ' '.join(r)
 
 
-class GithubCommonFilter(object):
-    def __init__(self, required_reviews=[], required_statuses=[],
-                 reject_reviews=[], reject_statuses=[]):
-        self._required_reviews = copy.deepcopy(required_reviews)
-        self._reject_reviews = copy.deepcopy(reject_reviews)
-        self.required_reviews = self._tidy_reviews(self._required_reviews)
-        self.reject_reviews = self._tidy_reviews(self._reject_reviews)
-        self.required_statuses = required_statuses
-        self.reject_statuses = reject_statuses
-
-    def _tidy_reviews(self, reviews):
-        for r in reviews:
-            for k, v in r.items():
-                if k == 'username':
-                    r['username'] = re.compile(v)
-                elif k == 'email':
-                    r['email'] = re.compile(v)
-                elif k == 'newer-than':
-                    r[k] = time_to_seconds(v)
-                elif k == 'older-than':
-                    r[k] = time_to_seconds(v)
-        return reviews
-
-    def _match_review_required_review(self, rreview, review):
-        # Check if the required review and review match
-        now = time.time()
-        by = review.get('by', {})
-        for k, v in rreview.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 (review['grantedOn'] < t):
-                    return False
-            elif k == 'older-than':
-                t = now - v
-                if (review['grantedOn'] >= t):
-                    return False
-            elif k == 'type':
-                if review['type'] != v:
-                    return False
-            elif k == 'permission':
-                # If permission is read, we've matched. You must have read
-                # to provide a review.
-                if v != 'read':
-                    # Admins have implicit write.
-                    if v == 'write':
-                        if review['permission'] not in ('write', 'admin'):
-                            return False
-                    elif v == 'admin':
-                        if review['permission'] != 'admin':
-                            return False
-        return True
-
-    def matchesReviews(self, change):
-        if self.required_reviews or self.reject_reviews:
-            if not hasattr(change, 'number'):
-                # not a PR, no reviews
-                return FalseWithReason("Change is not a PR")
-            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" % (
-                    self.required_reviews, change.reviews))
-
-        return (self.matchesRequiredReviews(change) and
-                self.matchesNoRejectReviews(change))
-
-    def matchesRequiredReviews(self, change):
-        for rreview in self.required_reviews:
-            matches_review = False
-            for review in change.reviews:
-                if self._match_review_required_review(rreview, review):
-                    # Consider matched if any review matches
-                    matches_review = True
-                    break
-            if not matches_review:
-                return FalseWithReason(
-                    "Required reviews %s does not match %s" % (
-                        self.required_reviews, change.reviews))
-        return True
-
-    def matchesNoRejectReviews(self, change):
-        for rreview in self.reject_reviews:
-            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" % (
-                        self.reject_reviews, change.reviews))
-        return True
-
-    def matchesStatuses(self, change):
-        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")
-            if self.required_statuses and not change.status:
-                return FalseWithReason(
-                    "Required statuses %s does not match %s" % (
-                        self.required_statuses, change.status))
-        required_statuses_results = self.matchesRequiredStatuses(change)
-        if not required_statuses_results:
-            return required_statuses_results
-        return self.matchesNoRejectStatuses(change)
-
-    def matchesRequiredStatuses(self, change):
-        # statuses are ORed
-        # A PR head can have multiple statuses on it. If the change
-        # statuses and the filter statuses are a null intersection, there
-        # are no matches and we return false
-        if self.required_statuses:
-            for required_status in self.required_statuses:
-                for status in change.status:
-                    if re2.fullmatch(required_status, status):
-                        return True
-            return FalseWithReason("RequiredStatuses %s does not match %s" % (
-                self.required_statuses, change.status))
-        return True
-
-    def matchesNoRejectStatuses(self, change):
-        # statuses are ANDed
-        # If any of the rejected statusses are present, we return false
-        for rstatus in self.reject_statuses:
-            for status in change.status:
-                if re2.fullmatch(rstatus, status):
-                    return FalseWithReason("NoRejectStatuses %s matches %s" % (
-                        self.reject_statuses, change.status))
-        return True
+class GithubEventFilter(EventFilter):
+    def __init__(self, connection_name, trigger, types=[],
+                 branches=[], refs=[], comments=[], actions=[],
+                 labels=[], unlabels=[], states=[], statuses=[],
+                 required_statuses=[], check_runs=[],
+                 ignore_deletes=True,
+                 require=None, reject=None):
 
+        EventFilter.__init__(self, connection_name, trigger)
 
-class GithubEventFilter(EventFilter, GithubCommonFilter):
-    def __init__(self, connection_name, trigger, types=[], branches=[],
-                 refs=[], comments=[], actions=[], labels=[], unlabels=[],
-                 states=[], statuses=[], required_statuses=[],
-                 check_runs=[], ignore_deletes=True):
+        # TODO: Backwards compat, remove after 9.x:
+        if required_statuses and require is None:
+            require = {'status': required_statuses}
 
-        EventFilter.__init__(self, connection_name, trigger)
+        if require:
+            self.require_filter = GithubRefFilter.requiresFromConfig(
+                connection_name, require)
+        else:
+            self.require_filter = None
 
-        GithubCommonFilter.__init__(self, required_statuses=required_statuses)
+        if reject:
+            self.reject_filter = GithubRefFilter.rejectFromConfig(
+                connection_name, reject)
+        else:
+            self.reject_filter = None
 
         self._types = types
         self._branches = branches
@@ -327,7 +209,6 @@ class GithubEventFilter(EventFilter, GithubCommonFilter):
         self.unlabels = unlabels
         self.states = states
         self.statuses = statuses
-        self.required_statuses = required_statuses
         self.check_runs = check_runs
         self.ignore_deletes = ignore_deletes
 
@@ -356,8 +237,10 @@ class GithubEventFilter(EventFilter, GithubCommonFilter):
             ret += ' states: %s' % ', '.join(self.states)
         if self.statuses:
             ret += ' statuses: %s' % ', '.join(self.statuses)
-        if self.required_statuses:
-            ret += ' required_statuses: %s' % ', '.join(self.required_statuses)
+        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
@@ -454,23 +337,37 @@ class GithubEventFilter(EventFilter, GithubCommonFilter):
                 return FalseWithReason("Statuses %s doesn't match %s" % (
                     self.statuses, event.status))
 
-        return self.matchesStatuses(change)
+        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 GithubRefFilter(RefFilter, GithubCommonFilter):
+class GithubRefFilter(RefFilter):
     def __init__(self, connection_name, statuses=[],
-                 required_reviews=[], reject_reviews=[], open=None,
+                 reviews=[], reject_reviews=[], open=None,
                  merged=None, current_patchset=None, draft=None,
                  reject_open=None, reject_merged=None,
                  reject_current_patchset=None, reject_draft=None,
                  labels=[], reject_labels=[], reject_statuses=[]):
         RefFilter.__init__(self, connection_name)
 
-        GithubCommonFilter.__init__(self, required_reviews=required_reviews,
-                                    reject_reviews=reject_reviews,
-                                    required_statuses=statuses,
-                                    reject_statuses=reject_statuses)
-        self.statuses = statuses
+        self._required_reviews = copy.deepcopy(reviews)
+        self._reject_reviews = copy.deepcopy(reject_reviews)
+        self.required_reviews = self._tidy_reviews(self._required_reviews)
+        self.reject_reviews = self._tidy_reviews(self._reject_reviews)
+        self.required_statuses = statuses
+        self.reject_statuses = reject_statuses
+        self.required_labels = labels
+        self.reject_labels = reject_labels
+
         if reject_open is not None:
             self.open = not reject_open
         else:
@@ -487,23 +384,51 @@ class GithubRefFilter(RefFilter, GithubCommonFilter):
             self.draft = not reject_draft
         else:
             self.draft = draft
-        self.labels = labels
-        self.reject_labels = reject_labels
+
+    @classmethod
+    def requiresFromConfig(cls, connection_name, config):
+        return cls(
+            connection_name=connection_name,
+            statuses=to_list(config.get('status')),
+            reviews=to_list(config.get('review')),
+            labels=to_list(config.get('label')),
+            open=config.get('open'),
+            merged=config.get('merged'),
+            current_patchset=config.get('current-patchset'),
+            draft=config.get('draft'),
+        )
+
+    @classmethod
+    def rejectFromConfig(cls, connection_name, config):
+        return cls(
+            connection_name=connection_name,
+            reject_statuses=to_list(config.get('status')),
+            reject_reviews=to_list(config.get('review')),
+            reject_labels=to_list(config.get('label')),
+            reject_open=config.get('open'),
+            reject_merged=config.get('merged'),
+            reject_current_patchset=config.get('current-patchset'),
+            reject_draft=config.get('draft'),
+        )
 
     def __repr__(self):
         ret = '<GithubRefFilter'
 
         ret += ' connection_name: %s' % self.connection_name
-        if self.statuses:
-            ret += ' statuses: %s' % ', '.join(self.statuses)
+        if self.required_statuses:
+            ret += ' status: %s' % str(self.required_statuses)
         if self.reject_statuses:
-            ret += ' reject-statuses: %s' % ', '.join(self.reject_statuses)
+            ret += ' reject-status: %s' % str(self.reject_statuses)
         if self.required_reviews:
-            ret += (' required-reviews: %s' %
+            ret += (' reviews: %s' %
                     str(self.required_reviews))
         if self.reject_reviews:
             ret += (' reject-reviews: %s' %
                     str(self.reject_reviews))
+        if self.required_labels:
+            ret += ' labels: %s' % str(self.required_labels)
+        if self.reject_labels:
+            ret += ' reject-labels: %s' % str(self.reject_labels)
         if self.open is not None:
             ret += ' open: %s' % self.open
         if self.merged is not None:
@@ -512,20 +437,175 @@ class GithubRefFilter(RefFilter, GithubCommonFilter):
             ret += ' current-patchset: %s' % self.current_patchset
         if self.draft is not None:
             ret += ' draft: %s' % self.draft
-        if self.labels:
-            ret += ' labels: %s' % self.labels
-        if self.reject_labels:
-            ret += ' reject-labels: %s' % self.reject_labels
 
         ret += '>'
 
         return ret
 
+    def _tidy_reviews(self, reviews):
+        for r in reviews:
+            for k, v in r.items():
+                if k == 'username':
+                    r['username'] = re.compile(v)
+                elif k == 'email':
+                    r['email'] = re.compile(v)
+                elif k == 'newer-than':
+                    r[k] = time_to_seconds(v)
+                elif k == 'older-than':
+                    r[k] = time_to_seconds(v)
+        return reviews
+
+    def _match_review_required_review(self, rreview, review):
+        # Check if the required review and review match
+        now = time.time()
+        by = review.get('by', {})
+        for k, v in rreview.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 (review['grantedOn'] < t):
+                    return False
+            elif k == 'older-than':
+                t = now - v
+                if (review['grantedOn'] >= t):
+                    return False
+            elif k == 'type':
+                if review['type'] != v:
+                    return False
+            elif k == 'permission':
+                # If permission is read, we've matched. You must have read
+                # to provide a review.
+                if v != 'read':
+                    # Admins have implicit write.
+                    if v == 'write':
+                        if review['permission'] not in ('write', 'admin'):
+                            return False
+                    elif v == 'admin':
+                        if review['permission'] != 'admin':
+                            return False
+        return True
+
+    def matchesReviews(self, change):
+        if self.required_reviews or self.reject_reviews:
+            if not hasattr(change, 'number'):
+                # not a PR, no reviews
+                return FalseWithReason("Change is not a PR")
+            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" % (
+                    self.required_reviews, change.reviews))
+
+        return (self.matchesRequiredReviews(change) and
+                self.matchesNoRejectReviews(change))
+
+    def matchesRequiredReviews(self, change):
+        for rreview in self.required_reviews:
+            matches_review = False
+            for review in change.reviews:
+                if self._match_review_required_review(rreview, review):
+                    # Consider matched if any review matches
+                    matches_review = True
+                    break
+            if not matches_review:
+                return FalseWithReason(
+                    "Required reviews %s does not match %s" % (
+                        self.required_reviews, change.reviews))
+        return True
+
+    def matchesNoRejectReviews(self, change):
+        for rreview in self.reject_reviews:
+            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" % (
+                        self.reject_reviews, change.reviews))
+        return True
+
+    def matchesStatuses(self, change):
+        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")
+            if self.required_statuses and not change.status:
+                return FalseWithReason(
+                    "Required statuses %s does not match %s" % (
+                        self.required_statuses, change.status))
+        required_statuses_results = self.matchesRequiredStatuses(change)
+        if not required_statuses_results:
+            return required_statuses_results
+        return self.matchesNoRejectStatuses(change)
+
+    def matchesRequiredStatuses(self, change):
+        # statuses are ORed
+        # A PR head can have multiple statuses on it. If the change
+        # statuses and the filter statuses are a null intersection, there
+        # are no matches and we return false
+        if self.required_statuses:
+            for required_status in self.required_statuses:
+                for status in change.status:
+                    if re2.fullmatch(required_status, status):
+                        return True
+            return FalseWithReason("RequiredStatuses %s does not match %s" % (
+                self.required_statuses, change.status))
+        return True
+
+    def matchesNoRejectStatuses(self, change):
+        # statuses are ANDed
+        # If any of the rejected statusses are present, we return false
+        for rstatus in self.reject_statuses:
+            for status in change.status:
+                if re2.fullmatch(rstatus, status):
+                    return FalseWithReason("NoRejectStatuses %s matches %s" % (
+                        self.reject_statuses, change.status))
+        return True
+
+    def matchesLabels(self, change):
+        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")
+            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
+                return FalseWithReason(
+                    "Required labels %s does not match %s" % (
+                        self.required_labels, change.labels))
+        return (self.matchesRequiredLabels(change) and
+                self.matchesNoRejectLabels(change))
+
+    def matchesRequiredLabels(self, change):
+        for label in self.required_labels:
+            if label not in change.labels:
+                return FalseWithReason("Labels %s does 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" % (
+                    self.reject_labels, change.labels))
+        return True
+
     def matches(self, change):
         statuses_result = self.matchesStatuses(change)
         if not statuses_result:
             return statuses_result
 
+        reviews_result = self.matchesReviews(change)
+        if not reviews_result:
+            return reviews_result
+
+        labels_result = self.matchesLabels(change)
+        if not labels_result:
+            return labels_result
+
         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.
@@ -566,21 +646,4 @@ class GithubRefFilter(RefFilter, GithubCommonFilter):
             else:
                 return FalseWithReason("Change is not a PR")
 
-        # required reviews are ANDed (reject reviews are ORed)
-        reviews_result = self.matchesReviews(change)
-        if not reviews_result:
-            return reviews_result
-
-        # required labels are ANDed
-        for label in self.labels:
-            if label not in change.labels:
-                return FalseWithReason("Labels %s does not match %s" % (
-                    self.labels, change.labels))
-
-        # rejected reviews are OR'd
-        for label in self.reject_labels:
-            if label in change.labels:
-                return FalseWithReason("RejectLabels %s matches %s" % (
-                    self.reject_labels, change.labels))
-
         return True
diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py
index 0a94a1730..01901dfd4 100644
--- a/zuul/driver/github/githubsource.py
+++ b/zuul/driver/github/githubsource.py
@@ -21,7 +21,7 @@ import voluptuous as v
 from zuul.source import BaseSource
 from zuul.model import Project
 from zuul.driver.github.githubmodel import GithubRefFilter
-from zuul.driver.util import scalar_or_list, to_list
+from zuul.driver.util import scalar_or_list
 from zuul.zk.change_cache import ChangeKey
 
 
@@ -165,29 +165,15 @@ class GithubSource(BaseSource):
         return time.strptime(timestamp, '%Y-%m-%dT%H:%M:%SZ')
 
     def getRequireFilters(self, config):
-        f = GithubRefFilter(
-            connection_name=self.connection.connection_name,
-            statuses=to_list(config.get('status')),
-            required_reviews=to_list(config.get('review')),
-            open=config.get('open'),
-            merged=config.get('merged'),
-            current_patchset=config.get('current-patchset'),
-            draft=config.get('draft'),
-            labels=to_list(config.get('label')),
-        )
+        f = GithubRefFilter.requiresFromConfig(
+            self.connection.connection_name,
+            config)
         return [f]
 
     def getRejectFilters(self, config):
-        f = GithubRefFilter(
-            connection_name=self.connection.connection_name,
-            reject_reviews=to_list(config.get('review')),
-            reject_labels=to_list(config.get('label')),
-            reject_statuses=to_list(config.get('status')),
-            reject_open=config.get('open'),
-            reject_merged=config.get('merged'),
-            reject_current_patchset=config.get('current-patchset'),
-            reject_draft=config.get('draft'),
-        )
+        f = GithubRefFilter.rejectFromConfig(
+            self.connection.connection_name,
+            config)
         return [f]
 
     def getRefForChange(self, change):
diff --git a/zuul/driver/github/githubtrigger.py b/zuul/driver/github/githubtrigger.py
index 76d8f574e..5072fda43 100644
--- a/zuul/driver/github/githubtrigger.py
+++ b/zuul/driver/github/githubtrigger.py
@@ -16,6 +16,7 @@ import logging
 import voluptuous as v
 from zuul.trigger import BaseTrigger
 from zuul.driver.github.githubmodel import GithubEventFilter
+from zuul.driver.github import githubsource
 from zuul.driver.util import scalar_or_list, to_list
 
 
@@ -50,7 +51,9 @@ class GithubTrigger(BaseTrigger):
                 unlabels=to_list(trigger.get('unlabel')),
                 states=to_list(trigger.get('state')),
                 statuses=to_list(trigger.get('status')),
-                required_statuses=to_list(trigger.get('require-status'))
+                required_statuses=to_list(trigger.get('require-status')),
+                require=trigger.get('require'),
+                reject=trigger.get('reject'),
             )
             efilters.append(f)
 
@@ -75,6 +78,8 @@ def getSchema():
         'unlabel': scalar_or_list(str),
         'state': scalar_or_list(str),
         'require-status': scalar_or_list(str),
+        'require': githubsource.getRequireSchema(),
+        'reject': githubsource.getRejectSchema(),
         'status': scalar_or_list(str),
         'check': scalar_or_list(str),
     }