Report config errors as line comments

Change-Id: I17f827d661a46f75b72d85e7890237e76ebeb0e6
This commit is contained in:
James E. Blair 2018-07-11 16:18:01 -07:00
parent 22e8e5b4f4
commit bcfcf4f396
7 changed files with 49 additions and 19 deletions

View File

@ -17,6 +17,7 @@ driver=gerrit
server=review.example.com
user=jenkins
sshkey=none
password=badpassword
[connection github]
driver=github

View File

@ -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"""

View File

@ -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):

View File

@ -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):

View File

@ -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

View File

@ -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):

View File

@ -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: