diff --git a/tests/fixtures/zuul-connections-gerrit-and-github.conf b/tests/fixtures/zuul-connections-gerrit-and-github.conf index 49e53c72fc..dcffe2ea11 100644 --- a/tests/fixtures/zuul-connections-gerrit-and-github.conf +++ b/tests/fixtures/zuul-connections-gerrit-and-github.conf @@ -17,6 +17,7 @@ driver=gerrit server=review.example.com user=jenkins sshkey=none +password=badpassword [connection github] driver=github diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 62e13449e4..195bc78045 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -1033,6 +1033,16 @@ class TestInRepoConfig(ZuulTestCase): self.assertIn('Job non-existent-job not defined', A.messages[0], "A should have failed the check pipeline") self.assertHistory([]) + self.assertEqual(len(A.comments), 1) + comments = sorted(A.comments, key=lambda x: x['line']) + self.assertEqual(comments[0], + {'file': '.zuul.yaml', + 'line': 9, + 'message': 'Job non-existent-job not defined', + 'reviewer': {'email': 'zuul@example.com', + 'name': 'Zuul', + 'username': 'jenkins'}} + ) def test_dynamic_config_non_existing_job_in_template(self): """Test that requesting a non existent job fails""" diff --git a/zuul/configloader.py b/zuul/configloader.py index 4a50e36db8..57fe1f9d74 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -233,7 +233,7 @@ def configuration_exceptions(stanza, conf, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.addError(context, start_mark, m) + accumulator.addError(context, start_mark, m, str(e)) @contextmanager @@ -269,7 +269,7 @@ def reference_exceptions(stanza, obj, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.addError(context, start_mark, m) + accumulator.addError(context, start_mark, m, str(e)) class ZuulMark(object): @@ -280,7 +280,10 @@ class ZuulMark(object): self.name = start_mark.name self.index = start_mark.index self.line = start_mark.line + self.end_line = end_mark.line + self.end_index = end_mark.index self.column = start_mark.column + self.end_column = end_mark.column self.snippet = stream[start_mark.index:end_mark.index] def __str__(self): diff --git a/zuul/driver/gerrit/gerritreporter.py b/zuul/driver/gerrit/gerritreporter.py index 21c61b07c3..9e7a524ff6 100644 --- a/zuul/driver/gerrit/gerritreporter.py +++ b/zuul/driver/gerrit/gerritreporter.py @@ -34,6 +34,14 @@ class GerritReporter(BaseReporter): for fn, comments in fc.items(): existing_comments = ret.setdefault(fn, []) existing_comments += comments + for err in item.getConfigErrors(): + context = err.key.context + mark = err.key.mark + if not (context and mark and err.short_error): + continue + existing_comments = ret.setdefault(context.path, []) + existing_comments.append(dict(line=mark.end_line, + message=err.short_error)) return ret def report(self, item): diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index a109cf168f..315bd97288 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -208,8 +208,8 @@ class PipelineManager(object): # Similarly, reset the item state. if item.current_build_set.unable_to_merge: item.setUnableToMerge() - if item.current_build_set.config_error: - item.setConfigError(item.current_build_set.config_error) + if item.current_build_set.config_errors: + item.setConfigErrors(item.current_build_set.config_errors) if item.dequeued_needing_change: item.setDequeuedNeedingChange() @@ -480,14 +480,17 @@ class PipelineManager(object): # Find a layout loading error that match # the current item.change and only report # if one is found. + relevant_errors = [] for err in layout.loading_errors.errors: econtext = err.key.context if ((err.key not in parent_layout.loading_errors.error_keys) or (econtext.project == item.change.project.name and econtext.branch == item.change.branch)): - item.setConfigError(err.error) - return None + relevant_errors.append(err) + if relevant_errors: + item.setConfigErrors(relevant_errors) + return None self.log.info( "Configuration syntax error not related to " "change context. Error won't be reported.") @@ -557,7 +560,7 @@ class PipelineManager(object): return False if build_set.unable_to_merge: return False - if build_set.config_error: + if build_set.config_errors: return False return True @@ -650,7 +653,7 @@ class PipelineManager(object): if (not item.live) and (not dequeued): self.dequeueItem(item) changed = dequeued = True - if item.current_build_set.config_error: + if item.current_build_set.config_errors: failing_reasons.append("it has an invalid configuration") if (not item.live) and (not dequeued): self.dequeueItem(item) @@ -808,7 +811,7 @@ class PipelineManager(object): item.change.project, self.pipeline, item.change)) project_in_pipeline = False actions = [] - elif item.getConfigError(): + elif item.getConfigErrors(): self.log.debug("Invalid config for change %s" % item.change) # TODOv3(jeblair): consider a new reporter action for this actions = self.pipeline.merge_failure_actions diff --git a/zuul/model.py b/zuul/model.py index 04a8c9ae1c..e58fcb1726 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -129,8 +129,9 @@ class ConfigurationErrorKey(object): class ConfigurationError(object): """A configuration error""" - def __init__(self, context, mark, error): + def __init__(self, context, mark, error, short_error=None): self.error = str(error) + self.short_error = short_error self.key = ConfigurationErrorKey(context, mark, self.error) @@ -141,8 +142,8 @@ class LoadingErrors(object): self.errors = [] self.error_keys = set() - def addError(self, context, mark, error): - e = ConfigurationError(context, mark, error) + def addError(self, context, mark, error, short_error=None): + e = ConfigurationError(context, mark, error, short_error) self.errors.append(e) self.error_keys.add(e.key) @@ -1641,7 +1642,7 @@ class BuildSet(object): self.dependent_changes = None self.merger_items = None self.unable_to_merge = False - self.config_error = None # None or an error message string. + self.config_errors = [] # list of ConfigurationErrors self.failing_reasons = [] self.debug_messages = [] self.merge_state = self.NEW @@ -1869,7 +1870,7 @@ class QueueItem(object): return True def areAllJobsComplete(self): - if (self.current_build_set.config_error or + if (self.current_build_set.config_errors or self.current_build_set.unable_to_merge): return True if not self.hasJobGraph(): @@ -1937,8 +1938,8 @@ class QueueItem(object): def didMergerFail(self): return self.current_build_set.unable_to_merge - def getConfigError(self): - return self.current_build_set.config_error + def getConfigErrors(self): + return self.current_build_set.config_errors def wasDequeuedNeedingChange(self): return self.dequeued_needing_change @@ -2131,7 +2132,11 @@ class QueueItem(object): self._setAllJobsSkipped() def setConfigError(self, error): - self.current_build_set.config_error = error + err = ConfigurationError(None, None, error) + self.setConfigErrors([err]) + + def setConfigErrors(self, errors): + self.current_build_set.config_errors = errors self._setAllJobsSkipped() def _setAllJobsSkipped(self): diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py index e05ee06654..1a7d11272d 100644 --- a/zuul/reporter/__init__.py +++ b/zuul/reporter/__init__.py @@ -90,8 +90,8 @@ class BaseReporter(object, metaclass=abc.ABCMeta): msg = 'This change depends on a change that failed to merge.\n' elif item.didMergerFail(): msg = item.pipeline.merge_failure_message - elif item.getConfigError(): - msg = item.getConfigError() + elif item.getConfigErrors(): + msg = str(item.getConfigErrors()[0].error) else: msg = item.pipeline.failure_message if with_jobs: