Report config errors as line comments
Change-Id: I17f827d661a46f75b72d85e7890237e76ebeb0e6
This commit is contained in:
parent
22e8e5b4f4
commit
bcfcf4f396
|
@ -17,6 +17,7 @@ driver=gerrit
|
||||||
server=review.example.com
|
server=review.example.com
|
||||||
user=jenkins
|
user=jenkins
|
||||||
sshkey=none
|
sshkey=none
|
||||||
|
password=badpassword
|
||||||
|
|
||||||
[connection github]
|
[connection github]
|
||||||
driver=github
|
driver=github
|
||||||
|
|
|
@ -1033,6 +1033,16 @@ class TestInRepoConfig(ZuulTestCase):
|
||||||
self.assertIn('Job non-existent-job not defined', A.messages[0],
|
self.assertIn('Job non-existent-job not defined', A.messages[0],
|
||||||
"A should have failed the check pipeline")
|
"A should have failed the check pipeline")
|
||||||
self.assertHistory([])
|
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):
|
def test_dynamic_config_non_existing_job_in_template(self):
|
||||||
"""Test that requesting a non existent job fails"""
|
"""Test that requesting a non existent job fails"""
|
||||||
|
|
|
@ -233,7 +233,7 @@ def configuration_exceptions(stanza, conf, accumulator):
|
||||||
content=indent(start_mark.snippet.rstrip()),
|
content=indent(start_mark.snippet.rstrip()),
|
||||||
start_mark=str(start_mark))
|
start_mark=str(start_mark))
|
||||||
|
|
||||||
accumulator.addError(context, start_mark, m)
|
accumulator.addError(context, start_mark, m, str(e))
|
||||||
|
|
||||||
|
|
||||||
@contextmanager
|
@contextmanager
|
||||||
|
@ -269,7 +269,7 @@ def reference_exceptions(stanza, obj, accumulator):
|
||||||
content=indent(start_mark.snippet.rstrip()),
|
content=indent(start_mark.snippet.rstrip()),
|
||||||
start_mark=str(start_mark))
|
start_mark=str(start_mark))
|
||||||
|
|
||||||
accumulator.addError(context, start_mark, m)
|
accumulator.addError(context, start_mark, m, str(e))
|
||||||
|
|
||||||
|
|
||||||
class ZuulMark(object):
|
class ZuulMark(object):
|
||||||
|
@ -280,7 +280,10 @@ class ZuulMark(object):
|
||||||
self.name = start_mark.name
|
self.name = start_mark.name
|
||||||
self.index = start_mark.index
|
self.index = start_mark.index
|
||||||
self.line = start_mark.line
|
self.line = start_mark.line
|
||||||
|
self.end_line = end_mark.line
|
||||||
|
self.end_index = end_mark.index
|
||||||
self.column = start_mark.column
|
self.column = start_mark.column
|
||||||
|
self.end_column = end_mark.column
|
||||||
self.snippet = stream[start_mark.index:end_mark.index]
|
self.snippet = stream[start_mark.index:end_mark.index]
|
||||||
|
|
||||||
def __str__(self):
|
def __str__(self):
|
||||||
|
|
|
@ -34,6 +34,14 @@ class GerritReporter(BaseReporter):
|
||||||
for fn, comments in fc.items():
|
for fn, comments in fc.items():
|
||||||
existing_comments = ret.setdefault(fn, [])
|
existing_comments = ret.setdefault(fn, [])
|
||||||
existing_comments += comments
|
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
|
return ret
|
||||||
|
|
||||||
def report(self, item):
|
def report(self, item):
|
||||||
|
|
|
@ -208,8 +208,8 @@ class PipelineManager(object):
|
||||||
# Similarly, reset the item state.
|
# Similarly, reset the item state.
|
||||||
if item.current_build_set.unable_to_merge:
|
if item.current_build_set.unable_to_merge:
|
||||||
item.setUnableToMerge()
|
item.setUnableToMerge()
|
||||||
if item.current_build_set.config_error:
|
if item.current_build_set.config_errors:
|
||||||
item.setConfigError(item.current_build_set.config_error)
|
item.setConfigErrors(item.current_build_set.config_errors)
|
||||||
if item.dequeued_needing_change:
|
if item.dequeued_needing_change:
|
||||||
item.setDequeuedNeedingChange()
|
item.setDequeuedNeedingChange()
|
||||||
|
|
||||||
|
@ -480,14 +480,17 @@ class PipelineManager(object):
|
||||||
# Find a layout loading error that match
|
# Find a layout loading error that match
|
||||||
# the current item.change and only report
|
# the current item.change and only report
|
||||||
# if one is found.
|
# if one is found.
|
||||||
|
relevant_errors = []
|
||||||
for err in layout.loading_errors.errors:
|
for err in layout.loading_errors.errors:
|
||||||
econtext = err.key.context
|
econtext = err.key.context
|
||||||
if ((err.key not in
|
if ((err.key not in
|
||||||
parent_layout.loading_errors.error_keys) or
|
parent_layout.loading_errors.error_keys) or
|
||||||
(econtext.project == item.change.project.name and
|
(econtext.project == item.change.project.name and
|
||||||
econtext.branch == item.change.branch)):
|
econtext.branch == item.change.branch)):
|
||||||
item.setConfigError(err.error)
|
relevant_errors.append(err)
|
||||||
return None
|
if relevant_errors:
|
||||||
|
item.setConfigErrors(relevant_errors)
|
||||||
|
return None
|
||||||
self.log.info(
|
self.log.info(
|
||||||
"Configuration syntax error not related to "
|
"Configuration syntax error not related to "
|
||||||
"change context. Error won't be reported.")
|
"change context. Error won't be reported.")
|
||||||
|
@ -557,7 +560,7 @@ class PipelineManager(object):
|
||||||
return False
|
return False
|
||||||
if build_set.unable_to_merge:
|
if build_set.unable_to_merge:
|
||||||
return False
|
return False
|
||||||
if build_set.config_error:
|
if build_set.config_errors:
|
||||||
return False
|
return False
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
@ -650,7 +653,7 @@ class PipelineManager(object):
|
||||||
if (not item.live) and (not dequeued):
|
if (not item.live) and (not dequeued):
|
||||||
self.dequeueItem(item)
|
self.dequeueItem(item)
|
||||||
changed = dequeued = True
|
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")
|
failing_reasons.append("it has an invalid configuration")
|
||||||
if (not item.live) and (not dequeued):
|
if (not item.live) and (not dequeued):
|
||||||
self.dequeueItem(item)
|
self.dequeueItem(item)
|
||||||
|
@ -808,7 +811,7 @@ class PipelineManager(object):
|
||||||
item.change.project, self.pipeline, item.change))
|
item.change.project, self.pipeline, item.change))
|
||||||
project_in_pipeline = False
|
project_in_pipeline = False
|
||||||
actions = []
|
actions = []
|
||||||
elif item.getConfigError():
|
elif item.getConfigErrors():
|
||||||
self.log.debug("Invalid config for change %s" % item.change)
|
self.log.debug("Invalid config for change %s" % item.change)
|
||||||
# TODOv3(jeblair): consider a new reporter action for this
|
# TODOv3(jeblair): consider a new reporter action for this
|
||||||
actions = self.pipeline.merge_failure_actions
|
actions = self.pipeline.merge_failure_actions
|
||||||
|
|
|
@ -129,8 +129,9 @@ class ConfigurationErrorKey(object):
|
||||||
class ConfigurationError(object):
|
class ConfigurationError(object):
|
||||||
|
|
||||||
"""A configuration error"""
|
"""A configuration error"""
|
||||||
def __init__(self, context, mark, error):
|
def __init__(self, context, mark, error, short_error=None):
|
||||||
self.error = str(error)
|
self.error = str(error)
|
||||||
|
self.short_error = short_error
|
||||||
self.key = ConfigurationErrorKey(context, mark, self.error)
|
self.key = ConfigurationErrorKey(context, mark, self.error)
|
||||||
|
|
||||||
|
|
||||||
|
@ -141,8 +142,8 @@ class LoadingErrors(object):
|
||||||
self.errors = []
|
self.errors = []
|
||||||
self.error_keys = set()
|
self.error_keys = set()
|
||||||
|
|
||||||
def addError(self, context, mark, error):
|
def addError(self, context, mark, error, short_error=None):
|
||||||
e = ConfigurationError(context, mark, error)
|
e = ConfigurationError(context, mark, error, short_error)
|
||||||
self.errors.append(e)
|
self.errors.append(e)
|
||||||
self.error_keys.add(e.key)
|
self.error_keys.add(e.key)
|
||||||
|
|
||||||
|
@ -1641,7 +1642,7 @@ class BuildSet(object):
|
||||||
self.dependent_changes = None
|
self.dependent_changes = None
|
||||||
self.merger_items = None
|
self.merger_items = None
|
||||||
self.unable_to_merge = False
|
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.failing_reasons = []
|
||||||
self.debug_messages = []
|
self.debug_messages = []
|
||||||
self.merge_state = self.NEW
|
self.merge_state = self.NEW
|
||||||
|
@ -1869,7 +1870,7 @@ class QueueItem(object):
|
||||||
return True
|
return True
|
||||||
|
|
||||||
def areAllJobsComplete(self):
|
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):
|
self.current_build_set.unable_to_merge):
|
||||||
return True
|
return True
|
||||||
if not self.hasJobGraph():
|
if not self.hasJobGraph():
|
||||||
|
@ -1937,8 +1938,8 @@ class QueueItem(object):
|
||||||
def didMergerFail(self):
|
def didMergerFail(self):
|
||||||
return self.current_build_set.unable_to_merge
|
return self.current_build_set.unable_to_merge
|
||||||
|
|
||||||
def getConfigError(self):
|
def getConfigErrors(self):
|
||||||
return self.current_build_set.config_error
|
return self.current_build_set.config_errors
|
||||||
|
|
||||||
def wasDequeuedNeedingChange(self):
|
def wasDequeuedNeedingChange(self):
|
||||||
return self.dequeued_needing_change
|
return self.dequeued_needing_change
|
||||||
|
@ -2131,7 +2132,11 @@ class QueueItem(object):
|
||||||
self._setAllJobsSkipped()
|
self._setAllJobsSkipped()
|
||||||
|
|
||||||
def setConfigError(self, error):
|
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()
|
self._setAllJobsSkipped()
|
||||||
|
|
||||||
def _setAllJobsSkipped(self):
|
def _setAllJobsSkipped(self):
|
||||||
|
|
|
@ -90,8 +90,8 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
|
||||||
msg = 'This change depends on a change that failed to merge.\n'
|
msg = 'This change depends on a change that failed to merge.\n'
|
||||||
elif item.didMergerFail():
|
elif item.didMergerFail():
|
||||||
msg = item.pipeline.merge_failure_message
|
msg = item.pipeline.merge_failure_message
|
||||||
elif item.getConfigError():
|
elif item.getConfigErrors():
|
||||||
msg = item.getConfigError()
|
msg = str(item.getConfigErrors()[0].error)
|
||||||
else:
|
else:
|
||||||
msg = item.pipeline.failure_message
|
msg = item.pipeline.failure_message
|
||||||
if with_jobs:
|
if with_jobs:
|
||||||
|
|
Loading…
Reference in New Issue