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
diff --git a/doc/source/zuul.rst b/doc/source/zuul.rst
index d597f23..700c9f7 100644
--- a/doc/source/zuul.rst
+++ b/doc/source/zuul.rst
@@ -477,7 +477,13 @@
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 @@
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 @@
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
diff --git a/tests/fixtures/layout-requirement-reject-username.yaml b/tests/fixtures/layout-requirement-reject-username.yaml
new file mode 100644
index 0000000..9c71045
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/tests/fixtures/layout-requirement-reject.yaml b/tests/fixtures/layout-requirement-reject.yaml
new file mode 100644
index 0000000..1f5d714
--- /dev/null
+++ b/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
diff --git a/tests/fixtures/layouts/bad_reject.yaml b/tests/fixtures/layouts/bad_reject.yaml
new file mode 100644
index 0000000..b1e7f84
--- /dev/null
+++ b/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
diff --git a/tests/fixtures/layouts/good_layout.yaml b/tests/fixtures/layouts/good_layout.yaml
index 9ba1806..3608d0c 100644
--- a/tests/fixtures/layouts/good_layout.yaml
+++ b/tests/fixtures/layouts/good_layout.yaml
@@ -4,9 +4,18 @@
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 @@
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
diff --git a/tests/test_requirements.py b/tests/test_requirements.py
index 4316925..3ae56ad 100644
--- a/tests/test_requirements.py
+++ b/tests/test_requirements.py
@@ -323,3 +323,105 @@
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')
diff --git a/zuul/layoutvalidator.py b/zuul/layoutvalidator.py
index 781f475..fd4f498 100644
--- a/zuul/layoutvalidator.py
+++ b/zuul/layoutvalidator.py
@@ -38,12 +38,12 @@
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 @@
'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 @@
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 @@
},
}
- 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 @@
'precedence': precedence,
'description': str,
'require': require,
+ 'reject': reject,
'success-message': str,
'failure-message': str,
'merge-failure-message': str,
diff --git a/zuul/model.py b/zuul/model.py
index 1b94029..9106887 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -1042,11 +1042,14 @@
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 @@
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 @@
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 @@
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 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 @@
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 @@
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
diff --git a/zuul/scheduler.py b/zuul/scheduler.py
index 9af6b66..2355801 100644
--- a/zuul/scheduler.py
+++ b/zuul/scheduler.py
@@ -326,13 +326,16 @@
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 @@
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 @@
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)