Add support to reject changes from approvals

Formally "support for negative requirements"

This change adds support to reject a change from entering a pipeline
based off any existing approvals. This is useful to stop queuing any
changes with a negative vote.

Two new options are added:

reject:
  approval:
    - ...

To the pipelines, and:

reject-approval:
  - ...

To the triggers.

The approval format is the same for the "require approval" section.
Reject approvals could be considered a negative of the require section.

Change-Id: I3369920530e0b7439208b8fd43a9e75994860666

doc/source/zuul.rst

@@ -477,7 +477,13 @@ explanation of each of the parameters::
     of approval be present for the current patchset of the change (the
     approval could be added by the event in question).  It follows the
     same syntax as the :ref:`"approval" pipeline requirement below
-    <pipeline-require-approval>`.
+    <pipeline-require-approval>`. For each specified criteria there must
+    exist a matching approval.
+
+    *reject-approval*
+    This takes a list of approvals in the same format as
+    *require-approval* but will fail to enter the pipeline if there is
+    a matching approval.
 
   **timer**
     This trigger will run based on a cron-style time specification.
@@ -513,7 +519,13 @@ explanation of each of the parameters::
     of approval be present for the current patchset of the change (the
     approval could be added by the event in question).  It follows the
     same syntax as the :ref:`"approval" pipeline requirement below
-    <pipeline-require-approval>`.
+    <pipeline-require-approval>`. For each specified criteria there must
+    exist a matching approval.
+
+    *reject-approval*
+    This takes a list of approvals in the same format as
+    *require-approval* but will fail to enter the pipeline if there is
+    a matching approval.
 
 
 **require**
@@ -572,6 +584,23 @@ explanation of each of the parameters::
   reported by the trigger.  For example, when using the Gerrit
   trigger, status values such as ``NEW`` or ``MERGED`` may be useful.
 
+**reject**
+  If this section is present, it establishes pre-requisites that can
+  block an item from being enqueued. It can be considered a negative
+  version of **require**.
+
+  **approval**
+  This takes a list of approvals. If an approval matches the provided
+  criteria the change can not be entered into the pipeline. It follows
+  the same syntax as the :ref:`"require approval" pipeline above
+  <pipeline-require-approval>`.
+
+  Example to reject a change with any negative vote::
+
+    reject:
+      approval:
+        - code-review: [-1, -2]
+
 **dequeue-on-new-patchset**
   Normally, if a new patchset is uploaded to a change that is in a
   pipeline, the existing entry in the pipeline will be removed (with

tests/fixtures/layout-requirement-reject-username.yaml

@@ -0,0 +1,37 @@
+pipelines:
+  - name: pipeline
+    manager: IndependentPipelineManager
+    reject:
+      approval:
+        - username: 'jenkins'
+    trigger:
+      gerrit:
+        - event: comment-added
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+  - name: trigger
+    manager: IndependentPipelineManager
+    trigger:
+      gerrit:
+        - event: comment-added
+          reject-approval:
+            - username: 'jenkins'
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+projects:
+  - name: org/project1
+    pipeline:
+      - project1-pipeline
+  - name: org/project2
+    trigger:
+      - project2-trigger

tests/fixtures/layout-requirement-reject.yaml

@@ -0,0 +1,44 @@
+pipelines:
+  - name: pipeline
+    manager: IndependentPipelineManager
+    require:
+      approval:
+        - username: jenkins
+          verified: [1, 2]
+    reject:
+      approval:
+        - verified: [-1, -2]
+    trigger:
+      gerrit:
+        - event: comment-added
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+  - name: trigger
+    manager: IndependentPipelineManager
+    trigger:
+      gerrit:
+        - event: comment-added
+          require-approval:
+            - username: jenkins
+              verified: [1, 2]
+          reject-approval:
+            - verified: [-1, -2]
+    success:
+      gerrit:
+        verified: 1
+    failure:
+      gerrit:
+        verified: -1
+
+projects:
+  - name: org/project1
+    pipeline:
+      - project1-pipeline
+  - name: org/project2
+    trigger:
+      - project2-trigger

tests/fixtures/layouts/bad_reject.yaml

@@ -0,0 +1,21 @@
+# Template is going to be called but missing a parameter
+
+pipelines:
+  - name: 'check'
+    manager: IndependentPipelineManager
+    require:
+      open: True
+      current-patchset: True
+      approval:
+        - verified: [1, 2]
+          username: jenkins
+        - workflow: 1
+    reject:
+      # Reject only takes 'approval', has no need for open etc..
+      open: True
+      approval:
+        - code-review: [-1, -2]
+          username: core-person
+    trigger:
+      gerrit:
+        - event: patchset-created

tests/fixtures/layouts/good_layout.yaml

@@ -4,9 +4,18 @@ includes:
 pipelines:
   - name: check
     manager: IndependentPipelineManager
+    require:
+      open: True
+      current-patchset: True
     trigger:
       gerrit:
         - event: patchset-created
+        - event: comment-added
+          require-approval:
+            - verified: [-1, -2]
+              username: jenkins
+          approval:
+            - workflow: 1
     success:
       gerrit:
         verified: 1
@@ -26,6 +35,16 @@ pipelines:
     manager: DependentPipelineManager
     success-message: Your change is awesome.
     failure-message: Build failed.  For information on how to proceed, see http://wiki.example.org/Test_Failures
+    require:
+      open: True
+      current-patchset: True
+      approval:
+        - verified: [1, 2]
+          username: jenkins
+        - workflow: 1
+    reject:
+      approval:
+        - code-review: [-1, -2]
     trigger:
       gerrit:
         - event: comment-added

tests/test_requirements.py

@@ -323,3 +323,105 @@ class TestRequirements(ZuulTestCase):
         self.fake_gerrit.addEvent(B.addApproval('CRVW', 2))
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 1)
+
+    def _test_require_reject_username(self, project, job):
+        "Test negative username's match"
+        # Should only trigger if Jenkins hasn't voted.
+        self.config.set(
+            'zuul', 'layout_config',
+            'tests/fixtures/layout-requirement-reject-username.yaml')
+        self.sched.reconfigure(self.config)
+        self.registerJobs()
+
+        # add in a change with no comments
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # add in a comment that will trigger
+        self.fake_gerrit.addEvent(A.addApproval('CRVW', 1,
+                                                username='reviewer'))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, job)
+
+        # add in a comment from jenkins user which shouldn't trigger
+        self.fake_gerrit.addEvent(A.addApproval('VRFY', 1, username='jenkins'))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # Check future reviews also won't trigger as a 'jenkins' user has
+        # commented previously
+        self.fake_gerrit.addEvent(A.addApproval('CRVW', 1,
+                                                username='reviewer'))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+    def test_pipeline_reject_username(self):
+        "Test negative pipeline requirement: no comment from jenkins"
+        return self._test_require_reject_username('org/project1',
+                                                  'project1-pipeline')
+
+    def test_trigger_reject_username(self):
+        "Test negative trigger requirement: no comment from jenkins"
+        return self._test_require_reject_username('org/project2',
+                                                  'project2-trigger')
+
+    def _test_require_reject(self, project, job):
+        "Test no approval matches a reject param"
+        self.config.set(
+            'zuul', 'layout_config',
+            'tests/fixtures/layout-requirement-reject.yaml')
+        self.sched.reconfigure(self.config)
+        self.registerJobs()
+
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # First positive vote should not queue until jenkins has +1'd
+        comment = A.addApproval('VRFY', 1, username='reviewer_a')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Jenkins should put in a +1 which will also queue
+        comment = A.addApproval('VRFY', 1, username='jenkins')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, job)
+
+        # Negative vote should not queue
+        comment = A.addApproval('VRFY', -1, username='reviewer_b')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # Future approvals should do nothing
+        comment = A.addApproval('VRFY', 1, username='reviewer_c')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+
+        # Change/update negative vote should queue
+        comment = A.addApproval('VRFY', 1, username='reviewer_b')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 2)
+        self.assertEqual(self.history[1].name, job)
+
+        # Future approvals should also queue
+        comment = A.addApproval('VRFY', 1, username='reviewer_d')
+        self.fake_gerrit.addEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 3)
+        self.assertEqual(self.history[2].name, job)
+
+    def test_pipeline_require_reject(self):
+        "Test pipeline requirement: rejections absent"
+        return self._test_require_reject('org/project1', 'project1-pipeline')
+
+    def test_trigger_require_reject(self):
+        "Test trigger requirement: rejections absent"
+        return self._test_require_reject('org/project2', 'project2-trigger')

zuul/layoutvalidator.py

@@ -38,12 +38,12 @@ class LayoutSchema(object):
 
     variable_dict = v.Schema({}, extra=True)
 
-    require_approval = v.Schema({'username': str,
-                                 'email-filter': str,
-                                 'email': str,
-                                 'older-than': str,
-                                 'newer-than': str,
-                                 }, extra=True)
+    approval = v.Schema({'username': str,
+                         'email-filter': str,
+                         'email': str,
+                         'older-than': str,
+                         'newer-than': str,
+                         }, extra=True)
 
     gerrit_trigger = {v.Required('event'):
                       toList(v.Any('patchset-created',
@@ -63,7 +63,8 @@ class LayoutSchema(object):
                       'ref': toList(str),
                       'ignore-deletes': bool,
                       'approval': toList(variable_dict),
-                      'require-approval': toList(require_approval),
+                      'require-approval': toList(approval),
+                      'reject-approval': toList(approval),
                       }
 
     timer_trigger = {v.Required('time'): str}
@@ -72,7 +73,8 @@ class LayoutSchema(object):
                     toList(v.Any('parent-change-enqueued',
                                  'project-change-merged')),
                     'pipeline': toList(str),
-                    'require-approval': toList(require_approval),
+                    'require-approval': toList(approval),
+                    'reject-approval': toList(approval),
                     }
 
     trigger = v.Required({'gerrit': toList(gerrit_trigger),
@@ -86,11 +88,13 @@ class LayoutSchema(object):
                                },
                       }
 
-    require = {'approval': toList(require_approval),
+    require = {'approval': toList(approval),
                'open': bool,
                'current-patchset': bool,
                'status': toList(str)}
 
+    reject = {'approval': toList(approval)}
+
     window = v.All(int, v.Range(min=0))
     window_floor = v.All(int, v.Range(min=1))
     window_type = v.Any('linear', 'exponential')
@@ -102,6 +106,7 @@ class LayoutSchema(object):
                 'precedence': precedence,
                 'description': str,
                 'require': require,
+                'reject': reject,
                 'success-message': str,
                 'failure-message': str,
                 'merge-failure-message': str,

zuul/model.py

@@ -1042,11 +1042,14 @@ class TriggerEvent(object):
 
 
 class BaseFilter(object):
-    def __init__(self, required_approvals=[]):
+    def __init__(self, required_approvals=[], reject_approvals=[]):
         self._required_approvals = copy.deepcopy(required_approvals)
-        self.required_approvals = required_approvals
+        self.required_approvals = self._tidy_approvals(required_approvals)
+        self._reject_approvals = copy.deepcopy(reject_approvals)
+        self.reject_approvals = self._tidy_approvals(reject_approvals)
 
-        for a in self.required_approvals:
+    def _tidy_approvals(self, approvals):
+        for a in approvals:
             for k, v in a.items():
                 if k == 'username':
                     pass
@@ -1056,55 +1059,87 @@ class BaseFilter(object):
                     a[k] = time_to_seconds(v)
                 elif k == 'older-than':
                     a[k] = time_to_seconds(v)
-                else:
-                    if not isinstance(v, list):
-                        a[k] = [v]
             if 'email-filter' in a:
                 del a['email-filter']
+        return approvals
+
+    def _match_approval_required_approval(self, rapproval, approval):
+        # Check if the required approval and approval match
+        if 'description' not in approval:
+            return False
+        now = time.time()
+        by = approval.get('by', {})
+        for k, v in rapproval.items():
+            if k == 'username':
+                if (by.get('username', '') != v):
+                        return False
+            elif k == 'email':
+                if (not v.search(by.get('email', ''))):
+                        return False
+            elif k == 'newer-than':
+                t = now - v
+                if (approval['grantedOn'] < t):
+                        return False
+            elif k == 'older-than':
+                t = now - v
+                if (approval['grantedOn'] >= t):
+                    return False
+            else:
+                if not isinstance(v, list):
+                    v = [v]
+                if (normalizeCategory(approval['description']) != k or
+                        int(approval['value']) not in v):
+                    return False
+        return True
+
+    def matchesApprovals(self, change):
+        if (self.required_approvals and not change.approvals
+                or self.reject_approvals and not change.approvals):
+            # A change with no approvals can not match
+            return False
+
+        # TODO(jhesketh): If we wanted to optimise this slightly we could
+        # analyse both the REQUIRE and REJECT filters by looping over the
+        # approvals on the change and keeping track of what we have checked
+        # rather than needing to loop on the change approvals twice
+        return (self.matchesRequiredApprovals(change) and
+                self.matchesNoRejectApprovals(change))
 
     def matchesRequiredApprovals(self, change):
-        now = time.time()
+        # Check if any approvals match the requirements
         for rapproval in self.required_approvals:
-            matches_approval = False
+            matches_rapproval = False
             for approval in change.approvals:
-                if 'description' not in approval:
-                    continue
-                found_approval = True
-                by = approval.get('by', {})
-                for k, v in rapproval.items():
-                    if k == 'username':
-                        if (by.get('username', '') != v):
-                            found_approval = False
-                    elif k == 'email':
-                        if (not v.search(by.get('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']) not in v):
-                            found_approval = False
-                if found_approval:
-                    matches_approval = True
+                if self._match_approval_required_approval(rapproval, approval):
+                    # We have a matching approval so this requirement is
+                    # fulfilled
+                    matches_rapproval = True
                     break
-            if not matches_approval:
+            if not matches_rapproval:
                 return False
         return True
 
+    def matchesNoRejectApprovals(self, change):
+        # Check to make sure no approvals match a reject criteria
+        for rapproval in self.reject_approvals:
+            for approval in change.approvals:
+                if self._match_approval_required_approval(rapproval, approval):
+                    # A reject approval has been matched, so we reject
+                    # immediately
+                    return False
+        # To get here no rejects can have been matched so we should be good to
+        # queue
+        return True
+
 
 class EventFilter(BaseFilter):
     def __init__(self, trigger, types=[], branches=[], refs=[],
                  event_approvals={}, comments=[], emails=[], usernames=[],
-                 timespecs=[], required_approvals=[], pipelines=[],
-                 ignore_deletes=True):
+                 timespecs=[], required_approvals=[], reject_approvals=[],
+                 pipelines=[], ignore_deletes=True):
         super(EventFilter, self).__init__(
-            required_approvals=required_approvals)
+            required_approvals=required_approvals,
+            reject_approvals=reject_approvals)
         self.trigger = trigger
         self._types = types
         self._branches = branches
@@ -1143,6 +1178,9 @@ class EventFilter(BaseFilter):
         if self.required_approvals:
             ret += ' required_approvals: %s' % ', '.join(
                 ['%s' % a for a in self._required_approvals])
+        if self.reject_approvals:
+            ret += ' reject_approvals: %s' % ', '.join(
+                ['%s' % a for a in self._reject_approvals])
         if self._comments:
             ret += ' comments: %s' % ', '.join(self._comments)
         if self._emails:
@@ -1235,12 +1273,8 @@ class EventFilter(BaseFilter):
             if not matches_approval:
                 return False
 
-        if self.required_approvals and not change.approvals:
-            # A change with no approvals can not match
-            return False
-
-        # required approvals are ANDed
-        if not self.matchesRequiredApprovals(change):
+        # required approvals are ANDed (reject approvals are ORed)
+        if not self.matchesApprovals(change):
             return False
 
         # timespecs are ORed
@@ -1256,9 +1290,11 @@ class EventFilter(BaseFilter):
 
 class ChangeishFilter(BaseFilter):
     def __init__(self, open=None, current_patchset=None,
-                 statuses=[], required_approvals=[]):
+                 statuses=[], required_approvals=[],
+                 reject_approvals=[]):
         super(ChangeishFilter, self).__init__(
-            required_approvals=required_approvals)
+            required_approvals=required_approvals,
+            reject_approvals=reject_approvals)
         self.open = open
         self.current_patchset = current_patchset
         self.statuses = statuses
@@ -1273,7 +1309,11 @@ class ChangeishFilter(BaseFilter):
         if self.statuses:
             ret += ' statuses: %s' % ', '.join(self.statuses)
         if self.required_approvals:
-            ret += ' required_approvals: %s' % str(self.required_approvals)
+            ret += (' required_approvals: %s' %
+                    str(self.required_approvals))
+        if self.reject_approvals:
+            ret += (' reject_approvals: %s' %
+                    str(self.reject_approvals))
         ret += '>'
 
         return ret
@@ -1291,12 +1331,8 @@ class ChangeishFilter(BaseFilter):
             if change.status not in self.statuses:
                 return False
 
-        if self.required_approvals and not change.approvals:
-            # A change with no approvals can not match
-            return False
-
-        # required approvals are ANDed
-        if not self.matchesRequiredApprovals(change):
+        # required approvals are ANDed (reject approvals are ORed)
+        if not self.matchesApprovals(change):
             return False
 
         return True

zuul/scheduler.py

@@ -326,13 +326,16 @@ class Scheduler(threading.Thread):
             pipeline.setManager(manager)
             layout.pipelines[conf_pipeline['name']] = pipeline
 
-            if 'require' in conf_pipeline:
-                require = conf_pipeline['require']
+            if 'require' in conf_pipeline or 'reject' in conf_pipeline:
+                require = conf_pipeline.get('require', {})
+                reject = conf_pipeline.get('reject', {})
                 f = ChangeishFilter(
                     open=require.get('open'),
                     current_patchset=require.get('current-patchset'),
                     statuses=toList(require.get('status')),
-                    required_approvals=toList(require.get('approval')))
+                    required_approvals=toList(require.get('approval')),
+                    reject_approvals=toList(reject.get('approval'))
+                )
                 manager.changeish_filters.append(f)
 
             # TODO: move this into triggers (may require pluggable
@@ -363,8 +366,11 @@ class Scheduler(threading.Thread):
                         comments=comments,
                         emails=emails,
                         usernames=usernames,
-                        required_approvals=toList(
-                            trigger.get('require-approval')
+                        required_approvals=(
+                            toList(trigger.get('require-approval'))
+                        ),
+                        reject_approvals=toList(
+                            trigger.get('reject-approval')
                         ),
                         ignore_deletes=ignore_deletes
                     )
@@ -381,9 +387,12 @@ class Scheduler(threading.Thread):
                         trigger=self.triggers['zuul'],
                         types=toList(trigger['event']),
                         pipelines=toList(trigger.get('pipeline')),
-                        required_approvals=toList(
-                            trigger.get('require-approval')
-                        )
+                        required_approvals=(
+                            toList(trigger.get('require-approval'))
+                        ),
+                        reject_approvals=toList(
+                            trigger.get('reject-approval')
+                        ),
                     )
                     manager.event_filters.append(f)