summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--doc/source/drivers/gerrit.rst94
-rw-r--r--releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml22
-rw-r--r--tests/base.py1
-rw-r--r--tests/fixtures/layouts/gerrit-trigger-requirements.yaml162
-rw-r--r--tests/unit/test_requirements.py221
-rw-r--r--zuul/driver/gerrit/gerritmodel.py381
-rw-r--r--zuul/driver/gerrit/gerritsource.py28
-rw-r--r--zuul/driver/gerrit/gerrittrigger.py7
-rw-r--r--zuul/driver/github/githubmodel.py42
9 files changed, 761 insertions, 197 deletions
diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst
index 4b9c93044..3c16f202a 100644
--- a/doc/source/drivers/gerrit.rst
+++ b/doc/source/drivers/gerrit.rst
@@ -239,6 +239,10 @@ be able to invoke the ``gerrit stream-events`` command over SSH.
.. attr:: require-approval
+ .. warning:: This is deprecated and will be removed in a future
+ version. Use :attr:`pipeline.trigger.<gerrit
+ source>.require` instead.
+
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
@@ -246,13 +250,40 @@ be able to invoke the ``gerrit stream-events`` command over SSH.
source>.approval`. For each specified criteria there must exist
a matching approval.
+ This is ignored if the :attr:`pipeline.trigger.<gerrit
+ source>.require` attribute is present.
+
.. attr:: reject-approval
+ .. warning:: This is deprecated and will be removed in a future
+ version. Use :attr:`pipeline.trigger.<gerrit
+ source>.reject` instead.
+
This takes a list of approvals in the same format as
:attr:`pipeline.trigger.<gerrit source>.require-approval` but
the item will fail to enter the pipeline if there is a matching
approval.
+ This is ignored if the :attr:`pipeline.trigger.<gerrit
+ source>.reject` attribute is present.
+
+ .. attr:: require
+
+ This may be used for any event. It describes conditions that
+ must be met by the change in order for the trigger event to
+ match. Those conditions may be satisfied by the event in
+ question. It follows the same syntax as
+ :ref:`gerrit_requirements`.
+
+ .. attr:: reject
+
+ This may be used for any event and is the mirror of
+ :attr:`pipeline.trigger.<gerrit source>.require`. It describes
+ conditions that when met by the change cause the trigger event
+ not to match. Those conditions may be satisfied by the event in
+ question. It follows the same syntax as
+ :ref:`gerrit_requirements`.
+
Reporter Configuration
----------------------
@@ -284,6 +315,8 @@ with an HTTP password, in which case the HTTP API is used.
A :ref:`connection<connections>` that uses the gerrit driver must be
supplied to the trigger.
+.. _gerrit_requirements:
+
Requirements Configuration
--------------------------
@@ -366,7 +399,7 @@ order to be enqueued into the pipeline.
.. attr:: status
A string value that corresponds with the status of the change
- reported by the trigger.
+ reported by Gerrit.
.. attr:: pipeline.reject.<gerrit source>
@@ -376,10 +409,12 @@ order to be enqueued into the pipeline.
.. attr:: approval
- This takes an approval or 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
- :attr:`pipeline.require.<gerrit source>.approval`.
+ This requires that a certain kind of approval not be present for the
+ current patchset of the change (the approval could be added by
+ the event in question). Approval is a dictionary or a list of
+ dictionaries with attributes listed below, all of which are
+ optional and are combined together so that there must be no approvals
+ matching all specified requirements.
Example to reject a change with any negative vote:
@@ -390,6 +425,55 @@ order to be enqueued into the pipeline.
approval:
- Code-Review: [-1, -2]
+ .. attr:: username
+
+ If present, an approval from this username is required. It is
+ treated as a regular expression.
+
+ .. attr:: email
+
+ If present, an approval with this email address is required. It is
+ treated as a regular expression.
+
+ .. attr:: older-than
+
+ If present, the approval must be older than this amount of time
+ to match. Provide a time interval as a number with a suffix of
+ "w" (weeks), "d" (days), "h" (hours), "m" (minutes), "s"
+ (seconds). Example ``48h`` or ``2d``.
+
+ .. attr:: newer-than
+
+ If present, the approval must be newer than this amount
+ of time to match. Same format as "older-than".
+
+ Any other field is interpreted as a review category and value
+ pair. For example ``Verified: 1`` would require that the
+ approval be for a +1 vote in the "Verified" column. The value
+ may either be a single value or a list: ``Verified: [1, 2]``
+ would match either a +1 or +2 vote.
+
+ .. attr:: open
+
+ A boolean value (``true`` or ``false``) that indicates whether
+ the change must be open or closed in order to be rejected.
+
+ .. attr:: current-patchset
+
+ A boolean value (``true`` or ``false``) that indicates whether the
+ change must be the current patchset in order to be rejected.
+
+ .. attr:: wip
+
+ A boolean value (``true`` or ``false``) that indicates whether the
+ change must be wip or not wip in order to be rejected.
+
+ .. attr:: status
+
+ A string value that corresponds with the status of the change
+ reported by Gerrit.
+
+
Reference Pipelines Configuration
---------------------------------
diff --git a/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml b/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml
new file mode 100644
index 000000000..479d6c153
--- /dev/null
+++ b/releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml
@@ -0,0 +1,22 @@
+---
+features:
+ - |
+ Gerrit pipeline triggers now support embedded require and reject
+ filters in order to match. Any conditions set for the pipeline in
+ require or reject filters may also be set for event trigger
+ filters.
+
+ This can be used to construct pipelines which trigger based on
+ certain events but only if certain other conditions are met. It
+ is distinct from pipeline requirements in that it only affects
+ items that are directly enqueued whereas pipeline requirements
+ affect dependencies as well.
+ - |
+ All Gerrit "requires" filters are now available as "reject"
+ filters as well.
+deprecations:
+ - |
+ The `require-approval` and `reject-approval` Gerrit trigger
+ attributes are deprecated. Use :attr:`pipeline.trigger.<gerrit
+ source>.require` and :attr:`pipeline.trigger.<gerrit
+ source>.reject` instead.
diff --git a/tests/base.py b/tests/base.py
index c031497a2..af10ebe96 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -950,6 +950,7 @@ class FakeGerritChange(object):
if self.fail_merge:
return
self.data['status'] = 'MERGED'
+ self.data['open'] = False
self.open = False
path = os.path.join(self.upstream_root, self.project)
diff --git a/tests/fixtures/layouts/gerrit-trigger-requirements.yaml b/tests/fixtures/layouts/gerrit-trigger-requirements.yaml
new file mode 100644
index 000000000..72ad6b41e
--- /dev/null
+++ b/tests/fixtures/layouts/gerrit-trigger-requirements.yaml
@@ -0,0 +1,162 @@
+- pipeline:
+ name: require-open
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test require-open
+ require:
+ open: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: reject-open
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test reject-open
+ reject:
+ open: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: require-wip
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test require-wip
+ require:
+ wip: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: reject-wip
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test reject-wip
+ reject:
+ wip: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: require-current-patchset
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test require-current-patchset
+ require:
+ current-patchset: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: reject-current-patchset
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test reject-current-patchset
+ reject:
+ current-patchset: true
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: require-status
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test require-status
+ require:
+ status: MERGED
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: reject-status
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test reject-status
+ reject:
+ status: MERGED
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: require-approval
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test require-approval
+ require:
+ approval:
+ username: zuul
+ Verified: 1
+ success:
+ gerrit:
+ Verified: 1
+
+- pipeline:
+ name: reject-approval
+ manager: independent
+ trigger:
+ gerrit:
+ - event: comment-added
+ comment: test reject-approval
+ reject:
+ approval:
+ username: zuul
+ Verified: 1
+ success:
+ gerrit:
+ Verified: 1
+
+- job:
+ name: base
+ parent: null
+ run: playbooks/base.yaml
+
+- job: {name: require-open}
+- job: {name: reject-open}
+- job: {name: require-wip}
+- job: {name: reject-wip}
+- job: {name: require-current-patchset}
+- job: {name: reject-current-patchset}
+- job: {name: require-status}
+- job: {name: reject-status}
+- job: {name: require-approval}
+- job: {name: reject-approval}
+
+- project:
+ name: org/project
+ require-open: {jobs: [require-open]}
+ reject-open: {jobs: [reject-open]}
+ require-wip: {jobs: [require-wip]}
+ reject-wip: {jobs: [reject-wip]}
+ require-current-patchset: {jobs: [require-current-patchset]}
+ reject-current-patchset: {jobs: [reject-current-patchset]}
+ require-status: {jobs: [require-status]}
+ reject-status: {jobs: [reject-status]}
+ require-approval: {jobs: [require-approval]}
+ reject-approval: {jobs: [reject-approval]}
diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py
index 9f3b87187..c5dca56cd 100644
--- a/tests/unit/test_requirements.py
+++ b/tests/unit/test_requirements.py
@@ -14,7 +14,7 @@
import time
-from tests.base import ZuulTestCase
+from tests.base import ZuulTestCase, simple_layout
class TestRequirementsApprovalNewerThan(ZuulTestCase):
@@ -490,3 +490,222 @@ class TestRequirementsTrustedCheck(ZuulTestCase):
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([])
+
+
+class TestGerritTriggerRequirements(ZuulTestCase):
+ scheduler_count = 1
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_require_open(self):
+ # Test trigger require-open
+ jobname = 'require-open'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's open, so it should be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ # Not open, so should be ignored
+ A.setMerged()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_reject_open(self):
+ # Test trigger reject-open
+ jobname = 'reject-open'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's open, so it should not be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Not open, so should be enqueued
+ A.setMerged()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_require_wip(self):
+ # Test trigger require-wip
+ jobname = 'require-wip'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's not WIP, so it should be ignored
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # WIP, so should be enqueued
+ A.setWorkInProgress(True)
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_reject_wip(self):
+ # Test trigger reject-wip
+ jobname = 'reject-wip'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's not WIP, so it should be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ # WIP, so should be ignored
+ A.setWorkInProgress(True)
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_require_current_patchset(self):
+ # Test trigger require-current_patchset
+ jobname = 'require-current-patchset'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's current, so it should be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ # Not current, so should be ignored
+ A.addPatchset()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_reject_current_patchset(self):
+ # Test trigger reject-current_patchset
+ jobname = 'reject-current-patchset'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's current, so it should be ignored
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Not current, so should be enqueued
+ A.addPatchset()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_require_status(self):
+ # Test trigger require-status
+ jobname = 'require-status'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's not merged, so it should be ignored
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Merged, so should be enqueued
+ A.setMerged()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_reject_status(self):
+ # Test trigger reject-status
+ jobname = 'reject-status'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # It's not merged, so it should be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ # Merged, so should be ignored
+ A.setMerged()
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_require_approval(self):
+ # Test trigger require-approval
+ jobname = 'require-approval'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # Missing approval, so it should be ignored
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Has approval, so it should be enqueued
+ A.addApproval('Verified', 1, username='zuul')
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ @simple_layout('layouts/gerrit-trigger-requirements.yaml')
+ def test_reject_approval(self):
+ # Test trigger reject-approval
+ jobname = 'reject-approval'
+ project = 'org/project'
+ A = self.fake_gerrit.addFakeChange(project, 'master', 'A')
+ # A comment event that we will keep submitting to trigger
+ comment = A.getChangeCommentEvent(1, f'test {jobname}')
+
+ # Missing approval, so it should be enqueued
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
+
+ # Has approval, so it should be ignored
+ A.addApproval('Verified', 1, username='zuul')
+ self.fake_gerrit.addEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, jobname)
diff --git a/zuul/driver/gerrit/gerritmodel.py b/zuul/driver/gerrit/gerritmodel.py
index 4ac291f2b..7b57ec934 100644
--- a/zuul/driver/gerrit/gerritmodel.py
+++ b/zuul/driver/gerrit/gerritmodel.py
@@ -19,8 +19,8 @@ import urllib.parse
import dateutil.parser
from zuul.model import EventFilter, RefFilter
-from zuul.model import Change, TriggerEvent
-from zuul.driver.util import time_to_seconds
+from zuul.model import Change, TriggerEvent, FalseWithReason
+from zuul.driver.util import time_to_seconds, to_list
from zuul import exceptions
EMPTY_GIT_REF = '0' * 40 # git sha of all zeros, used during creates/deletes
@@ -247,110 +247,32 @@ class GerritTriggerEvent(TriggerEvent):
return 'change-abandoned' == self.type
-class GerritApprovalFilter(object):
- def __init__(self, required_approvals=[], reject_approvals=[]):
- self._required_approvals = copy.deepcopy(required_approvals)
- self.required_approvals = self._tidy_approvals(
- self._required_approvals)
- self._reject_approvals = copy.deepcopy(reject_approvals)
- self.reject_approvals = self._tidy_approvals(self._reject_approvals)
-
- def _tidy_approvals(self, approvals):
- for a in approvals:
- for k, v in a.items():
- if k == 'username':
- a['username'] = re.compile(v)
- elif k == 'email':
- a['email'] = re.compile(v)
- elif k == 'newer-than':
- a[k] = time_to_seconds(v)
- elif k == 'older-than':
- a[k] = time_to_seconds(v)
- 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 (not v.search(by.get('username', ''))):
- 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 (approval['description'] != k or
- int(approval['value']) not in v):
- return False
- return True
-
- def matchesApprovals(self, change):
- if self.required_approvals or self.reject_approvals:
- if not hasattr(change, 'number'):
- # Not a change, no reviews
- return False
- if self.required_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):
- # Check if any approvals match the requirements
- for rapproval in self.required_approvals:
- matches_rapproval = False
- for approval in change.approvals:
- 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_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 GerritEventFilter(EventFilter, GerritApprovalFilter):
+class GerritEventFilter(EventFilter):
def __init__(self, connection_name, trigger, types=[], branches=[],
refs=[], event_approvals={}, comments=[], emails=[],
usernames=[], required_approvals=[], reject_approvals=[],
- uuid=None, scheme=None, ignore_deletes=True):
+ uuid=None, scheme=None, ignore_deletes=True,
+ require=None, reject=None):
EventFilter.__init__(self, connection_name, trigger)
- GerritApprovalFilter.__init__(self,
- required_approvals=required_approvals,
- reject_approvals=reject_approvals)
+ # TODO: Backwards compat, remove after 9.x:
+ if required_approvals and require is None:
+ require = {'approval': required_approvals}
+ if reject_approvals and reject is None:
+ reject = {'approval': reject_approvals}
+
+ if require:
+ self.require_filter = GerritRefFilter.requiresFromConfig(
+ connection_name, require)
+ else:
+ self.require_filter = None
+
+ if reject:
+ self.reject_filter = GerritRefFilter.rejectFromConfig(
+ connection_name, reject)
+ else:
+ self.reject_filter = None
self._types = types
self._branches = branches
@@ -388,18 +310,16 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
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.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:
ret += ' emails: %s' % ', '.join(self._emails)
if self._usernames:
ret += ' usernames: %s' % ', '.join(self._usernames)
+ if self.require_filter:
+ ret += ' require: %s' % repr(self.require_filter)
+ if self.reject_filter:
+ ret += ' reject: %s' % repr(self.reject_filter)
ret += '>'
return ret
@@ -414,7 +334,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
if etype.match(event.type):
matches_type = True
if self.types and not matches_type:
- return False
+ return FalseWithReason("Types %s do not match %s" % (
+ self.types, event.type))
if event.type == 'pending-check':
if self.uuid and event.uuid != self.uuid:
@@ -428,7 +349,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
if branch.match(event.branch):
matches_branch = True
if self.branches and not matches_branch:
- return False
+ return FalseWithReason("Branches %s do not match %s" % (
+ self.branches, event.branch))
# refs are ORed
matches_ref = False
@@ -437,11 +359,12 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
if ref.match(event.ref):
matches_ref = True
if self.refs and not matches_ref:
- return False
+ return FalseWithReason(
+ "Refs %s do not match %s" % (self.refs, event.ref))
if self.ignore_deletes and event.newrev == EMPTY_GIT_REF:
# If the updated ref has an empty git sha (all 0s),
# then the ref is being deleted
- return False
+ return FalseWithReason("Ref deletion events are ignored")
# comments are ORed
matches_comment_re = False
@@ -455,7 +378,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
comment_re.search(comment)):
matches_comment_re = True
if self.comments and not matches_comment_re:
- return False
+ return FalseWithReason("Comments %s do not match %s" % (
+ self.comments, event.patchsetcomments))
# We better have an account provided by Gerrit to do
# email filtering.
@@ -468,7 +392,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
email_re.search(account_email)):
matches_email_re = True
if self.emails and not matches_email_re:
- return False
+ return FalseWithReason("Username %s does not match %s" % (
+ self.emails, account_email))
# usernames are ORed
account_username = event.account.get('username')
@@ -478,7 +403,8 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
username_re.search(account_username)):
matches_username_re = True
if self.usernames and not matches_username_re:
- return False
+ return FalseWithReason("Username %s does not match %s" % (
+ self.usernames, account_username))
# approvals are ANDed
for category, value in self.event_approvals.items():
@@ -488,29 +414,73 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
int(eapp['value']) == int(value)):
matches_approval = True
if not matches_approval:
- return False
+ return FalseWithReason("Approvals %s do not match %s" % (
+ self.event_approvals, event.approvals))
- # required approvals are ANDed (reject approvals are ORed)
- if not self.matchesApprovals(change):
- return False
+ if self.require_filter:
+ require_filter_result = self.require_filter.matches(change)
+ if not require_filter_result:
+ return require_filter_result
+
+ if self.reject_filter:
+ reject_filter_result = self.reject_filter.matches(change)
+ if not reject_filter_result:
+ return reject_filter_result
return True
-class GerritRefFilter(RefFilter, GerritApprovalFilter):
- def __init__(self, connection_name, open=None, current_patchset=None,
- wip=None, statuses=[], required_approvals=[],
- reject_approvals=[]):
+class GerritRefFilter(RefFilter):
+ def __init__(self, connection_name,
+ open=None, reject_open=None,
+ current_patchset=None, reject_current_patchset=None,
+ wip=None, reject_wip=None,
+ statuses=[], reject_statuses=[],
+ required_approvals=[], reject_approvals=[]):
RefFilter.__init__(self, connection_name)
- GerritApprovalFilter.__init__(self,
- required_approvals=required_approvals,
- reject_approvals=reject_approvals)
-
- self.open = open
- self.wip = wip
- self.current_patchset = current_patchset
+ self._required_approvals = copy.deepcopy(required_approvals)
+ self.required_approvals = self._tidy_approvals(
+ self._required_approvals)
+ self._reject_approvals = copy.deepcopy(reject_approvals)
+ self.reject_approvals = self._tidy_approvals(self._reject_approvals)
self.statuses = statuses
+ self.reject_statuses = reject_statuses
+
+ if reject_open is not None:
+ self.open = not reject_open
+ else:
+ self.open = open
+ if reject_wip is not None:
+ self.wip = not reject_wip
+ else:
+ self.wip = wip
+ if reject_current_patchset is not None:
+ self.current_patchset = not reject_current_patchset
+ else:
+ self.current_patchset = current_patchset
+
+ @classmethod
+ def requiresFromConfig(cls, connection_name, config):
+ return cls(
+ connection_name=connection_name,
+ open=config.get('open'),
+ current_patchset=config.get('current-patchset'),
+ wip=config.get('wip'),
+ statuses=to_list(config.get('status')),
+ required_approvals=to_list(config.get('approval')),
+ )
+
+ @classmethod
+ def rejectFromConfig(cls, connection_name, config):
+ return cls(
+ connection_name=connection_name,
+ reject_open=config.get('open'),
+ reject_current_patchset=config.get('current-patchset'),
+ reject_wip=config.get('wip'),
+ reject_statuses=to_list(config.get('status')),
+ reject_approvals=to_list(config.get('approval')),
+ )
def __repr__(self):
ret = '<GerritRefFilter'
@@ -518,10 +488,14 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
ret += ' connection_name: %s' % self.connection_name
if self.open is not None:
ret += ' open: %s' % self.open
+ if self.wip is not None:
+ ret += ' wip: %s' % self.wip
if self.current_patchset is not None:
ret += ' current-patchset: %s' % self.current_patchset
if self.statuses:
ret += ' statuses: %s' % ', '.join(self.statuses)
+ if self.reject_statuses:
+ ret += ' reject-statuses: %s' % ', '.join(self.reject_statuses)
if self.required_approvals:
ret += (' required-approvals: %s' %
str(self.required_approvals))
@@ -533,37 +507,138 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
return ret
def matches(self, change):
+ if self.open is not None:
+ # if a "change" has no number, it's not a change, but a push
+ # and cannot possibly pass this test.
+ if hasattr(change, 'number'):
+ if self.open != change.open:
+ return FalseWithReason(
+ "Change does not match open requirement")
+ else:
+ return FalseWithReason("Ref is not a Change")
- filters = [
- {
- "required": self.open,
- "value": change.open
- },
- {
- "required": self.current_patchset,
- "value": change.is_current_patchset
- },
- {
- "required": self.wip,
- "value": change.wip
- },
- ]
- configured = filter(lambda x: x["required"] is not None, filters)
-
- # if a "change" has no number, it's not a change, but a push
- # and cannot possibly pass this test.
- if hasattr(change, 'number'):
- if any(map(lambda x: x["required"] != x["value"], configured)):
- return False
- elif configured:
- return False
+ if self.current_patchset is not None:
+ # if a "change" has no number, it's not a change, but a push
+ # and cannot possibly pass this test.
+ if hasattr(change, 'number'):
+ if self.current_patchset != change.is_current_patchset:
+ return FalseWithReason(
+ "Change does not match current patchset requirement")
+ else:
+ return FalseWithReason("Ref is not a Change")
+
+ if self.wip is not None:
+ # if a "change" has no number, it's not a change, but a push
+ # and cannot possibly pass this test.
+ if hasattr(change, 'number'):
+ if self.wip != change.wip:
+ return FalseWithReason(
+ "Change does not match WIP requirement")
+ else:
+ return FalseWithReason("Ref is not a Change")
if self.statuses:
if change.status not in self.statuses:
- return False
+ return FalseWithReason(
+ "Required statuses %s do not match %s" % (
+ self.statuses, change.status))
+ if self.reject_statuses:
+ if change.status in self.reject_statuses:
+ return FalseWithReason(
+ "Reject statuses %s match %s" % (
+ self.reject_statuses, change.status))
# required approvals are ANDed (reject approvals are ORed)
- if not self.matchesApprovals(change):
+ matches_approvals_result = self.matchesApprovals(change)
+ if not matches_approvals_result:
+ return matches_approvals_result
+
+ return True
+
+ def _tidy_approvals(self, approvals):
+ for a in approvals:
+ for k, v in a.items():
+ if k == 'username':
+ a['username'] = re.compile(v)
+ elif k == 'email':
+ a['email'] = re.compile(v)
+ elif k == 'newer-than':
+ a[k] = time_to_seconds(v)
+ elif k == 'older-than':
+ a[k] = time_to_seconds(v)
+ 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 (not v.search(by.get('username', ''))):
+ 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 (approval['description'] != k or
+ int(approval['value']) not in v):
+ return False
+ return True
+ def matchesApprovals(self, change):
+ if self.required_approvals or self.reject_approvals:
+ if not hasattr(change, 'number'):
+ # Not a change, no reviews
+ return FalseWithReason("Ref is not a Change")
+ if self.required_approvals and not change.approvals:
+ # A change with no approvals can not match
+ return FalseWithReason("Approvals %s does not match %s" % (
+ self.required_approvals, change.approvals))
+
+ # 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):
+ # Check if any approvals match the requirements
+ for rapproval in self.required_approvals:
+ matches_rapproval = False
+ for approval in change.approvals:
+ 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_rapproval:
+ return FalseWithReason(
+ "Required approvals %s do not match %s" % (
+ self.required_approvals, change.approvals))
+ 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 FalseWithReason("Reject approvals %s match %s" % (
+ self.reject_approvals, change.approvals))
+ # To get here no rejects can have been matched so we should be good to
+ # queue
return True
diff --git a/zuul/driver/gerrit/gerritsource.py b/zuul/driver/gerrit/gerritsource.py
index b0bd3c448..4e7a32b83 100644
--- a/zuul/driver/gerrit/gerritsource.py
+++ b/zuul/driver/gerrit/gerritsource.py
@@ -20,7 +20,7 @@ from urllib.parse import urlparse
from zuul.source import BaseSource
from zuul.model import Project
from zuul.driver.gerrit.gerritmodel import GerritRefFilter
-from zuul.driver.util import scalar_or_list, to_list
+from zuul.driver.util import scalar_or_list
from zuul.lib.dependson import find_dependency_headers
from zuul.zk.change_cache import ChangeKey
@@ -209,21 +209,15 @@ class GerritSource(BaseSource):
return self.connection._getGitwebUrl(project, sha)
def getRequireFilters(self, config):
- f = GerritRefFilter(
- connection_name=self.connection.connection_name,
- open=config.get('open'),
- current_patchset=config.get('current-patchset'),
- wip=config.get('wip'),
- statuses=to_list(config.get('status')),
- required_approvals=to_list(config.get('approval')),
- )
+ f = GerritRefFilter.requiresFromConfig(
+ self.connection.connection_name,
+ config)
return [f]
def getRejectFilters(self, config):
- f = GerritRefFilter(
- connection_name=self.connection.connection_name,
- reject_approvals=to_list(config.get('approval')),
- )
+ f = GerritRefFilter.rejectFromConfig(
+ self.connection.connection_name,
+ config)
return [f]
def getRefForChange(self, change):
@@ -247,11 +241,13 @@ def getRequireSchema():
'current-patchset': bool,
'wip': bool,
'status': scalar_or_list(str)}
-
return require
def getRejectSchema():
- reject = {'approval': scalar_or_list(approval)}
-
+ reject = {'approval': scalar_or_list(approval),
+ 'open': bool,
+ 'current-patchset': bool,
+ 'wip': bool,
+ 'status': scalar_or_list(str)}
return reject
diff --git a/zuul/driver/gerrit/gerrittrigger.py b/zuul/driver/gerrit/gerrittrigger.py
index dc8a7db68..dff5dc32c 100644
--- a/zuul/driver/gerrit/gerrittrigger.py
+++ b/zuul/driver/gerrit/gerrittrigger.py
@@ -16,6 +16,7 @@ import logging
import voluptuous as v
from zuul.trigger import BaseTrigger
from zuul.driver.gerrit.gerritmodel import GerritEventFilter
+from zuul.driver.gerrit import gerritsource
from zuul.driver.util import scalar_or_list, to_list
@@ -59,7 +60,9 @@ class GerritTrigger(BaseTrigger):
),
uuid=trigger.get('uuid'),
scheme=trigger.get('scheme'),
- ignore_deletes=ignore_deletes
+ ignore_deletes=ignore_deletes,
+ require=trigger.get('require'),
+ reject=trigger.get('reject'),
)
efilters.append(f)
@@ -101,6 +104,8 @@ def getSchema():
'approval': scalar_or_list(variable_dict),
'require-approval': scalar_or_list(approval),
'reject-approval': scalar_or_list(approval),
+ 'require': gerritsource.getRequireSchema(),
+ 'reject': gerritsource.getRejectSchema(),
}
return gerrit_trigger
diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py
index 536cce903..71238d070 100644
--- a/zuul/driver/github/githubmodel.py
+++ b/zuul/driver/github/githubmodel.py
@@ -255,7 +255,7 @@ class GithubEventFilter(EventFilter):
if etype.match(event.type):
matches_type = True
if self.types and not matches_type:
- return FalseWithReason("Types %s doesn't match %s" % (
+ return FalseWithReason("Types %s do not match %s" % (
self.types, event.type))
# branches are ORed
@@ -264,7 +264,7 @@ class GithubEventFilter(EventFilter):
if branch.match(event.branch):
matches_branch = True
if self.branches and not matches_branch:
- return FalseWithReason("Branches %s doesn't match %s" % (
+ return FalseWithReason("Branches %s do not match %s" % (
self.branches, event.branch))
# refs are ORed
@@ -275,11 +275,11 @@ class GithubEventFilter(EventFilter):
matches_ref = True
if self.refs and not matches_ref:
return FalseWithReason(
- "Refs %s doesn't match %s" % (self.refs, event.ref))
+ "Refs %s do not match %s" % (self.refs, event.ref))
if self.ignore_deletes and event.newrev == EMPTY_GIT_REF:
# If the updated ref has an empty git sha (all 0s),
# then the ref is being deleted
- return FalseWithReason("Ref deletion are ignored")
+ return FalseWithReason("Ref deletion events are ignored")
# comments are ORed
matches_comment_re = False
@@ -288,7 +288,7 @@ class GithubEventFilter(EventFilter):
comment_re.search(event.comment)):
matches_comment_re = True
if self.comments and not matches_comment_re:
- return FalseWithReason("Comments %s doesn't match %s" % (
+ return FalseWithReason("Comments %s do not match %s" % (
self.comments, event.comment))
# actions are ORed
@@ -297,7 +297,7 @@ class GithubEventFilter(EventFilter):
if (event.action == action):
matches_action = True
if self.actions and not matches_action:
- return FalseWithReason("Actions %s doesn't match %s" % (
+ return FalseWithReason("Actions %s do not match %s" % (
self.actions, event.action))
# check_runs are ORed
@@ -308,22 +308,22 @@ class GithubEventFilter(EventFilter):
check_run_found = True
break
if not check_run_found:
- return FalseWithReason("Check_runs %s doesn't match %s" % (
+ return FalseWithReason("Check runs %s do not match %s" % (
self.check_runs, event.check_run))
# labels are ORed
if self.labels and event.label not in self.labels:
- return FalseWithReason("Labels %s doesn't match %s" % (
+ return FalseWithReason("Labels %s do not match %s" % (
self.labels, event.label))
# unlabels are ORed
if self.unlabels and event.unlabel not in self.unlabels:
- return FalseWithReason("Unlabels %s doesn't match %s" % (
+ return FalseWithReason("Unlabels %s do not match %s" % (
self.unlabels, event.unlabel))
# states are ORed
if self.states and event.state not in self.states:
- return FalseWithReason("States %s doesn't match %s" % (
+ return FalseWithReason("States %s do not match %s" % (
self.states, event.state))
# statuses are ORed
@@ -334,7 +334,7 @@ class GithubEventFilter(EventFilter):
status_found = True
break
if not status_found:
- return FalseWithReason("Statuses %s doesn't match %s" % (
+ return FalseWithReason("Statuses %s do not match %s" % (
self.statuses, event.status))
if self.require_filter:
@@ -498,7 +498,7 @@ class GithubRefFilter(RefFilter):
if self.required_reviews and not change.reviews:
# No reviews means no matching of required bits
# having reject reviews but no reviews on the change is okay
- return FalseWithReason("Reviews %s does not match %s" % (
+ return FalseWithReason("Reviews %s do not match %s" % (
self.required_reviews, change.reviews))
return (self.matchesRequiredReviews(change) and
@@ -514,7 +514,7 @@ class GithubRefFilter(RefFilter):
break
if not matches_review:
return FalseWithReason(
- "Required reviews %s does not match %s" % (
+ "Required reviews %s do not match %s" % (
self.required_reviews, change.reviews))
return True
@@ -523,7 +523,7 @@ class GithubRefFilter(RefFilter):
for review in change.reviews:
if self._match_review_required_review(rreview, review):
# A review matched, we can reject right away
- return FalseWithReason("Reject reviews %s matches %s" % (
+ return FalseWithReason("Reject reviews %s match %s" % (
self.reject_reviews, change.reviews))
return True
@@ -531,10 +531,10 @@ class GithubRefFilter(RefFilter):
if self.required_statuses or self.reject_statuses:
if not hasattr(change, 'number'):
# not a PR, no status
- return FalseWithReason("Can't match statuses without PR")
+ return FalseWithReason("Can not match statuses without PR")
if self.required_statuses and not change.status:
return FalseWithReason(
- "Required statuses %s does not match %s" % (
+ "Required statuses %s do not match %s" % (
self.required_statuses, change.status))
required_statuses_results = self.matchesRequiredStatuses(change)
if not required_statuses_results:
@@ -551,7 +551,7 @@ class GithubRefFilter(RefFilter):
for status in change.status:
if re2.fullmatch(required_status, status):
return True
- return FalseWithReason("RequiredStatuses %s does not match %s" % (
+ return FalseWithReason("Required statuses %s do not match %s" % (
self.required_statuses, change.status))
return True
@@ -561,7 +561,7 @@ class GithubRefFilter(RefFilter):
for rstatus in self.reject_statuses:
for status in change.status:
if re2.fullmatch(rstatus, status):
- return FalseWithReason("NoRejectStatuses %s matches %s" % (
+ return FalseWithReason("Reject statuses %s match %s" % (
self.reject_statuses, change.status))
return True
@@ -569,7 +569,7 @@ class GithubRefFilter(RefFilter):
if self.required_labels or self.reject_labels:
if not hasattr(change, 'number'):
# not a PR, no label
- return FalseWithReason("Can't match labels without PR")
+ return FalseWithReason("Can not match labels without PR")
if self.required_labels and not change.labels:
# No labels means no matching of required bits
# having reject labels but no labels on the change is okay
@@ -582,14 +582,14 @@ class GithubRefFilter(RefFilter):
def matchesRequiredLabels(self, change):
for label in self.required_labels:
if label not in change.labels:
- return FalseWithReason("Labels %s does not match %s" % (
+ return FalseWithReason("Labels %s do not match %s" % (
self.required_labels, change.labels))
return True
def matchesNoRejectLabels(self, change):
for label in self.reject_labels:
if label in change.labels:
- return FalseWithReason("NoRejectLabels %s matches %s" % (
+ return FalseWithReason("Reject labels %s match %s" % (
self.reject_labels, change.labels))
return True