diff options
| -rw-r--r-- | doc/source/drivers/gerrit.rst | 94 | ||||
| -rw-r--r-- | releasenotes/notes/gerrit-trigger-status-88cb4c52bd3ba86a.yaml | 22 | ||||
| -rw-r--r-- | tests/base.py | 1 | ||||
| -rw-r--r-- | tests/fixtures/layouts/gerrit-trigger-requirements.yaml | 162 | ||||
| -rw-r--r-- | tests/unit/test_requirements.py | 221 | ||||
| -rw-r--r-- | zuul/driver/gerrit/gerritmodel.py | 381 | ||||
| -rw-r--r-- | zuul/driver/gerrit/gerritsource.py | 28 | ||||
| -rw-r--r-- | zuul/driver/gerrit/gerrittrigger.py | 7 | ||||
| -rw-r--r-- | zuul/driver/github/githubmodel.py | 42 |
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 |