Merge "Add support for negative requirements"

This commit is contained in:
Jenkins 2015-07-20 19:15:49 +00:00 committed by Gerrit Code Review
commit 5fe1b12b86
15 changed files with 433 additions and 81 deletions

View File

@ -462,13 +462,21 @@ explanation of each of the parameters::
A deprecated alternate spelling of *comment*. Only one of *comment* or
*comment_filter* should be used.
*require-approval*
*require-any-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 follows the
same syntax as the :ref:`"approval" pipeline requirement below
<pipeline-require-approval>`.
*require-all-approvals*
This takes a list of approvals in the same format as
*require-any-approval* but requires all approvals match the rules.
**require-approval** (depreciated)
A deprecated alternate spelling of *require-any-approval*. This will
be joined with *require-any-approval* if both are present.
**timer**
This trigger will run based on a cron-style time specification.
It will enqueue an event into its pipeline for every project
@ -515,7 +523,7 @@ explanation of each of the parameters::
.. _pipeline-require-approval:
**approval**
**any-approval**
This 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
@ -549,6 +557,24 @@ explanation of each of the parameters::
be a single value or a list: ``verified: [1, 2]`` would match
either a +1 or +2 vote.
You can also match negative conditions by starting with an
exclamation mark (!). This requires the value to be a string.
Example: ``verified: '![-1, -2]'``
This takes a list of approvals in the same format as above. It
requires that any approval on a change can meet the specified
criteria.
**all-approvals**
This takes a list of approvals in the same format as *any-approval* but
requires all approvals match the rules. For example, you can stop any
new changes from queueing when there is a negative vote by requiring
all approves to not have a -1.
**approval** (depreciated)
A deprecated alternate spelling of *any-approval*. This will be
joined with *any-approval* if both are present.
**open**
A boolean value (``true`` or ``false``) that indicates whether the change
must be open or closed in order to be enqueued.

View File

@ -0,0 +1,41 @@
pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
all-approvals:
- username: jenkins
verified: [1, 2]
- 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-all-approvals:
- username: jenkins
verified: [1, 2]
- verified: "![-1, -2]"
success:
gerrit:
verified: 1
failure:
gerrit:
verified: -1
projects:
- name: org/project1
pipeline:
- project1-pipeline
- name: org/project2
trigger:
- project2-trigger

View File

@ -0,0 +1,43 @@
pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
any-approval:
- username: jenkins
verified: [1, 2]
- username: core-reviewer
code-review: "![-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-any-approval:
- username: jenkins
verified: [1, 2]
- username: core-reviewer
code-review: "![-1, -2]"
success:
gerrit:
verified: 1
failure:
gerrit:
verified: -1
projects:
- name: org/project1
pipeline:
- project1-pipeline
- name: org/project2
trigger:
- project2-trigger

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- email: jenkins@example.com
trigger:
gerrit:
@ -19,7 +19,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- email: jenkins@example.com
success:
gerrit:

View File

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

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
newer-than: 48h
trigger:
@ -20,7 +20,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
newer-than: 48h
success:

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
older-than: 48h
trigger:
@ -20,7 +20,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
older-than: 48h
success:

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
trigger:
gerrit:
@ -19,7 +19,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
success:
gerrit:

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
verified: 1
trigger:
@ -20,7 +20,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
verified: 1
success:

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
verified: 1
trigger:
@ -20,7 +20,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
verified: 1
success:

View File

@ -2,7 +2,7 @@ pipelines:
- name: pipeline
manager: IndependentPipelineManager
require:
approval:
any-approval:
- username: jenkins
verified: [1, 2]
trigger:
@ -20,7 +20,7 @@ pipelines:
trigger:
gerrit:
- event: comment-added
require-approval:
require-any-approval:
- username: jenkins
verified: [1, 2]
success:

View File

@ -323,3 +323,131 @@ class TestRequirements(ZuulTestCase):
self.fake_gerrit.addEvent(B.addApproval('CRVW', 2))
self.waitUntilSettled()
self.assertEqual(len(self.history), 1)
def test_pipeline_require_negative_username(self):
"Test negative pipeline requirement: no comment from jenkins"
return self._test_require_negative_username('org/project1',
'project1-pipeline')
def test_trigger_require_negative_username(self):
"Test negative trigger requirement: no comment from jenkins"
return self._test_require_negative_username('org/project2',
'project2-trigger')
def _test_require_negative_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-negative-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_require_any(self):
"Test pipeline requirement: any requirement passes"
return self._test_require_any('org/project1', 'project1-pipeline')
def test_trigger_require_any(self):
"Test trigger requirement: any requirement passes"
return self._test_require_any('org/project2', 'project2-trigger')
def _test_require_any(self, project, job):
"Test any of the given requirements are matched"
self.config.set(
'zuul', 'layout_config',
'tests/fixtures/layout-requirement-any.yaml')
self.sched.reconfigure(self.config)
self.registerJobs()
A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
# A comment event that we will keep submitting to trigger
comment = A.addApproval('CRVW', 1, username='nobody')
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
# No approval from Jenkins so should not be enqueued
self.assertEqual(len(self.history), 0)
# A +1 from jenkins should allow it to be enqueued
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)
# A non-negative from a non-core should not queue
B = self.fake_gerrit.addFakeChange(project, 'master', 'B')
# A comment event that we will keep submitting to trigger
comment = B.addApproval('CRVW', 1, username='nobody')
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
self.assertEqual(len(self.history), 1)
# A non-negative from a core member should queue
B.addApproval('CRVW', 2, username='core-reviewer')
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
self.assertEqual(len(self.history), 2)
self.assertEqual(self.history[1].name, job)
def test_pipeline_require_all(self):
"Test pipeline requirement: all requirements pass"
return self._test_require_all('org/project1', 'project1-pipeline')
def test_trigger_require_all(self):
"Test trigger requirement: all requirements pass"
return self._test_require_all('org/project2', 'project2-trigger')
def _test_require_all(self, project, job):
"Test all of the given requirements are matched"
self.config.set(
'zuul', 'layout_config',
'tests/fixtures/layout-requirement-all.yaml')
self.sched.reconfigure(self.config)
self.registerJobs()
A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
self.waitUntilSettled()
self.assertEqual(len(self.history), 0)
# A +2 from 'nobody' only satisfies the non-negative requirement,
# not the requirement to be from 'jenkins'
comment = A.addApproval('VRFY', 1, username='nobody')
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
self.assertEqual(len(self.history), 0)
B = self.fake_gerrit.addFakeChange(project, 'master', 'A')
self.waitUntilSettled()
self.assertEqual(len(self.history), 0)
# A +2 from Jenkins satisfies both the user condition and the
# non-negative condition
comment = B.addApproval('VRFY', 2, username='jenkins')
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
self.assertEqual(len(self.history), 1)
self.assertEqual(self.history[0].name, job)

View File

@ -62,6 +62,8 @@ class LayoutSchema(object):
'branch': toList(str),
'ref': toList(str),
'approval': toList(variable_dict),
'require-all-approvals': toList(require_approval),
'require-any-approval': toList(require_approval),
'require-approval': toList(require_approval),
}
@ -85,7 +87,9 @@ class LayoutSchema(object):
},
}
require = {'approval': toList(require_approval),
require = {'all-approvals': toList(require_approval),
'any-approval': toList(require_approval),
'approval': toList(require_approval),
'open': bool,
'current-patchset': bool,
'status': toList(str)}

View File

@ -12,8 +12,10 @@
# License for the specific language governing permissions and limitations
# under the License.
import ast
import copy
import re
import six
import time
from uuid import uuid4
import extras
@ -1025,68 +1027,127 @@ class TriggerEvent(object):
class BaseFilter(object):
def __init__(self, required_approvals=[]):
self._required_approvals = copy.deepcopy(required_approvals)
self.required_approvals = required_approvals
def __init__(self, required_any_approval=[], required_all_approvals=[]):
self._required_any_approval = copy.deepcopy(required_any_approval)
self.required_any_approval = self._tidy_approvals(
required_any_approval)
self._required_all_approvals = copy.deepcopy(required_all_approvals)
self.required_all_approvals = self._tidy_approvals(
required_all_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
elif k in ['email', 'email-filter']:
a['email'] = re.compile(v)
a['email'] = v
elif k == 'newer-than':
a[k] = time_to_seconds(v)
a[k] = v
elif k == 'older-than':
a[k] = time_to_seconds(v)
else:
if not isinstance(v, list):
a[k] = [v]
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()
found_approval = True
by = approval.get('by', {})
for k, v in rapproval.items():
negative_match = False
item_match = True
if isinstance(v, six.string_types) and v[0] == '!':
v = v[1:].strip()
item_match = False
negative_match = True
if k == 'username':
if (by.get('username', '') != v):
item_match = negative_match
elif k == 'email':
v = re.compile(v)
if (not v.search(by.get('email', ''))):
item_match = negative_match
elif k == 'newer-than':
t = now - time_to_seconds(v)
if (approval['grantedOn'] < t):
item_match = negative_match
elif k == 'older-than':
t = now - time_to_seconds(v)
if (approval['grantedOn'] >= t):
item_match = negative_match
else:
if isinstance(v, six.string_types):
v = ast.literal_eval(v)
if not isinstance(v, list):
v = [v]
if (normalizeCategory(approval['description']) != k or
int(approval['value']) not in v):
item_match = negative_match
if not item_match:
found_approval = False
return found_approval
def matchesRequiredApprovals(self, change):
now = time.time()
for rapproval in self.required_approvals:
if (self.required_any_approval and not change.approvals
or self.required_all_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 ANY and ALL 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.matchesRequiredAnyApproval(change) and
self.matchesRequiredAllApprovals(change))
def matchesRequiredAnyApproval(self, change):
# Check if any approvals match the any requirements
if not self.required_any_approval:
# No approval required, so we must match
return True
for rapproval in self.required_any_approval:
matches_approval = 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
break
if not matches_approval:
return False
matches_approval = self._match_approval_required_approval(
rapproval, approval)
if matches_approval:
# We have a matching approval so this requirement is
# fulfilled
return True
return False
def matchesRequiredAllApprovals(self, change):
# Check that /all/ of the approvals match the requirements
if not self.required_all_approvals:
# No approvals required, so we must match
return True
for rapproval in self.required_all_approvals:
for approval in change.approvals:
matches_approval = self._match_approval_required_approval(
rapproval, approval)
if not matches_approval:
# We have an approval that doesn't match so this
# requirement can't be fulfilled
return False
# We must have matched everything
return True
class EventFilter(BaseFilter):
def __init__(self, trigger, types=[], branches=[], refs=[],
event_approvals={}, comments=[], emails=[], usernames=[],
timespecs=[], required_approvals=[], pipelines=[]):
timespecs=[], required_any_approval=[],
required_all_approvals=[], pipelines=[]):
super(EventFilter, self).__init__(
required_approvals=required_approvals)
required_any_approval=required_any_approval,
required_all_approvals=required_all_approvals)
self.trigger = trigger
self._types = types
self._branches = branches
@ -1119,9 +1180,12 @@ class EventFilter(BaseFilter):
if self.event_approvals:
ret += ' event_approvals: %s' % ', '.join(
['%s:%s' % a for a in self.event_approvals.items()])
if self.required_approvals:
ret += ' required_approvals: %s' % ', '.join(
['%s' % a for a in self._required_approvals])
if self.required_any_approval:
ret += ' required_any_approval: %s' % ', '.join(
['%s' % a for a in self._required_any_approval])
if self.required_all_approvals:
ret += ' required_all_approvals: %s' % ', '.join(
['%s' % a for a in self._required_all_approvals])
if self._comments:
ret += ' comments: %s' % ', '.join(self._comments)
if self._emails:
@ -1210,10 +1274,6 @@ 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):
return False
@ -1231,9 +1291,11 @@ class EventFilter(BaseFilter):
class ChangeishFilter(BaseFilter):
def __init__(self, open=None, current_patchset=None,
statuses=[], required_approvals=[]):
statuses=[], required_any_approval=[],
required_all_approvals=[]):
super(ChangeishFilter, self).__init__(
required_approvals=required_approvals)
required_any_approval=required_any_approval,
required_all_approvals=required_all_approvals)
self.open = open
self.current_patchset = current_patchset
self.statuses = statuses
@ -1247,8 +1309,12 @@ class ChangeishFilter(BaseFilter):
ret += ' current-patchset: %s' % self.current_patchset
if self.statuses:
ret += ' statuses: %s' % ', '.join(self.statuses)
if self.required_approvals:
ret += ' required_approvals: %s' % str(self.required_approvals)
if self.required_any_approval:
ret += (' required_any_approval: %s' %
str(self.required_any_approval))
if self.required_all_approvals:
ret += (' required_all_approvals: %s' %
str(self.required_all_approvals))
ret += '>'
return ret
@ -1266,10 +1332,6 @@ 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):
return False

View File

@ -326,7 +326,10 @@ class Scheduler(threading.Thread):
open=require.get('open'),
current_patchset=require.get('current-patchset'),
statuses=toList(require.get('status')),
required_approvals=toList(require.get('approval')))
required_any_approval=(toList(require.get('any-approval'))
+ toList(require.get('approval'))),
required_all_approvals=toList(require.get('all-approvals'))
)
manager.changeish_filters.append(f)
# TODO: move this into triggers (may require pluggable
@ -356,9 +359,13 @@ class Scheduler(threading.Thread):
comments=comments,
emails=emails,
usernames=usernames,
required_approvals=toList(
trigger.get('require-approval')
)
required_any_approval=(
toList(trigger.get('require-any-approval'))
+ toList(trigger.get('require-approval'))
),
required_all_approvals=toList(
trigger.get('require-all-approvals')
),
)
manager.event_filters.append(f)
if 'timer' in conf_pipeline['trigger']:
@ -373,9 +380,13 @@ 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_any_approval=(
toList(trigger.get('require-any-approval'))
+ toList(trigger.get('require-approval'))
),
required_all_approvals=toList(
trigger.get('require-all-approvals')
),
)
manager.event_filters.append(f)