From c053d0268cd1355fcb86dac621a7658ee8d3b814 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 22 Jan 2014 14:57:33 -0800 Subject: [PATCH] Add require-approval to Gerrit trigger This feature allows Zuul to consider existing (or new) approval votes associated with a change when determining whether an event matches. For example, it can be used to require that a Verified vote of a certain age be present before a change is enqueued in a pipeline. Change-Id: I81344713d71b345b08576334568b9c49c810c7e9 --- doc/source/zuul.rst | 29 +++++ tests/fixtures/layout-require-approval.yaml | 58 ++++++++++ .../layouts/good_require_approvals.yaml | 36 +++++++ tests/test_scheduler.py | 102 +++++++++++++++++- zuul/layoutvalidator.py | 7 ++ zuul/model.py | 74 +++++++++++-- zuul/scheduler.py | 12 ++- zuul/trigger/gerrit.py | 2 + 8 files changed, 305 insertions(+), 15 deletions(-) create mode 100644 tests/fixtures/layout-require-approval.yaml create mode 100644 tests/fixtures/layouts/good_require_approvals.yaml diff --git a/doc/source/zuul.rst b/doc/source/zuul.rst index f71df22a90..b4adc4da6e 100644 --- a/doc/source/zuul.rst +++ b/doc/source/zuul.rst @@ -348,6 +348,35 @@ explanation of each of the parameters:: containing 'retrigger' somewhere in the comment text are added to a change. + *require-approval* + This may be used for any event. It requires that a certain kind + of approval be present for the current patchset of the change (the + approval could be added by the event in question). It takes + several sub-parameters, all of which are optional and are combined + together so that there must be an approval matching all specified + requirements. + + *username* + If present, an approval from this username is required. + + *email-filter* + If present, an approval with this email address is required. It + is treated as a regular expression as above. + + *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". + + *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. + **timer** This trigger will run based on a cron-style time specification. It will enqueue an event into its pipeline for every project diff --git a/tests/fixtures/layout-require-approval.yaml b/tests/fixtures/layout-require-approval.yaml new file mode 100644 index 0000000000..18eee99a84 --- /dev/null +++ b/tests/fixtures/layout-require-approval.yaml @@ -0,0 +1,58 @@ +includes: + - python-file: custom_functions.py + +pipelines: + - name: check + manager: IndependentPipelineManager + trigger: + gerrit: + - event: patchset-created + - event: comment-added + require-approval: + - email-filter: jenkins@example.com + older-than: 48h + success: + gerrit: + verified: 1 + failure: + gerrit: + verified: -1 + + - name: gate + manager: DependentPipelineManager + failure-message: Build failed. For information on how to proceed, see http://wiki.example.org/Test_Failures + trigger: + gerrit: + - event: comment-added + require-approval: + - verified: 1 + username: jenkins + newer-than: 48h + approval: + - approved: 1 + - event: comment-added + require-approval: + - verified: 1 + username: jenkins + newer-than: 48h + approval: + - verified: 1 + success: + gerrit: + verified: 2 + submit: true + failure: + gerrit: + verified: -2 + start: + gerrit: + verified: 0 + precedence: high + +projects: + - name: org/project + merge-mode: cherry-pick + check: + - project-check + gate: + - project-gate diff --git a/tests/fixtures/layouts/good_require_approvals.yaml b/tests/fixtures/layouts/good_require_approvals.yaml new file mode 100644 index 0000000000..b7993b0927 --- /dev/null +++ b/tests/fixtures/layouts/good_require_approvals.yaml @@ -0,0 +1,36 @@ +includes: + - python-file: custom_functions.py + +pipelines: + - name: check + manager: IndependentPipelineManager + trigger: + gerrit: + - event: comment-added + require-approval: + - username: jenkins + older-than: 48h + - event: comment-added + require-approval: + - email-filter: jenkins@example.com + newer-than: 48h + - event: comment-added + require-approval: + - approved: 1 + - event: comment-added + require-approval: + - approved: 1 + username: jenkins + email-filter: jenkins@example.com + success: + gerrit: + verified: 1 + failure: + gerrit: + verified: -1 + +projects: + - name: org/project + merge-mode: cherry-pick + check: + - project-check diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 0ce0f88c60..3b7b9f0863 100755 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -214,10 +214,19 @@ class FakeChange(object): "reason": ""} return event - def addApproval(self, category, value): + def addApproval(self, category, value, username='jenkins', + granted_on=None): + if not granted_on: + granted_on = time.time() approval = {'description': self.categories[category][0], 'type': category, - 'value': str(value)} + 'value': str(value), + 'username': username, + 'email': username + '@example.com', + 'grantedOn': int(granted_on)} + for i, x in enumerate(self.patchsets[-1]['approvals'][:]): + if x['username'] == username and x['type'] == category: + del self.patchsets[-1]['approvals'][i] self.patchsets[-1]['approvals'].append(approval) event = {'approvals': [approval], 'author': {'email': 'user@example.com', @@ -2658,6 +2667,95 @@ class TestScheduler(testtools.TestCase): self.assertEqual(D.data['status'], 'MERGED') self.assertEqual(D.reported, 2) + def test_required_approval_check_and_gate(self): + "Test required-approval triggers both check and gate" + self.config.set('zuul', 'layout_config', + 'tests/fixtures/layout-require-approval.yaml') + self.sched.reconfigure(self.config) + self.registerJobs() + + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + A.addApproval('CRVW', 2) + # Add a too-old +1 + A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60) + + aprv = A.addApproval('APRV', 1) + self.fake_gerrit.addEvent(aprv) + self.waitUntilSettled() + # Should have run a check job + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project-check') + + # Report the result of that check job (overrides previous vrfy) + # Skynet alert: this should trigger a gate job now that + # all reqs are met + self.fake_gerrit.addEvent(A.addApproval('VRFY', 1)) + self.waitUntilSettled() + self.assertEqual(len(self.history), 2) + self.assertEqual(self.history[1].name, 'project-gate') + + def test_required_approval_newer(self): + "Test required-approval newer trigger parameter" + self.config.set('zuul', 'layout_config', + 'tests/fixtures/layout-require-approval.yaml') + self.sched.reconfigure(self.config) + self.registerJobs() + + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + A.addApproval('CRVW', 2) + aprv = A.addApproval('APRV', 1) + self.fake_gerrit.addEvent(aprv) + self.waitUntilSettled() + # No +1 from Jenkins so should not be enqueued + self.assertEqual(len(self.history), 0) + + # Add a too-old +1, should trigger check but not gate + A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60) + self.fake_gerrit.addEvent(aprv) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project-check') + + # Add a recent +1 + self.fake_gerrit.addEvent(A.addApproval('VRFY', 1)) + self.fake_gerrit.addEvent(aprv) + self.waitUntilSettled() + self.assertEqual(len(self.history), 2) + self.assertEqual(self.history[1].name, 'project-gate') + + def test_required_approval_older(self): + "Test required-approval older trigger parameter" + self.config.set('zuul', 'layout_config', + 'tests/fixtures/layout-require-approval.yaml') + self.sched.reconfigure(self.config) + self.registerJobs() + + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + crvw = A.addApproval('CRVW', 2) + self.fake_gerrit.addEvent(crvw) + self.waitUntilSettled() + # No +1 from Jenkins so should not be enqueued + self.assertEqual(len(self.history), 0) + + # Add an old +1 and trigger check with a comment + A.addApproval('VRFY', 1, granted_on=time.time() - 72 * 60 * 60) + self.fake_gerrit.addEvent(crvw) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project-check') + + # Add a recent +1 and make sure nothing changes + A.addApproval('VRFY', 1) + self.fake_gerrit.addEvent(crvw) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + + # The last thing we did was query a change then do nothing + # with a pipeline, so it will be in the cache; clean it up so + # it does not fail the test. + for pipeline in self.sched.layout.pipelines.values(): + pipeline.trigger.maintainCache([]) + def test_rerun_on_error(self): "Test that if a worker fails to run a job, it is run again" self.worker.hold_jobs_in_build = True diff --git a/zuul/layoutvalidator.py b/zuul/layoutvalidator.py index 847bce703f..48aab03490 100644 --- a/zuul/layoutvalidator.py +++ b/zuul/layoutvalidator.py @@ -35,6 +35,12 @@ class LayoutSchema(object): variable_dict = v.Schema({}, extra=True) + require_approval = v.Schema({'username': str, + 'email-filter': str, + 'older-than': str, + 'newer-than': str, + }, extra=True) + gerrit_trigger = {v.Required('event'): toList(v.Any('patchset-created', 'change-abandoned', @@ -48,6 +54,7 @@ class LayoutSchema(object): 'branch': toList(str), 'ref': toList(str), 'approval': toList(variable_dict), + 'require-approval': toList(require_approval), } timer_trigger = {v.Required('time'): str} diff --git a/zuul/model.py b/zuul/model.py index cc1817faff..8b76ff4bc8 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -43,6 +43,20 @@ PRECEDENCE_MAP = { } +def time_to_seconds(s): + if s.endswith('s'): + return int(s[:-1]) + if s.endswith('m'): + return int(s[:-1]) * 60 + if s.endswith('h'): + return int(s[:-1]) * 60 * 60 + if s.endswith('d'): + return int(s[:-1]) * 24 * 60 * 60 + if s.endswith('w'): + return int(s[:-1]) * 7 * 24 * 60 * 60 + raise Exception("Unable to parse time value: %s" % s) + + class Pipeline(object): """A top-level pipeline such as check, gate, post, etc.""" def __init__(self, name): @@ -735,6 +749,7 @@ class Change(Changeish): self.can_merge = False self.is_merged = False self.failed_to_merge = False + self.approvals = [] def _id(self): return '%s,%s' % (self.number, self.patchset) @@ -873,9 +888,9 @@ class TriggerEvent(object): class EventFilter(object): - def __init__(self, types=[], branches=[], refs=[], approvals={}, + def __init__(self, types=[], branches=[], refs=[], event_approvals={}, comment_filters=[], email_filters=[], username_filters=[], - timespecs=[]): + timespecs=[], require_approvals=[]): self._types = types self._branches = branches self._refs = refs @@ -888,9 +903,18 @@ class EventFilter(object): self.comment_filters = [re.compile(x) for x in comment_filters] self.email_filters = [re.compile(x) for x in email_filters] self.username_filters = [re.compile(x) for x in username_filters] - self.approvals = approvals + self.event_approvals = event_approvals + self.require_approvals = require_approvals self.timespecs = timespecs + for a in self.require_approvals: + if 'older-than' in a: + a['older-than'] = time_to_seconds(a['older-than']) + if 'newer-than' in a: + a['newer-than'] = time_to_seconds(a['newer-than']) + if 'email-filter' in a: + a['email-filter'] = re.compile(a['email-filter']) + def __repr__(self): ret = '= t): + found_approval = False + else: + if (normalizeCategory(approval['description']) != k or + int(approval['value']) != v): + found_approval = False + if found_approval: + matches_approval = True + break + if not matches_approval: + return False + # timespecs are ORed matches_timespec = False for timespec in self.timespecs: diff --git a/zuul/scheduler.py b/zuul/scheduler.py index f9d8085064..213c23a587 100644 --- a/zuul/scheduler.py +++ b/zuul/scheduler.py @@ -227,13 +227,15 @@ class Scheduler(threading.Thread): f = EventFilter(types=toList(trigger['event']), branches=toList(trigger.get('branch')), refs=toList(trigger.get('ref')), - approvals=approvals, + event_approvals=approvals, comment_filters= toList(trigger.get('comment_filter')), email_filters= toList(trigger.get('email_filter')), username_filters= - toList(trigger.get('username_filter'))) + toList(trigger.get('username_filter')), + require_approvals= + toList(trigger.get('require-approval'))) manager.event_filters.append(f) elif 'timer' in conf_pipeline['trigger']: pipeline.trigger = self.triggers['timer'] @@ -723,7 +725,7 @@ class Scheduler(threading.Thread): self.triggers.get(event.trigger_name)) if event.type == 'patchset-created': pipeline.manager.removeOldVersionsOfChange(change) - if pipeline.manager.eventMatches(event): + if pipeline.manager.eventMatches(event, change): self.log.info("Adding %s, %s to %s" % (project, change, pipeline)) pipeline.manager.addChange(change) @@ -881,14 +883,14 @@ class BasePipelineManager(object): allow_needs.update(action_reporter.getSubmitAllowNeeds()) return allow_needs - def eventMatches(self, event): + def eventMatches(self, event, change): if event.forced_pipeline: if event.forced_pipeline == self.pipeline.name: return True else: return False for ef in self.event_filters: - if ef.matches(event): + if ef.matches(event, change): return True return False diff --git a/zuul/trigger/gerrit.py b/zuul/trigger/gerrit.py index 3a8644addf..fe9a1aab29 100644 --- a/zuul/trigger/gerrit.py +++ b/zuul/trigger/gerrit.py @@ -361,6 +361,8 @@ class Gerrit(object): if not dep.is_merged and dep.is_current_patchset: change.needed_by_changes.append(dep) + change.approvals = data['currentPatchSet'].get('approvals', []) + return change def getGitUrl(self, project):