Add require-approval to Gerrit trigger

This feature allows Zuul to consider existing (or new) approval
votes associated with a change when determining whether an event
matches.  For example, it can be used to require that a Verified
vote of a certain age be present before a change is enqueued in
a pipeline.

Change-Id: I81344713d71b345b08576334568b9c49c810c7e9

doc/source/zuul.rst

@@ -348,6 +348,35 @@ explanation of each of the parameters::
     containing 'retrigger' somewhere in the comment text are added to a
     change.
 
+    *require-approval*
+    This may be used for any event.  It requires that a certain kind
+    of approval be present for the current patchset of the change (the
+    approval could be added by the event in question).  It takes
+    several sub-parameters, all of which are optional and are combined
+    together so that there must be an approval matching all specified
+    requirements.
+
+      *username*
+      If present, an approval from this username is required.
+
+      *email-filter*
+      If present, an approval with this email address is required.  It
+      is treated as a regular expression as above.
+
+      *older-than*
+      If present, the approval must be older than this amount of time
+      to match.  Provide a time interval as a number with a suffix of
+      "w" (weeks), "d" (days), "h" (hours), "m" (minutes), "s"
+      (seconds).  Example "48h" or "2d".
+
+      *newer-than*
+      If present, the approval must be newer than this amount of time
+      to match.  Same format as "older-than".
+
+      Any other field is interpreted as a review category and value
+      pair.  For example "verified: 1" would require that the approval
+      be for a +1 vote in the "Verified" column.
+
   **timer**
     This trigger will run based on a cron-style time specification.
     It will enqueue an event into its pipeline for every project

tests/fixtures/layout-require-approval.yaml

@@ -0,0 +1,58 @@
+includes:
+  - python-file: custom_functions.py
+
+pipelines:
+  - name: check
+    manager: IndependentPipelineManager
+    trigger:
+      gerrit:
+        - event: patchset-created
+        - event: comment-added
+          require-approval:
+            - email-filter: jenkins@example.com
+              older-than: 48h
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+  - name: gate
+    manager: DependentPipelineManager
+    failure-message: Build failed.  For information on how to proceed, see http://wiki.example.org/Test_Failures
+    trigger:
+      gerrit:
+        - event: comment-added
+          require-approval:
+            - verified: 1
+              username: jenkins
+              newer-than: 48h
+          approval:
+            - approved: 1
+        - event: comment-added
+          require-approval:
+            - verified: 1
+              username: jenkins
+              newer-than: 48h
+          approval:
+            - verified: 1
+    success:
+      gerrit:
+        verified: 2
+        submit: true
+    failure:
+      gerrit:
+        verified: -2
+    start:
+      gerrit:
+        verified: 0
+    precedence: high
+
+projects:
+  - name: org/project
+    merge-mode: cherry-pick
+    check:
+      - project-check
+    gate:
+      - project-gate

tests/fixtures/layouts/good_require_approvals.yaml

@@ -0,0 +1,36 @@
+includes:
+  - python-file: custom_functions.py
+
+pipelines:
+  - name: check
+    manager: IndependentPipelineManager
+    trigger:
+      gerrit:
+        - event: comment-added
+          require-approval:
+            - username: jenkins
+              older-than: 48h
+        - event: comment-added
+          require-approval:
+            - email-filter: jenkins@example.com
+              newer-than: 48h
+        - event: comment-added
+          require-approval:
+            - approved: 1
+        - event: comment-added
+          require-approval:
+            - approved: 1
+              username: jenkins
+              email-filter: jenkins@example.com
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+projects:
+  - name: org/project
+    merge-mode: cherry-pick
+    check:
+      - project-check

tests/test_scheduler.py

@@ -214,10 +214,19 @@ class FakeChange(object):
                  "reason": ""}
         return event
 
-    def addApproval(self, category, value):
+    def addApproval(self, category, value, username='jenkins',
+                    granted_on=None):
+        if not granted_on:
+            granted_on = time.time()
         approval = {'description': self.categories[category][0],
                     'type': category,
-                    'value': str(value)}
+                    'value': str(value),
+                    'username': username,
+                    'email': username + '@example.com',
+                    'grantedOn': int(granted_on)}
+        for i, x in enumerate(self.patchsets[-1]['approvals'][:]):
+            if x['username'] == username and x['type'] == category:
+                del self.patchsets[-1]['approvals'][i]
         self.patchsets[-1]['approvals'].append(approval)
         event = {'approvals': [approval],
                  'author': {'email': 'user@example.com',
@@ -2658,6 +2667,95 @@ class TestScheduler(testtools.TestCase):
         self.assertEqual(D.data['status'], 'MERGED')
         self.assertEqual(D.reported, 2)
 
+    def test_required_approval_check_and_gate(self):
+        "Test required-approval triggers both check and gate"
+        self.config.set('zuul', 'layout_config',
+                        'tests/fixtures/layout-require-approval.yaml')
+        self.sched.reconfigure(self.config)
+        self.registerJobs()
+
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        A.addApproval('CRVW', 2)
+        # Add a too-old +1
+        A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60)
+
+        aprv = A.addApproval('APRV', 1)
+        self.fake_gerrit.addEvent(aprv)
+        self.waitUntilSettled()
+        # Should have run a check job
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project-check')
+
+        # Report the result of that check job (overrides previous vrfy)
+        # Skynet alert: this should trigger a gate job now that
+        # all reqs are met
+        self.fake_gerrit.addEvent(A.addApproval('VRFY', 1))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, 'project-gate')
+
+    def test_required_approval_newer(self):
+        "Test required-approval newer trigger parameter"
+        self.config.set('zuul', 'layout_config',
+                        'tests/fixtures/layout-require-approval.yaml')
+        self.sched.reconfigure(self.config)
+        self.registerJobs()
+
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        A.addApproval('CRVW', 2)
+        aprv = A.addApproval('APRV', 1)
+        self.fake_gerrit.addEvent(aprv)
+        self.waitUntilSettled()
+        # No +1 from Jenkins so should not be enqueued
+        self.assertEqual(len(self.history), 0)
+
+        # Add a too-old +1, should trigger check but not gate
+        A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60)
+        self.fake_gerrit.addEvent(aprv)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project-check')
+
+        # Add a recent +1
+        self.fake_gerrit.addEvent(A.addApproval('VRFY', 1))
+        self.fake_gerrit.addEvent(aprv)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, 'project-gate')
+
+    def test_required_approval_older(self):
+        "Test required-approval older trigger parameter"
+        self.config.set('zuul', 'layout_config',
+                        'tests/fixtures/layout-require-approval.yaml')
+        self.sched.reconfigure(self.config)
+        self.registerJobs()
+
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        crvw = A.addApproval('CRVW', 2)
+        self.fake_gerrit.addEvent(crvw)
+        self.waitUntilSettled()
+        # No +1 from Jenkins so should not be enqueued
+        self.assertEqual(len(self.history), 0)
+
+        # Add an old +1 and trigger check with a comment
+        A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60)
+        self.fake_gerrit.addEvent(crvw)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project-check')
+
+        # Add a recent +1 and make sure nothing changes
+        A.addApproval('VRFY', 1)
+        self.fake_gerrit.addEvent(crvw)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # The last thing we did was query a change then do nothing
+        # with a pipeline, so it will be in the cache; clean it up so
+        # it does not fail the test.
+        for pipeline in self.sched.layout.pipelines.values():
+            pipeline.trigger.maintainCache([])
+
     def test_rerun_on_error(self):
         "Test that if a worker fails to run a job, it is run again"
         self.worker.hold_jobs_in_build = True

zuul/layoutvalidator.py

@@ -35,6 +35,12 @@ class LayoutSchema(object):
 
     variable_dict = v.Schema({}, extra=True)
 
+    require_approval = v.Schema({'username': str,
+                                 'email-filter': str,
+                                 'older-than': str,
+                                 'newer-than': str,
+                                 }, extra=True)
+
     gerrit_trigger = {v.Required('event'):
                       toList(v.Any('patchset-created',
                                    'change-abandoned',
@@ -48,6 +54,7 @@ class LayoutSchema(object):
                       'branch': toList(str),
                       'ref': toList(str),
                       'approval': toList(variable_dict),
+                      'require-approval': toList(require_approval),
                       }
 
     timer_trigger = {v.Required('time'): str}

zuul/model.py

@@ -43,6 +43,20 @@ PRECEDENCE_MAP = {
 }
 
 
+def time_to_seconds(s):
+    if s.endswith('s'):
+        return int(s[:-1])
+    if s.endswith('m'):
+        return int(s[:-1]) * 60
+    if s.endswith('h'):
+        return int(s[:-1]) * 60 * 60
+    if s.endswith('d'):
+        return int(s[:-1]) * 24 * 60 * 60
+    if s.endswith('w'):
+        return int(s[:-1]) * 7 * 24 * 60 * 60
+    raise Exception("Unable to parse time value: %s" % s)
+
+
 class Pipeline(object):
     """A top-level pipeline such as check, gate, post, etc."""
     def __init__(self, name):
@@ -735,6 +749,7 @@ class Change(Changeish):
         self.can_merge = False
         self.is_merged = False
         self.failed_to_merge = False
+        self.approvals = []
 
     def _id(self):
         return '%s,%s' % (self.number, self.patchset)
@@ -873,9 +888,9 @@ class TriggerEvent(object):
 
 
 class EventFilter(object):
-    def __init__(self, types=[], branches=[], refs=[], approvals={},
+    def __init__(self, types=[], branches=[], refs=[], event_approvals={},
                  comment_filters=[], email_filters=[], username_filters=[],
-                 timespecs=[]):
+                 timespecs=[], require_approvals=[]):
         self._types = types
         self._branches = branches
         self._refs = refs
@@ -888,9 +903,18 @@ class EventFilter(object):
         self.comment_filters = [re.compile(x) for x in comment_filters]
         self.email_filters = [re.compile(x) for x in email_filters]
         self.username_filters = [re.compile(x) for x in username_filters]
-        self.approvals = approvals
+        self.event_approvals = event_approvals
+        self.require_approvals = require_approvals
         self.timespecs = timespecs
 
+        for a in self.require_approvals:
+            if 'older-than' in a:
+                a['older-than'] = time_to_seconds(a['older-than'])
+            if 'newer-than' in a:
+                a['newer-than'] = time_to_seconds(a['newer-than'])
+            if 'email-filter' in a:
+                a['email-filter'] = re.compile(a['email-filter'])
+
     def __repr__(self):
         ret = '<EventFilter'
 
@@ -900,9 +924,9 @@ class EventFilter(object):
             ret += ' branches: %s' % ', '.join(self._branches)
         if self._refs:
             ret += ' refs: %s' % ', '.join(self._refs)
-        if self.approvals:
-            ret += ' approvals: %s' % ', '.join(
-                ['%s:%s' % a for a in self.approvals.items()])
+        if self.event_approvals:
+            ret += ' event_approvals: %s' % ', '.join(
+                ['%s:%s' % a for a in self.event_approvals.items()])
         if self._comment_filters:
             ret += ' comment_filters: %s' % ', '.join(self._comment_filters)
         if self._email_filters:
@@ -915,7 +939,7 @@ class EventFilter(object):
 
         return ret
 
-    def matches(self, event):
+    def matches(self, event, change):
         def normalizeCategory(name):
             name = name.lower()
             return re.sub(' ', '-', name)
@@ -977,7 +1001,7 @@ class EventFilter(object):
                 return False
 
         # approvals are ANDed
-        for category, value in self.approvals.items():
+        for category, value in self.event_approvals.items():
             matches_approval = False
             for eapproval in event.approvals:
                 if (normalizeCategory(eapproval['description']) == category and
@@ -986,6 +1010,40 @@ class EventFilter(object):
             if not matches_approval:
                 return False
 
+        if self.require_approvals and not change.approvals:
+            # A change with no approvals can not match
+            return False
+
+        now = time.time()
+        for rapproval in self.require_approvals:
+            matches_approval = False
+            for approval in change.approvals:
+                found_approval = True
+                for k, v in rapproval.items():
+                    if k == 'username':
+                        if (approval['username'] != v):
+                            found_approval = False
+                    elif k == 'email-filter':
+                        if (not v.search(approval['email'])):
+                            found_approval = False
+                    elif k == 'newer-than':
+                        t = now - v
+                        if (approval['grantedOn'] < t):
+                            found_approval = False
+                    elif k == 'older-than':
+                        t = now - v
+                        if (approval['grantedOn'] >= t):
+                            found_approval = False
+                    else:
+                        if (normalizeCategory(approval['description']) != k or
+                            int(approval['value']) != v):
+                            found_approval = False
+                if found_approval:
+                    matches_approval = True
+                    break
+            if not matches_approval:
+                return False
+
         # timespecs are ORed
         matches_timespec = False
         for timespec in self.timespecs:

zuul/scheduler.py

@@ -227,13 +227,15 @@ class Scheduler(threading.Thread):
                     f = EventFilter(types=toList(trigger['event']),
                                     branches=toList(trigger.get('branch')),
                                     refs=toList(trigger.get('ref')),
-                                    approvals=approvals,
+                                    event_approvals=approvals,
                                     comment_filters=
                                     toList(trigger.get('comment_filter')),
                                     email_filters=
                                     toList(trigger.get('email_filter')),
                                     username_filters=
-                                    toList(trigger.get('username_filter')))
+                                    toList(trigger.get('username_filter')),
+                                    require_approvals=
+                                    toList(trigger.get('require-approval')))
                     manager.event_filters.append(f)
             elif 'timer' in conf_pipeline['trigger']:
                 pipeline.trigger = self.triggers['timer']
@@ -723,7 +725,7 @@ class Scheduler(threading.Thread):
                                      self.triggers.get(event.trigger_name))
             if event.type == 'patchset-created':
                 pipeline.manager.removeOldVersionsOfChange(change)
-            if pipeline.manager.eventMatches(event):
+            if pipeline.manager.eventMatches(event, change):
                 self.log.info("Adding %s, %s to %s" %
                               (project, change, pipeline))
                 pipeline.manager.addChange(change)
@@ -881,14 +883,14 @@ class BasePipelineManager(object):
             allow_needs.update(action_reporter.getSubmitAllowNeeds())
         return allow_needs
 
-    def eventMatches(self, event):
+    def eventMatches(self, event, change):
         if event.forced_pipeline:
             if event.forced_pipeline == self.pipeline.name:
                 return True
             else:
                 return False
         for ef in self.event_filters:
-            if ef.matches(event):
+            if ef.matches(event, change):
                 return True
         return False
 

zuul/trigger/gerrit.py

@@ -361,6 +361,8 @@ class Gerrit(object):
                 if not dep.is_merged and dep.is_current_patchset:
                     change.needed_by_changes.append(dep)
 
+        change.approvals = data['currentPatchSet'].get('approvals', [])
+
         return change
 
     def getGitUrl(self, project):