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

1 files changed, 241 insertions, 178 deletions

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