gerrit: Add `approval-change` trigger
Adds a new type of trigger to the Gerrit driver that only triggers if the approval value was changed by the user in the comment. This is useful if Zuul is configured to allow many different scores to trigger a pipeline (with an additional requirement on all of them), but arbitrary comments made while the scores are present should _not_ trigger (or potentially re-trigger) the pipeline. This can happen because Gerrit sends all approvals by a user on all comments, regardless of if they were changed by the comment. The new `approval-change` trigger requirement inspects the `oldValue` field in the Gerrit event. The pipeline will only trigger if this value is present and not equal to the new approval value (thus, only when the user actually changed it). `oldValue` has been present since at least Gerrit 3.4 Change-Id: I88cf840ae8b4e63c77f10ee68b6901e85f7c5fb1
This commit is contained in:
parent
25cc922116
commit
ffb615e6c7
|
@ -404,6 +404,14 @@ Trigger Configuration
|
||||||
Example: ``Code-Review: 2`` matches a ``+2`` vote on the code
|
Example: ``Code-Review: 2`` matches a ``+2`` vote on the code
|
||||||
review category. Multiple approvals may be listed.
|
review category. Multiple approvals may be listed.
|
||||||
|
|
||||||
|
.. attr:: approval-change
|
||||||
|
|
||||||
|
This is only used for ``comment-added`` events. It works the same way as
|
||||||
|
``approval``, with the additional requirement that the approval value
|
||||||
|
must have changed from its previous value. This means that it only
|
||||||
|
matches when a user modifies an approval score instead of any comment
|
||||||
|
where the score is present.
|
||||||
|
|
||||||
.. attr:: email
|
.. attr:: email
|
||||||
|
|
||||||
This is used for any event. It takes a regex applied on the
|
This is used for any event. It takes a regex applied on the
|
||||||
|
|
|
@ -0,0 +1,7 @@
|
||||||
|
---
|
||||||
|
features:
|
||||||
|
- |
|
||||||
|
The gerrit driver now supports a new synthetic trigger,
|
||||||
|
:attr:`pipeline.trigger.<gerrit source>.approval-change`. This is
|
||||||
|
similar to `approval`, but only triggers when the value of the
|
||||||
|
approval changes instead of simply being present.
|
|
@ -735,7 +735,7 @@ class FakeGerritChange(object):
|
||||||
return event
|
return event
|
||||||
|
|
||||||
def addApproval(self, category, value, username='reviewer_john',
|
def addApproval(self, category, value, username='reviewer_john',
|
||||||
granted_on=None, message='', tag=None):
|
granted_on=None, message='', tag=None, old_value=None):
|
||||||
if not granted_on:
|
if not granted_on:
|
||||||
granted_on = time.time()
|
granted_on = time.time()
|
||||||
approval = {
|
approval = {
|
||||||
|
@ -749,6 +749,8 @@ class FakeGerritChange(object):
|
||||||
'grantedOn': int(granted_on),
|
'grantedOn': int(granted_on),
|
||||||
'__tag': tag, # Not available in ssh api
|
'__tag': tag, # Not available in ssh api
|
||||||
}
|
}
|
||||||
|
if old_value is not None:
|
||||||
|
approval['oldValue'] = str(old_value)
|
||||||
for i, x in enumerate(self.patchsets[-1]['approvals'][:]):
|
for i, x in enumerate(self.patchsets[-1]['approvals'][:]):
|
||||||
if x['by']['username'] == username and x['type'] == category:
|
if x['by']['username'] == username and x['type'] == category:
|
||||||
del self.patchsets[-1]['approvals'][i]
|
del self.patchsets[-1]['approvals'][i]
|
||||||
|
|
|
@ -0,0 +1,39 @@
|
||||||
|
- pipeline:
|
||||||
|
name: check
|
||||||
|
manager: independent
|
||||||
|
trigger:
|
||||||
|
gerrit:
|
||||||
|
- event: comment-added
|
||||||
|
approval-change:
|
||||||
|
Code-Review: 2
|
||||||
|
|
||||||
|
require:
|
||||||
|
gerrit:
|
||||||
|
approval:
|
||||||
|
- Code-Review: 2
|
||||||
|
success:
|
||||||
|
gerrit:
|
||||||
|
Verified: 1
|
||||||
|
failure:
|
||||||
|
gerrit:
|
||||||
|
Verified: -1
|
||||||
|
|
||||||
|
- job:
|
||||||
|
name: base
|
||||||
|
parent: null
|
||||||
|
run: playbooks/base.yaml
|
||||||
|
nodeset:
|
||||||
|
nodes:
|
||||||
|
- label: ubuntu-xenial
|
||||||
|
name: controller
|
||||||
|
|
||||||
|
- job:
|
||||||
|
name: check-job
|
||||||
|
run: playbooks/check.yaml
|
||||||
|
|
||||||
|
- project:
|
||||||
|
name: org/project
|
||||||
|
check:
|
||||||
|
jobs:
|
||||||
|
- check-job
|
||||||
|
|
|
@ -1305,3 +1305,30 @@ class TestGerritDriver(ZuulTestCase):
|
||||||
self.assertHistory([
|
self.assertHistory([
|
||||||
dict(name='check-job', result='SUCCESS', changes='1,1'),
|
dict(name='check-job', result='SUCCESS', changes='1,1'),
|
||||||
])
|
])
|
||||||
|
|
||||||
|
@simple_layout('layouts/gerrit-approval-change.yaml')
|
||||||
|
def test_approval_change(self):
|
||||||
|
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
|
||||||
|
A.data['hashtags'] = ['check']
|
||||||
|
self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2))
|
||||||
|
self.waitUntilSettled()
|
||||||
|
|
||||||
|
# Does not meet pipeline requirements, because old value is not present
|
||||||
|
self.assertHistory([])
|
||||||
|
|
||||||
|
# Still not sufficient because old value matches new value
|
||||||
|
self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2, old_value=2))
|
||||||
|
self.waitUntilSettled()
|
||||||
|
self.assertHistory([])
|
||||||
|
|
||||||
|
# Old value present, but new value is insufficient
|
||||||
|
self.fake_gerrit.addEvent(A.addApproval('Code-Review', 0, old_value=2))
|
||||||
|
self.waitUntilSettled()
|
||||||
|
self.assertHistory([])
|
||||||
|
|
||||||
|
# This should work now
|
||||||
|
self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2, old_value=0))
|
||||||
|
self.waitUntilSettled()
|
||||||
|
self.assertHistory([
|
||||||
|
dict(name='check-job', result='SUCCESS', changes='1,1'),
|
||||||
|
])
|
||||||
|
|
|
@ -280,11 +280,11 @@ class GerritTriggerEvent(TriggerEvent):
|
||||||
|
|
||||||
class GerritEventFilter(EventFilter):
|
class GerritEventFilter(EventFilter):
|
||||||
def __init__(self, connection_name, trigger, types=[], branches=[],
|
def __init__(self, connection_name, trigger, types=[], branches=[],
|
||||||
refs=[], event_approvals={}, comments=[], emails=[],
|
refs=[], event_approvals={}, event_approval_changes={},
|
||||||
usernames=[], required_approvals=[], reject_approvals=[],
|
comments=[], emails=[], usernames=[], required_approvals=[],
|
||||||
added=[], removed=[],
|
reject_approvals=[], added=[], removed=[], uuid=None,
|
||||||
uuid=None, scheme=None, ignore_deletes=True,
|
scheme=None, ignore_deletes=True, require=None, reject=None,
|
||||||
require=None, reject=None, parse_context=None):
|
parse_context=None):
|
||||||
|
|
||||||
EventFilter.__init__(self, connection_name, trigger)
|
EventFilter.__init__(self, connection_name, trigger)
|
||||||
|
|
||||||
|
@ -323,6 +323,7 @@ class GerritEventFilter(EventFilter):
|
||||||
self.added = added
|
self.added = added
|
||||||
self.removed = removed
|
self.removed = removed
|
||||||
self.event_approvals = event_approvals
|
self.event_approvals = event_approvals
|
||||||
|
self.event_approval_changes = event_approval_changes
|
||||||
self.uuid = uuid
|
self.uuid = uuid
|
||||||
self.scheme = scheme
|
self.scheme = scheme
|
||||||
self.ignore_deletes = ignore_deletes
|
self.ignore_deletes = ignore_deletes
|
||||||
|
@ -346,6 +347,9 @@ class GerritEventFilter(EventFilter):
|
||||||
if self.event_approvals:
|
if self.event_approvals:
|
||||||
ret += ' event_approvals: %s' % ', '.join(
|
ret += ' event_approvals: %s' % ', '.join(
|
||||||
['%s:%s' % a for a in self.event_approvals.items()])
|
['%s:%s' % a for a in self.event_approvals.items()])
|
||||||
|
if self.event_approval_changes:
|
||||||
|
ret += ' event_approval_changes: %s' % ', '.join(
|
||||||
|
['%s:%s' % a for a in self.event_approval_changes.items()])
|
||||||
if self._comments:
|
if self._comments:
|
||||||
ret += ' comments: %s' % ', '.join(self._comments)
|
ret += ' comments: %s' % ', '.join(self._comments)
|
||||||
if self._emails:
|
if self._emails:
|
||||||
|
@ -457,6 +461,19 @@ class GerritEventFilter(EventFilter):
|
||||||
return FalseWithReason("Approvals %s do not match %s" % (
|
return FalseWithReason("Approvals %s do not match %s" % (
|
||||||
self.event_approvals, event.approvals))
|
self.event_approvals, event.approvals))
|
||||||
|
|
||||||
|
for category, value in self.event_approval_changes.items():
|
||||||
|
matches_approval = False
|
||||||
|
for eapp in event.approvals:
|
||||||
|
if (eapp['description'] == category and
|
||||||
|
int(eapp['value']) == int(value) and
|
||||||
|
'oldValue' in eapp and
|
||||||
|
int(eapp['value']) != int(eapp['oldValue'])):
|
||||||
|
matches_approval = True
|
||||||
|
if not matches_approval:
|
||||||
|
return FalseWithReason(
|
||||||
|
"Changed approvals %s do not match %s" % (
|
||||||
|
self.event_approval_changes, event.approvals))
|
||||||
|
|
||||||
# hashtags are ORed
|
# hashtags are ORed
|
||||||
if self.added:
|
if self.added:
|
||||||
matches_token = False
|
matches_token = False
|
||||||
|
|
|
@ -52,6 +52,10 @@ class GerritTrigger(BaseTrigger):
|
||||||
for approval_dict in to_list(trigger.get('approval')):
|
for approval_dict in to_list(trigger.get('approval')):
|
||||||
for key, val in approval_dict.items():
|
for key, val in approval_dict.items():
|
||||||
approvals[key] = val
|
approvals[key] = val
|
||||||
|
approval_changes = {}
|
||||||
|
for approval_dict in to_list(trigger.get('approval-change')):
|
||||||
|
for key, val in approval_dict.items():
|
||||||
|
approval_changes[key] = val
|
||||||
# Backwards compat for *_filter versions of these args
|
# Backwards compat for *_filter versions of these args
|
||||||
attrname = 'comment' if 'comment' in trigger else 'comment_filter'
|
attrname = 'comment' if 'comment' in trigger else 'comment_filter'
|
||||||
with pcontext.confAttr(trigger, attrname) as attr:
|
with pcontext.confAttr(trigger, attrname) as attr:
|
||||||
|
@ -96,6 +100,7 @@ class GerritTrigger(BaseTrigger):
|
||||||
branches=branches,
|
branches=branches,
|
||||||
refs=refs,
|
refs=refs,
|
||||||
event_approvals=approvals,
|
event_approvals=approvals,
|
||||||
|
event_approval_changes=approval_changes,
|
||||||
comments=comments,
|
comments=comments,
|
||||||
emails=emails,
|
emails=emails,
|
||||||
usernames=usernames,
|
usernames=usernames,
|
||||||
|
@ -153,6 +158,7 @@ def getSchema():
|
||||||
'ref': scalar_or_list(v.Any(ZUUL_REGEX, str)),
|
'ref': scalar_or_list(v.Any(ZUUL_REGEX, str)),
|
||||||
'ignore-deletes': bool,
|
'ignore-deletes': bool,
|
||||||
'approval': scalar_or_list(variable_dict),
|
'approval': scalar_or_list(variable_dict),
|
||||||
|
'approval-change': scalar_or_list(variable_dict),
|
||||||
'require-approval': scalar_or_list(approval),
|
'require-approval': scalar_or_list(approval),
|
||||||
'reject-approval': scalar_or_list(approval),
|
'reject-approval': scalar_or_list(approval),
|
||||||
'added': scalar_or_list(v.Any(ZUUL_REGEX, str)),
|
'added': scalar_or_list(v.Any(ZUUL_REGEX, str)),
|
||||||
|
|
Loading…
Reference in New Issue