Merge "Add GitHub pipeline trigger requirements"

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),
     }