From b7179777f4df9d0832abb12c2fb055ab60b4499d Mon Sep 17 00:00:00 2001 From: Joshua Hesketh Date: Thu, 30 Jan 2014 23:30:46 +1100 Subject: [PATCH] Allow merge failures to have unique reporters. For example, we would like to be able to count (with the MySQL reporter) what jobs have failed because they can't be merged vs. real failures. Change-Id: I98eb8b8817bbda57efc2ef5bfcc2a5076fe8f4fd --- doc/source/zuul.rst | 12 ++ tests/fixtures/layout-merge-failure.yaml | 56 ++++++++ tests/fixtures/layouts/bad_merge_failure.yaml | 39 +++++ .../fixtures/layouts/good_merge_failure.yaml | 53 +++++++ tests/test_scheduler.py | 79 ++++++++++ zuul/layoutvalidator.py | 2 + zuul/model.py | 10 +- zuul/scheduler.py | 135 ++++++++++-------- 8 files changed, 324 insertions(+), 62 deletions(-) create mode 100644 tests/fixtures/layout-merge-failure.yaml create mode 100644 tests/fixtures/layouts/bad_merge_failure.yaml create mode 100644 tests/fixtures/layouts/good_merge_failure.yaml diff --git a/doc/source/zuul.rst b/doc/source/zuul.rst index 5086d4d640..24de7653f7 100644 --- a/doc/source/zuul.rst +++ b/doc/source/zuul.rst @@ -238,6 +238,12 @@ explanation of each of the parameters:: reported back to Gerrit when at least one voting build fails. Defaults to "Build failed." +**merge-failure-message** + An optional field that supplies the introductory text in message + reported back to Gerrit when a change fails to merge with the + current state of the repository. + Defaults to "Merge failed." + **footer-message** An optional field to supply additional information after test results. Useful for adding information about the CI system such as debugging @@ -413,6 +419,12 @@ explanation of each of the parameters:: Uses the same syntax as **success**, but describes what Zuul should do if at least one job fails. +**merge-failure** + Uses the same syntax as **success**, but describes what Zuul should + do if it is unable to merge in the patchset. If no merge-failure + reporters are listed then the ``failure`` reporters will be used to + notify of unsuccessful merges. + **start** Uses the same syntax as **success**, but describes what Zuul should do when a change is added to the pipeline manager. This can be used, diff --git a/tests/fixtures/layout-merge-failure.yaml b/tests/fixtures/layout-merge-failure.yaml new file mode 100644 index 0000000000..72bc9c9c09 --- /dev/null +++ b/tests/fixtures/layout-merge-failure.yaml @@ -0,0 +1,56 @@ +pipelines: + - name: check + manager: IndependentPipelineManager + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + verified: 1 + failure: + gerrit: + verified: -1 + + - name: post + manager: IndependentPipelineManager + trigger: + gerrit: + - event: ref-updated + ref: ^(?!refs/).*$ + + - name: gate + manager: DependentPipelineManager + failure-message: Build failed. For information on how to proceed, see http://wiki.example.org/Test_Failures + merge-failure-message: "The merge failed! For more information..." + trigger: + gerrit: + - event: comment-added + approval: + - approved: 1 + success: + gerrit: + verified: 2 + submit: true + failure: + gerrit: + verified: -2 + merge-failure: + gerrit: + verified: -1 + smtp: + to: you@example.com + start: + gerrit: + verified: 0 + precedence: high + +projects: + - name: org/project + check: + - project-merge: + - project-test1 + - project-test2 + gate: + - project-merge: + - project-test1 + - project-test2 diff --git a/tests/fixtures/layouts/bad_merge_failure.yaml b/tests/fixtures/layouts/bad_merge_failure.yaml new file mode 100644 index 0000000000..313d23b8a3 --- /dev/null +++ b/tests/fixtures/layouts/bad_merge_failure.yaml @@ -0,0 +1,39 @@ +pipelines: + - name: check + manager: IndependentPipelineManager + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + verified: 1 + failure: + gerrit: + verified: -1 + merge-failure-message: + + - 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 + approval: + - approved: 1 + success: + gerrit: + verified: 2 + submit: true + failure: + gerrit: + verified: -2 + merge-failure: + start: + gerrit: + verified: 0 + precedence: high + +projects: + - name: org/project + check: + - project-check diff --git a/tests/fixtures/layouts/good_merge_failure.yaml b/tests/fixtures/layouts/good_merge_failure.yaml new file mode 100644 index 0000000000..f69b764932 --- /dev/null +++ b/tests/fixtures/layouts/good_merge_failure.yaml @@ -0,0 +1,53 @@ +pipelines: + - name: check + manager: IndependentPipelineManager + merge-failure-message: "Could not merge the change. Please rebase..." + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + verified: 1 + failure: + gerrit: + verified: -1 + + - name: post + manager: IndependentPipelineManager + trigger: + gerrit: + - event: ref-updated + ref: ^(?!refs/).*$ + merge-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 + approval: + - approved: 1 + success: + gerrit: + verified: 2 + submit: true + failure: + gerrit: + verified: -2 + merge-failure: + gerrit: + verified: -1 + smtp: + to: you@example.com + start: + gerrit: + verified: 0 + precedence: high + +projects: + - name: org/project + check: + - project-check diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 1d7be4a6d8..496d46812b 100755 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -3712,3 +3712,82 @@ For CI problems and help debugging, contact ci@example.org""" self.assertEqual(failure_body, self.smtp_messages[0]['body']) self.assertEqual(success_body, self.smtp_messages[1]['body']) + + def test_merge_failure_reporters(self): + """Check that the config is set up correctly""" + + self.config.set('zuul', 'layout_config', + 'tests/fixtures/layout-merge-failure.yaml') + self.sched.reconfigure(self.config) + self.registerJobs() + + self.assertEqual( + "Merge Failed.\n\nThis change was unable to be automatically " + "merged with the current state of the repository. Please rebase " + "your change and upload a new patchset.", + self.sched.layout.pipelines['check'].merge_failure_message) + self.assertEqual( + "The merge failed! For more information...", + self.sched.layout.pipelines['gate'].merge_failure_message) + + self.assertEqual( + len(self.sched.layout.pipelines['check'].merge_failure_actions), 1) + self.assertEqual( + len(self.sched.layout.pipelines['gate'].merge_failure_actions), 2) + + self.assertTrue(isinstance( + self.sched.layout.pipelines['check'].merge_failure_actions[0]. + reporter, zuul.reporter.gerrit.Reporter)) + + self.assertTrue( + ( + isinstance(self.sched.layout.pipelines['gate']. + merge_failure_actions[0].reporter, + zuul.reporter.smtp.Reporter) and + isinstance(self.sched.layout.pipelines['gate']. + merge_failure_actions[1].reporter, + zuul.reporter.gerrit.Reporter) + ) or ( + isinstance(self.sched.layout.pipelines['gate']. + merge_failure_actions[0].reporter, + zuul.reporter.gerrit.Reporter) and + isinstance(self.sched.layout.pipelines['gate']. + merge_failure_actions[1].reporter, + zuul.reporter.smtp.Reporter) + ) + ) + + def test_merge_failure_reports(self): + """Check that when a change fails to merge the correct message is sent + to the correct reporter""" + self.config.set('zuul', 'layout_config', + 'tests/fixtures/layout-merge-failure.yaml') + self.sched.reconfigure(self.config) + self.registerJobs() + + # Check a test failure isn't reported to SMTP + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + A.addApproval('CRVW', 2) + self.worker.addFailTest('project-test1', A) + self.fake_gerrit.addEvent(A.addApproval('APRV', 1)) + self.waitUntilSettled() + + self.assertEqual(3, len(self.history)) # 3 jobs + self.assertEqual(0, len(self.smtp_messages)) + + # Check a merge failure is reported to SMTP + # B should be merged, but C will conflict with B + B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B') + B.addPatchset(['conflict']) + C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C') + C.addPatchset(['conflict']) + B.addApproval('CRVW', 2) + C.addApproval('CRVW', 2) + self.fake_gerrit.addEvent(B.addApproval('APRV', 1)) + self.fake_gerrit.addEvent(C.addApproval('APRV', 1)) + self.waitUntilSettled() + + self.assertEqual(6, len(self.history)) # A and B jobs + self.assertEqual(1, len(self.smtp_messages)) + self.assertEqual('The merge failed! For more information...', + self.smtp_messages[0]['body']) diff --git a/zuul/layoutvalidator.py b/zuul/layoutvalidator.py index de58c251e0..18b532d88a 100644 --- a/zuul/layoutvalidator.py +++ b/zuul/layoutvalidator.py @@ -79,11 +79,13 @@ class LayoutSchema(object): 'description': str, 'success-message': str, 'failure-message': str, + 'merge-failure-message': str, 'footer-message': str, 'dequeue-on-new-patchset': bool, 'trigger': trigger, 'success': report_actions, 'failure': report_actions, + 'merge-failure': report_actions, 'start': report_actions, 'window': window, 'window-floor': window_floor, diff --git a/zuul/model.py b/zuul/model.py index b20c08ea29..1a7c421c20 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -63,6 +63,7 @@ class Pipeline(object): self.name = name self.description = None self.failure_message = None + self.merge_failure_message = None self.success_message = None self.footer_message = None self.dequeue_on_new_patchset = True @@ -171,6 +172,11 @@ class Pipeline(object): return False return True + def didMergerSucceed(self, item): + if item.current_build_set.unable_to_merge: + return False + return True + def didAnyJobFail(self, item): for job in self.getJobs(item.change): if not job.voting: @@ -206,9 +212,8 @@ class Pipeline(object): fakebuild.result = 'SKIPPED' item.addBuild(fakebuild) - def setUnableToMerge(self, item, msg): + def setUnableToMerge(self, item): item.current_build_set.unable_to_merge = True - item.current_build_set.unable_to_merge_message = msg root = self.getJobTree(item.change.project) for job in root.getJobs(): fakebuild = Build(job, None) @@ -677,7 +682,6 @@ class BuildSet(object): self.commit = None self.zuul_url = None self.unable_to_merge = False - self.unable_to_merge_message = None self.failing_reasons = [] self.merge_state = self.NEW diff --git a/zuul/scheduler.py b/zuul/scheduler.py index c941a98eea..1a4474c233 100644 --- a/zuul/scheduler.py +++ b/zuul/scheduler.py @@ -226,6 +226,11 @@ class Scheduler(threading.Thread): pipeline.precedence = precedence pipeline.failure_message = conf_pipeline.get('failure-message', "Build failed.") + pipeline.merge_failure_message = conf_pipeline.get( + 'merge-failure-message', "Merge Failed.\n\nThis change was " + "unable to be automatically merged with the current state of " + "the repository. Please rebase your change and upload a new " + "patchset.") pipeline.success_message = conf_pipeline.get('success-message', "Build succeeded.") pipeline.footer_message = conf_pipeline.get('footer-message', "") @@ -233,7 +238,7 @@ class Scheduler(threading.Thread): 'dequeue-on-new-patchset', True) action_reporters = {} - for action in ['start', 'success', 'failure']: + for action in ['start', 'success', 'failure', 'merge-failure']: action_reporters[action] = [] if conf_pipeline.get(action): for reporter_name, params \ @@ -247,6 +252,11 @@ class Scheduler(threading.Thread): pipeline.start_actions = action_reporters['start'] pipeline.success_actions = action_reporters['success'] pipeline.failure_actions = action_reporters['failure'] + if len(action_reporters['merge-failure']) > 0: + pipeline.merge_failure_actions = \ + action_reporters['merge-failure'] + else: + pipeline.merge_failure_actions = action_reporters['failure'] pipeline.window = conf_pipeline.get('window', 20) pipeline.window_floor = conf_pipeline.get('window-floor', 3) @@ -936,6 +946,8 @@ class BasePipelineManager(object): self.log.info(" %s" % self.pipeline.success_actions) self.log.info(" On failure:") self.log.info(" %s" % self.pipeline.failure_actions) + self.log.info(" On merge-failure:") + self.log.info(" %s" % self.pipeline.merge_failure_actions) def getSubmitAllowNeeds(self): # Get a list of code review labels that are allowed to be @@ -1334,10 +1346,7 @@ class BasePipelineManager(object): build_set.commit = item.change.newrev if not build_set.commit: self.log.info("Unable to merge change %s" % item.change) - msg = ("This change was unable to be automatically merged " - "with the current state of the repository. Please " - "rebase your change and upload a new patchset.") - self.pipeline.setUnableToMerge(item, msg) + self.pipeline.setUnableToMerge(item) def reportItem(self, item): if item.reported: @@ -1370,10 +1379,12 @@ class BasePipelineManager(object): self.log.debug("Reporting change %s" % item.change) ret = True # Means error as returned by trigger.report if self.pipeline.didAllJobsSucceed(item): - self.log.debug("success %s %s" % (self.pipeline.success_actions, - self.pipeline.failure_actions)) + self.log.debug("success %s" % (self.pipeline.success_actions)) actions = self.pipeline.success_actions item.setReportedResult('SUCCESS') + elif not self.pipeline.didMergerSucceed(item): + actions = self.pipeline.merge_failure_actions + item.setReportedResult('MERGER_FAILURE') else: actions = self.pipeline.failure_actions item.setReportedResult('FAILURE') @@ -1395,66 +1406,72 @@ class BasePipelineManager(object): def formatReport(self, item): ret = '' + + if not self.pipeline.didMergerSucceed(item): + ret += self.pipeline.merge_failure_message + if item.dequeued_needing_change: + ret += ('\n\nThis change depends on a change that failed to ' + 'merge.') + if self.pipeline.footer_message: + ret += '\n\n' + self.pipeline.footer_message + return ret + if self.pipeline.didAllJobsSucceed(item): ret += self.pipeline.success_message + '\n\n' else: ret += self.pipeline.failure_message + '\n\n' - if item.dequeued_needing_change: - ret += "This change depends on a change that failed to merge." - elif item.current_build_set.unable_to_merge_message: - ret += item.current_build_set.unable_to_merge_message + if self.sched.config.has_option('zuul', 'url_pattern'): + url_pattern = self.sched.config.get('zuul', 'url_pattern') else: - if self.sched.config.has_option('zuul', 'url_pattern'): - url_pattern = self.sched.config.get('zuul', 'url_pattern') + url_pattern = None + + for job in self.pipeline.getJobs(item.change): + build = item.current_build_set.getBuild(job.name) + result = build.result + pattern = url_pattern + if result == 'SUCCESS': + if job.success_message: + result = job.success_message + if job.success_pattern: + pattern = job.success_pattern + elif result == 'FAILURE': + if job.failure_message: + result = job.failure_message + if job.failure_pattern: + pattern = job.failure_pattern + if pattern: + url = pattern.format(change=item.change, + pipeline=self.pipeline, + job=job, + build=build) else: - url_pattern = None - for job in self.pipeline.getJobs(item.change): - build = item.current_build_set.getBuild(job.name) - result = build.result - pattern = url_pattern - if result == 'SUCCESS': - if job.success_message: - result = job.success_message - if job.success_pattern: - pattern = job.success_pattern - elif result == 'FAILURE': - if job.failure_message: - result = job.failure_message - if job.failure_pattern: - pattern = job.failure_pattern - if pattern: - url = pattern.format(change=item.change, - pipeline=self.pipeline, - job=job, - build=build) + url = build.url or job.name + if not job.voting: + voting = ' (non-voting)' + else: + voting = '' + if self.report_times and build.end_time and build.start_time: + dt = int(build.end_time - build.start_time) + m, s = divmod(dt, 60) + h, m = divmod(m, 60) + if h: + elapsed = ' in %dh %02dm %02ds' % (h, m, s) + elif m: + elapsed = ' in %dm %02ds' % (m, s) else: - url = build.url or job.name - if not job.voting: - voting = ' (non-voting)' - else: - voting = '' - if self.report_times and build.end_time and build.start_time: - dt = int(build.end_time - build.start_time) - m, s = divmod(dt, 60) - h, m = divmod(m, 60) - if h: - elapsed = ' in %dh %02dm %02ds' % (h, m, s) - elif m: - elapsed = ' in %dm %02ds' % (m, s) - else: - elapsed = ' in %ds' % (s) - else: - elapsed = '' - name = '' - if self.sched.config.has_option('zuul', 'job_name_in_report'): - if self.sched.config.getboolean('zuul', - 'job_name_in_report'): - name = job.name + ' ' - ret += '- %s%s : %s%s%s\n' % (name, url, result, elapsed, - voting) - ret += '\n' - ret += self.pipeline.footer_message + elapsed = ' in %ds' % (s) + else: + elapsed = '' + name = '' + if self.sched.config.has_option('zuul', 'job_name_in_report'): + if self.sched.config.getboolean('zuul', + 'job_name_in_report'): + name = job.name + ' ' + ret += '- %s%s : %s%s%s\n' % (name, url, result, elapsed, + voting) + if self.pipeline.footer_message: + ret += '\n' + self.pipeline.footer_message return ret def formatDescription(self, build):