Report dynamic layout config errors
If Zuul encounters a syntax error when loading an in-repo layout, report that as a failure on the change. Try to provide as much readable information as possible to allow the user to diagnose the error. All top-level configuration objects are updated to support a hidden source context variable to aid in creating a useful error message. Item.areAllJobsComplete is updated so that it returns true in the case of a merge failure or config error. In Zuulv2, we knew what jobs we would run even in those cases, but in Zuulv3, we don't, so the item does not have a job tree, so in those cases, the item does not report that all jobs are complete. Returining True in this case allows the NNFI algorithm to continue and expel the item from the queue, avoiding an inifinite loop. The merge failure accessor method is simplified. Change-Id: I9cf19b87c6af5926b5e8bb403b81ce0470e3592d
This commit is contained in:
James E. Blair
parent
a962657f56
commit
e53250c80d
|
|
@ -185,6 +185,27 @@ class TestInRepoConfig(ZuulTestCase):
|
|||
dict(name='project-test1', result='SUCCESS', changes='2,1'),
|
||||
dict(name='project-test2', result='SUCCESS', changes='3,1')])
|
||||
|
||||
def test_dynamic_syntax_error(self):
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: project-test2
|
||||
foo: error
|
||||
""")
|
||||
|
||||
file_dict = {'.zuul.yaml': in_repo_conf}
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
|
||||
files=file_dict)
|
||||
A.addApproval('code-review', 2)
|
||||
self.fake_gerrit.addEvent(A.addApproval('approved', 1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertEqual(A.data['status'], 'NEW')
|
||||
self.assertEqual(A.reported, 2,
|
||||
"A should report start and failure")
|
||||
self.assertIn('syntax error', A.messages[1],
|
||||
"A should have a syntax error reported")
|
||||
|
||||
|
||||
class TestAnsible(AnsibleZuulTestCase):
|
||||
# A temporary class to hold new tests while others are disabled
|
||||
|
|
|
|||
|
|
@ -10,11 +10,13 @@
|
|||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
from contextlib import contextmanager
|
||||
import copy
|
||||
import os
|
||||
import logging
|
||||
import six
|
||||
import yaml
|
||||
import pprint
|
||||
|
||||
import voluptuous as vs
|
||||
|
||||
|
|
@ -38,6 +40,35 @@ def as_list(item):
|
|||
return [item]
|
||||
|
||||
|
||||
class ConfigurationSyntaxError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
@contextmanager
|
||||
def configuration_exceptions(stanza, conf):
|
||||
try:
|
||||
yield
|
||||
except vs.Invalid as e:
|
||||
conf = copy.deepcopy(conf)
|
||||
context = conf.pop('_source_context')
|
||||
m = """
|
||||
Zuul encountered a syntax error while parsing its configuration in the
|
||||
repo {repo} on branch {branch}. The error was:
|
||||
|
||||
{error}
|
||||
|
||||
The offending content was a {stanza} stanza with the content:
|
||||
|
||||
{content}
|
||||
"""
|
||||
m = m.format(repo=context.project.name,
|
||||
branch=context.branch,
|
||||
error=str(e),
|
||||
stanza=stanza,
|
||||
content=pprint.pformat(conf))
|
||||
raise ConfigurationSyntaxError(m)
|
||||
|
||||
|
||||
class NodeSetParser(object):
|
||||
@staticmethod
|
||||
def getSchema():
|
||||
|
|
@ -47,13 +78,15 @@ class NodeSetParser(object):
|
|||
|
||||
nodeset = {vs.Required('name'): str,
|
||||
vs.Required('nodes'): [node],
|
||||
'_source_context': model.SourceContext,
|
||||
}
|
||||
|
||||
return vs.Schema(nodeset)
|
||||
|
||||
@staticmethod
|
||||
def fromYaml(layout, conf):
|
||||
NodeSetParser.getSchema()(conf)
|
||||
with configuration_exceptions('nodeset', conf):
|
||||
NodeSetParser.getSchema()(conf)
|
||||
ns = model.NodeSet(conf['name'])
|
||||
for conf_node in as_list(conf['nodes']):
|
||||
node = model.Node(conf_node['name'], conf_node['image'])
|
||||
|
|
@ -136,7 +169,8 @@ class JobParser(object):
|
|||
|
||||
@staticmethod
|
||||
def fromYaml(tenant, layout, conf):
|
||||
JobParser.getSchema()(conf)
|
||||
with configuration_exceptions('job', conf):
|
||||
JobParser.getSchema()(conf)
|
||||
|
||||
# NB: The default detection system in the Job class requires
|
||||
# that we always assign values directly rather than modifying
|
||||
|
|
@ -272,7 +306,8 @@ class ProjectTemplateParser(object):
|
|||
|
||||
@staticmethod
|
||||
def fromYaml(tenant, layout, conf):
|
||||
ProjectTemplateParser.getSchema(layout)(conf)
|
||||
with configuration_exceptions('project or project-template', conf):
|
||||
ProjectTemplateParser.getSchema(layout)(conf)
|
||||
# Make a copy since we modify this later via pop
|
||||
conf = copy.deepcopy(conf)
|
||||
project_template = model.ProjectConfig(conf['name'])
|
||||
|
|
@ -338,11 +373,13 @@ class ProjectParser(object):
|
|||
for p in layout.pipelines.values():
|
||||
project[p.name] = {'queue': str,
|
||||
'jobs': [vs.Any(str, dict)]}
|
||||
return vs.Schema([project])
|
||||
return vs.Schema(project)
|
||||
|
||||
@staticmethod
|
||||
def fromYaml(tenant, layout, conf_list):
|
||||
ProjectParser.getSchema(layout)(conf_list)
|
||||
for conf in conf_list:
|
||||
with configuration_exceptions('project', conf):
|
||||
ProjectParser.getSchema(layout)(conf)
|
||||
project = model.ProjectConfig(conf_list[0]['name'])
|
||||
mode = conf_list[0].get('merge-mode', 'merge-resolve')
|
||||
project.merge_mode = model.MERGER_MAP[mode]
|
||||
|
|
@ -461,6 +498,7 @@ class PipelineParser(object):
|
|||
'window-increase-factor': window_factor,
|
||||
'window-decrease-type': window_type,
|
||||
'window-decrease-factor': window_factor,
|
||||
'_source_context': model.SourceContext,
|
||||
}
|
||||
pipeline['trigger'] = vs.Required(
|
||||
PipelineParser.getDriverSchema('trigger', connections))
|
||||
|
|
@ -472,7 +510,8 @@ class PipelineParser(object):
|
|||
|
||||
@staticmethod
|
||||
def fromYaml(layout, connections, scheduler, conf):
|
||||
PipelineParser.getSchema(layout, connections)(conf)
|
||||
with configuration_exceptions('pipeline', conf):
|
||||
PipelineParser.getSchema(layout, connections)(conf)
|
||||
pipeline = model.Pipeline(conf['name'], layout)
|
||||
pipeline.description = conf.get('description')
|
||||
|
||||
|
|
|
|||
|
|
@ -265,6 +265,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.dequeued_needing_change:
|
||||
item.setDequeuedNeedingChange()
|
||||
|
||||
|
|
@ -482,8 +484,21 @@ class PipelineManager(object):
|
|||
import zuul.configloader
|
||||
loader = zuul.configloader.ConfigLoader()
|
||||
self.log.debug("Load dynamic layout with %s" % build_set.files)
|
||||
layout = loader.createDynamicLayout(item.pipeline.layout.tenant,
|
||||
build_set.files)
|
||||
try:
|
||||
layout = loader.createDynamicLayout(
|
||||
item.pipeline.layout.tenant,
|
||||
build_set.files)
|
||||
except zuul.configloader.ConfigurationSyntaxError as e:
|
||||
self.log.info("Configuration syntax error "
|
||||
"in dynamic layout %s" %
|
||||
build_set.files)
|
||||
item.setConfigError(str(e))
|
||||
return None
|
||||
except Exception:
|
||||
self.log.exception("Error in dynamic layout %s" %
|
||||
build_set.files)
|
||||
item.setConfigError("Unknown configuration error")
|
||||
return None
|
||||
return layout
|
||||
build_set.merge_state = build_set.PENDING
|
||||
self.log.debug("Preparing dynamic layout for: %s" % item.change)
|
||||
|
|
@ -556,6 +571,8 @@ class PipelineManager(object):
|
|||
ready = self.prepareLayout(item)
|
||||
if item.current_build_set.unable_to_merge:
|
||||
failing_reasons.append("it has a merge conflict")
|
||||
if item.current_build_set.config_error:
|
||||
failing_reasons.append("it has an invalid configuration")
|
||||
if ready and self.provisionNodes(item):
|
||||
changed = True
|
||||
if actionable and ready and self.launchJobs(item):
|
||||
|
|
@ -693,7 +710,12 @@ class PipelineManager(object):
|
|||
def _reportItem(self, item):
|
||||
self.log.debug("Reporting change %s" % item.change)
|
||||
ret = True # Means error as returned by trigger.report
|
||||
if not item.getJobs():
|
||||
if item.getConfigError():
|
||||
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
|
||||
item.setReportedResult('CONFIG_ERROR')
|
||||
elif not item.getJobs():
|
||||
# We don't send empty reports with +1,
|
||||
# and the same for -1's (merge failures or transient errors)
|
||||
# as they cannot be followed by +1's
|
||||
|
|
|
|||
|
|
@ -1012,6 +1012,7 @@ class BuildSet(object):
|
|||
self.commit = None
|
||||
self.zuul_url = None
|
||||
self.unable_to_merge = False
|
||||
self.config_error = None # None or an error message string.
|
||||
self.failing_reasons = []
|
||||
self.merge_state = self.NEW
|
||||
self.nodesets = {} # job -> nodeset
|
||||
|
|
@ -1170,6 +1171,9 @@ class QueueItem(object):
|
|||
return True
|
||||
|
||||
def areAllJobsComplete(self):
|
||||
if (self.current_build_set.config_error or
|
||||
self.current_build_set.unable_to_merge):
|
||||
return True
|
||||
if not self.hasJobTree():
|
||||
return False
|
||||
for job in self.getJobs():
|
||||
|
|
@ -1203,9 +1207,10 @@ class QueueItem(object):
|
|||
return False
|
||||
|
||||
def didMergerFail(self):
|
||||
if self.current_build_set.unable_to_merge:
|
||||
return True
|
||||
return False
|
||||
return self.current_build_set.unable_to_merge
|
||||
|
||||
def getConfigError(self):
|
||||
return self.current_build_set.config_error
|
||||
|
||||
def isHoldingFollowingChanges(self):
|
||||
if not self.live:
|
||||
|
|
@ -1325,6 +1330,10 @@ class QueueItem(object):
|
|||
self.current_build_set.unable_to_merge = True
|
||||
self._setAllJobsSkipped()
|
||||
|
||||
def setConfigError(self, error):
|
||||
self.current_build_set.config_error = error
|
||||
self._setAllJobsSkipped()
|
||||
|
||||
def _setAllJobsSkipped(self):
|
||||
for job in self.getJobs():
|
||||
fakebuild = Build(job, None)
|
||||
|
|
@ -2083,8 +2092,7 @@ class UnparsedTenantConfig(object):
|
|||
"a single key (when parsing %s)" %
|
||||
(conf,))
|
||||
key, value = item.items()[0]
|
||||
if key in ['project', 'project-template', 'job']:
|
||||
value['_source_context'] = source_context
|
||||
value['_source_context'] = source_context
|
||||
if key == 'project':
|
||||
name = value['name']
|
||||
self.projects.setdefault(name, []).append(value)
|
||||
|
|
|
|||
|
|
@ -85,6 +85,8 @@ class BaseReporter(object):
|
|||
msg = 'This change depends on a change that failed to merge.\n'
|
||||
elif item.didMergerFail():
|
||||
msg = pipeline.merge_failure_message
|
||||
elif item.getConfigError():
|
||||
msg = item.getConfigError()
|
||||
else:
|
||||
msg = (pipeline.failure_message + '\n\n' +
|
||||
self._formatItemReportJobs(pipeline, item))
|
||||
|
|
|
|||
Loading…
Reference in New Issue