zuul
/
zuul
Code Issues Proposed changes

Revert "Add support for negative requirements" 

This reverts commit b2068e80a8.

This change does not appear to do the correct thing with "any" and
caused issues for us in production.  Our gate requirement of
jenkins>=+1 _and_ workflow=+1 became jenkins >=+1 or workflow=+1.

Change-Id: Ie5aa776b2dd718fcf025148073a0171332f358fb
This commit is contained in:
James E. Blair 2015-07-30 18:08:24 +00:00 committed by James E. Blair
parent adda0d6954
commit 5bf78a3ade
15 changed files with 81 additions and 433 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
doc/source/zuul.rst
View File

@ -462,21 +462,13 @@ explanation of each of the parameters::
A deprecated alternate spelling of *comment*. Only one of *comment* or
*comment_filter* should be used.
*require-any-approval*
*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 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
@ -523,7 +515,7 @@ explanation of each of the parameters::
.. _pipeline-require-approval:
**any-approval**
**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
@ -557,24 +549,6 @@ 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.

41
tests/fixtures/layout-requirement-all.yaml vendored
View File

@ -1,41 +0,0 @@
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

43
tests/fixtures/layout-requirement-any.yaml vendored
View File

@ -1,43 +0,0 @@
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

4
tests/fixtures/layout-requirement-email.yaml vendored
View File

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

37
tests/fixtures/layout-requirement-negative-username.yaml vendored
View File

@ -1,37 +0,0 @@
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

4
tests/fixtures/layout-requirement-newer-than.yaml vendored
View File

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

4
tests/fixtures/layout-requirement-older-than.yaml vendored
View File

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

4
tests/fixtures/layout-requirement-username.yaml vendored
View File

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

4
tests/fixtures/layout-requirement-vote.yaml vendored
View File

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

4
tests/fixtures/layout-requirement-vote1.yaml vendored
View File

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

4
tests/fixtures/layout-requirement-vote2.yaml vendored
View File

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

128
tests/test_requirements.py
View File

@ -323,131 +323,3 @@ 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)

6
zuul/layoutvalidator.py
View File

@ -62,8 +62,6 @@ 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),
}
@ -87,9 +85,7 @@ class LayoutSchema(object):
},
}
require = {'all-approvals': toList(require_approval),
'any-approval': toList(require_approval),
'approval': toList(require_approval),
require = {'approval': toList(require_approval),
'open': bool,
'current-patchset': bool,
'status': toList(str)}

176
zuul/model.py
View File

@ -12,10 +12,8 @@
# 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
@ -1027,127 +1025,68 @@ class TriggerEvent(object):
class BaseFilter(object):
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)
def __init__(self, required_approvals=[]):
self._required_approvals = copy.deepcopy(required_approvals)
self.required_approvals = required_approvals
def _tidy_approvals(self, approvals):
for a in approvals:
for a in self.required_approvals:
for k, v in a.items():
if k == 'username':
pass
elif k in ['email', 'email-filter']:
a['email'] = v
a['email'] = re.compile(v)
elif k == 'newer-than':
a[k] = v
a[k] = time_to_seconds(v)
elif k == 'older-than':
a[k] = v
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()
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):
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:
now = time.time()
for rapproval in self.required_approvals:
matches_approval = False
for approval in change.approvals:
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
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
return True
class EventFilter(BaseFilter):
def __init__(self, trigger, types=[], branches=[], refs=[],
event_approvals={}, comments=[], emails=[], usernames=[],
timespecs=[], required_any_approval=[],
required_all_approvals=[], pipelines=[]):
timespecs=[], required_approvals=[], pipelines=[]):
super(EventFilter, self).__init__(
required_any_approval=required_any_approval,
required_all_approvals=required_all_approvals)
required_approvals=required_approvals)
self.trigger = trigger
self._types = types
self._branches = branches
@ -1180,12 +1119,9 @@ 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_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.required_approvals:
ret += ' required_approvals: %s' % ', '.join(
['%s' % a for a in self._required_approvals])
if self._comments:
ret += ' comments: %s' % ', '.join(self._comments)
if self._emails:
@ -1274,6 +1210,10 @@ 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
@ -1291,11 +1231,9 @@ class EventFilter(BaseFilter):
class ChangeishFilter(BaseFilter):
def __init__(self, open=None, current_patchset=None,
statuses=[], required_any_approval=[],
required_all_approvals=[]):
statuses=[], required_approvals=[]):
super(ChangeishFilter, self).__init__(
required_any_approval=required_any_approval,
required_all_approvals=required_all_approvals)
required_approvals=required_approvals)
self.open = open
self.current_patchset = current_patchset
self.statuses = statuses
@ -1309,12 +1247,8 @@ class ChangeishFilter(BaseFilter):
ret += ' current-patchset: %s' % self.current_patchset
if self.statuses:
ret += ' statuses: %s' % ', '.join(self.statuses)
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))
if self.required_approvals:
ret += ' required_approvals: %s' % str(self.required_approvals)
ret += '>'
return ret
@ -1332,6 +1266,10 @@ 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

25
zuul/scheduler.py
View File

@ -327,10 +327,7 @@ class Scheduler(threading.Thread):
open=require.get('open'),
current_patchset=require.get('current-patchset'),
statuses=toList(require.get('status')),
required_any_approval=(toList(require.get('any-approval'))
+ toList(require.get('approval'))),
required_all_approvals=toList(require.get('all-approvals'))
)
required_approvals=toList(require.get('approval')))
manager.changeish_filters.append(f)
# TODO: move this into triggers (may require pluggable
@ -360,13 +357,9 @@ class Scheduler(threading.Thread):
comments=comments,
emails=emails,
usernames=usernames,
required_any_approval=(
toList(trigger.get('require-any-approval'))
+ toList(trigger.get('require-approval'))
),
required_all_approvals=toList(
trigger.get('require-all-approvals')
),
required_approvals=toList(
trigger.get('require-approval')
)
)
manager.event_filters.append(f)
if 'timer' in conf_pipeline['trigger']:
@ -381,13 +374,9 @@ class Scheduler(threading.Thread):
trigger=self.triggers['zuul'],
types=toList(trigger['event']),
pipelines=toList(trigger.get('pipeline')),
required_any_approval=(
toList(trigger.get('require-any-approval'))
+ toList(trigger.get('require-approval'))
),
required_all_approvals=toList(
trigger.get('require-all-approvals')
),
required_approvals=toList(
trigger.get('require-approval')
)
)
manager.event_filters.append(f)