From 1a4ec7e9266989207f879786a1c19b6d18180eb2 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Tue, 21 Feb 2023 14:54:10 -0800
Subject: [PATCH] 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
---
 doc/source/drivers/gerrit.rst                 |   3 +-
 doc/source/drivers/github.rst                 |  36 +-
 ...ithub-trigger-status-948e81b9f45418f1.yaml |  17 +
 .../layouts/github-trigger-requirements.yaml  | 112 ++++
 tests/unit/test_github_requirements.py        | 178 ++++++
 zuul/driver/github/githubmodel.py             | 539 ++++++++++--------
 zuul/driver/github/githubsource.py            |  28 +-
 zuul/driver/github/githubtrigger.py           |   7 +-
 8 files changed, 658 insertions(+), 262 deletions(-)
 create mode 100644 releasenotes/notes/github-trigger-status-948e81b9f45418f1.yaml
 create mode 100644 tests/fixtures/layouts/github-trigger-requirements.yaml

diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst
index 4e7fc7cea4..4b9c930442 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 148c6f9769..7cacf45ac2 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 0000000000..19cac86423
--- /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 0000000000..5014df3bbf
--- /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 ef1f759441..f3021d41de 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 30610cf4e0..536cce9031 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,15 +170,277 @@ 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)
+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)
+
+        # TODO: Backwards compat, remove after 9.x:
+        if required_statuses and require is None:
+            require = {'status': required_statuses}
+
+        if require:
+            self.require_filter = GithubRefFilter.requiresFromConfig(
+                connection_name, require)
+        else:
+            self.require_filter = None
+
+        if reject:
+            self.reject_filter = GithubRefFilter.rejectFromConfig(
+                connection_name, reject)
+        else:
+            self.reject_filter = None
+
+        self._types = types
+        self._branches = branches
+        self._refs = refs
+        self._comments = comments
+        self.types = [re.compile(x) for x in types]
+        self.branches = [re.compile(x) for x in branches]
+        self.refs = [re.compile(x) for x in refs]
+        self.comments = [re.compile(x) for x in comments]
+        self.actions = actions
+        self.labels = labels
+        self.unlabels = unlabels
+        self.states = states
+        self.statuses = statuses
+        self.check_runs = check_runs
+        self.ignore_deletes = ignore_deletes
+
+    def __repr__(self):
+        ret = '<GithubEventFilter'
+        ret += ' connection: %s' % self.connection_name
+        if self._types:
+            ret += ' types: %s' % ', '.join(self._types)
+        if self._branches:
+            ret += ' branches: %s' % ', '.join(self._branches)
+        if self._refs:
+            ret += ' refs: %s' % ', '.join(self._refs)
+        if self.ignore_deletes:
+            ret += ' ignore_deletes: %s' % self.ignore_deletes
+        if self._comments:
+            ret += ' comments: %s' % ', '.join(self._comments)
+        if self.actions:
+            ret += ' actions: %s' % ', '.join(self.actions)
+        if self.check_runs:
+            ret += ' check_runs: %s' % ','.join(self.check_runs)
+        if self.labels:
+            ret += ' labels: %s' % ', '.join(self.labels)
+        if self.unlabels:
+            ret += ' unlabels: %s' % ', '.join(self.unlabels)
+        if self.states:
+            ret += ' states: %s' % ', '.join(self.states)
+        if self.statuses:
+            ret += ' statuses: %s' % ', '.join(self.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
+
+    def matches(self, event, change):
+        if not super().matches(event, change):
+            return False
+
+        # event types are ORed
+        matches_type = False
+        for etype in self.types:
+            if etype.match(event.type):
+                matches_type = True
+        if self.types and not matches_type:
+            return FalseWithReason("Types %s doesn't match %s" % (
+                self.types, event.type))
+
+        # branches are ORed
+        matches_branch = False
+        for branch in self.branches:
+            if branch.match(event.branch):
+                matches_branch = True
+        if self.branches and not matches_branch:
+            return FalseWithReason("Branches %s doesn't match %s" % (
+                self.branches, event.branch))
+
+        # refs are ORed
+        matches_ref = False
+        if event.ref is not None:
+            for ref in self.refs:
+                if ref.match(event.ref):
+                    matches_ref = True
+        if self.refs and not matches_ref:
+            return FalseWithReason(
+                "Refs %s doesn't 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")
+
+        # comments are ORed
+        matches_comment_re = False
+        for comment_re in self.comments:
+            if (event.comment is not None and
+                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" % (
+                self.comments, event.comment))
+
+        # actions are ORed
+        matches_action = False
+        for action in self.actions:
+            if (event.action == action):
+                matches_action = True
+        if self.actions and not matches_action:
+            return FalseWithReason("Actions %s doesn't match %s" % (
+                self.actions, event.action))
+
+        # check_runs are ORed
+        if self.check_runs:
+            check_run_found = False
+            for check_run in self.check_runs:
+                if re2.fullmatch(check_run, event.check_run):
+                    check_run_found = True
+                    break
+            if not check_run_found:
+                return FalseWithReason("Check_runs %s doesn't 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" % (
+                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" % (
+                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" % (
+                self.states, event.state))
+
+        # statuses are ORed
+        if self.statuses:
+            status_found = False
+            for status in self.statuses:
+                if re2.fullmatch(status, event.status):
+                    status_found = True
+                    break
+            if not status_found:
+                return FalseWithReason("Statuses %s doesn't match %s" % (
+                    self.statuses, event.status))
+
+        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):
+    def __init__(self, connection_name, statuses=[],
+                 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)
+
+        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 = required_statuses
+        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:
+            self.open = open
+        if reject_merged is not None:
+            self.merged = not reject_merged
+        else:
+            self.merged = merged
+        if reject_current_patchset is not None:
+            self.current_patchset = not reject_current_patchset
+        else:
+            self.current_patchset = current_patchset
+        if reject_draft is not None:
+            self.draft = not reject_draft
+        else:
+            self.draft = draft
+
+    @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.required_statuses:
+            ret += ' status: %s' % str(self.required_statuses)
+        if self.reject_statuses:
+            ret += ' reject-status: %s' % str(self.reject_statuses)
+        if self.required_reviews:
+            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:
+            ret += ' merged: %s' % self.merged
+        if self.current_patchset is not None:
+            ret += ' current-patchset: %s' % self.current_patchset
+        if self.draft is not None:
+            ret += ' draft: %s' % self.draft
+
+        ret += '>'
+
+        return ret
 
     def _tidy_reviews(self, reviews):
         for r in reviews:
@@ -303,229 +565,47 @@ class GithubCommonFilter(object):
                         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))
 
-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):
+    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
 
-        EventFilter.__init__(self, connection_name, trigger)
-
-        GithubCommonFilter.__init__(self, required_statuses=required_statuses)
-
-        self._types = types
-        self._branches = branches
-        self._refs = refs
-        self._comments = comments
-        self.types = [re.compile(x) for x in types]
-        self.branches = [re.compile(x) for x in branches]
-        self.refs = [re.compile(x) for x in refs]
-        self.comments = [re.compile(x) for x in comments]
-        self.actions = actions
-        self.labels = labels
-        self.unlabels = unlabels
-        self.states = states
-        self.statuses = statuses
-        self.required_statuses = required_statuses
-        self.check_runs = check_runs
-        self.ignore_deletes = ignore_deletes
-
-    def __repr__(self):
-        ret = '<GithubEventFilter'
-        ret += ' connection: %s' % self.connection_name
-        if self._types:
-            ret += ' types: %s' % ', '.join(self._types)
-        if self._branches:
-            ret += ' branches: %s' % ', '.join(self._branches)
-        if self._refs:
-            ret += ' refs: %s' % ', '.join(self._refs)
-        if self.ignore_deletes:
-            ret += ' ignore_deletes: %s' % self.ignore_deletes
-        if self._comments:
-            ret += ' comments: %s' % ', '.join(self._comments)
-        if self.actions:
-            ret += ' actions: %s' % ', '.join(self.actions)
-        if self.check_runs:
-            ret += ' check_runs: %s' % ','.join(self.check_runs)
-        if self.labels:
-            ret += ' labels: %s' % ', '.join(self.labels)
-        if self.unlabels:
-            ret += ' unlabels: %s' % ', '.join(self.unlabels)
-        if self.states:
-            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)
-        ret += '>'
-
-        return ret
-
-    def matches(self, event, change):
-        if not super().matches(event, change):
-            return False
-
-        # event types are ORed
-        matches_type = False
-        for etype in self.types:
-            if etype.match(event.type):
-                matches_type = True
-        if self.types and not matches_type:
-            return FalseWithReason("Types %s doesn't match %s" % (
-                self.types, event.type))
-
-        # branches are ORed
-        matches_branch = False
-        for branch in self.branches:
-            if branch.match(event.branch):
-                matches_branch = True
-        if self.branches and not matches_branch:
-            return FalseWithReason("Branches %s doesn't match %s" % (
-                self.branches, event.branch))
-
-        # refs are ORed
-        matches_ref = False
-        if event.ref is not None:
-            for ref in self.refs:
-                if ref.match(event.ref):
-                    matches_ref = True
-        if self.refs and not matches_ref:
-            return FalseWithReason(
-                "Refs %s doesn't 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")
-
-        # comments are ORed
-        matches_comment_re = False
-        for comment_re in self.comments:
-            if (event.comment is not None and
-                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" % (
-                self.comments, event.comment))
-
-        # actions are ORed
-        matches_action = False
-        for action in self.actions:
-            if (event.action == action):
-                matches_action = True
-        if self.actions and not matches_action:
-            return FalseWithReason("Actions %s doesn't match %s" % (
-                self.actions, event.action))
-
-        # check_runs are ORed
-        if self.check_runs:
-            check_run_found = False
-            for check_run in self.check_runs:
-                if re2.fullmatch(check_run, event.check_run):
-                    check_run_found = True
-                    break
-            if not check_run_found:
-                return FalseWithReason("Check_runs %s doesn't 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" % (
-                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" % (
-                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" % (
-                self.states, event.state))
-
-        # statuses are ORed
-        if self.statuses:
-            status_found = False
-            for status in self.statuses:
-                if re2.fullmatch(status, event.status):
-                    status_found = True
-                    break
-            if not status_found:
-                return FalseWithReason("Statuses %s doesn't match %s" % (
-                    self.statuses, event.status))
-
-        return self.matchesStatuses(change)
-
-
-class GithubRefFilter(RefFilter, GithubCommonFilter):
-    def __init__(self, connection_name, statuses=[],
-                 required_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
-        if reject_open is not None:
-            self.open = not reject_open
-        else:
-            self.open = open
-        if reject_merged is not None:
-            self.merged = not reject_merged
-        else:
-            self.merged = merged
-        if reject_current_patchset is not None:
-            self.current_patchset = not reject_current_patchset
-        else:
-            self.current_patchset = current_patchset
-        if reject_draft is not None:
-            self.draft = not reject_draft
-        else:
-            self.draft = draft
-        self.labels = labels
-        self.reject_labels = reject_labels
-
-    def __repr__(self):
-        ret = '<GithubRefFilter'
-
-        ret += ' connection_name: %s' % self.connection_name
-        if self.statuses:
-            ret += ' statuses: %s' % ', '.join(self.statuses)
-        if self.reject_statuses:
-            ret += ' reject-statuses: %s' % ', '.join(self.reject_statuses)
-        if self.required_reviews:
-            ret += (' required-reviews: %s' %
-                    str(self.required_reviews))
-        if self.reject_reviews:
-            ret += (' reject-reviews: %s' %
-                    str(self.reject_reviews))
-        if self.open is not None:
-            ret += ' open: %s' % self.open
-        if self.merged is not None:
-            ret += ' merged: %s' % self.merged
-        if self.current_patchset is not None:
-            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 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 0a94a17303..01901dfd4a 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 76d8f574e7..5072fda430 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),
     }