Merge "Provide file locations of config syntax errors" into feature/zuulv3

3 files changed, 99 insertions, 51 deletions

diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py
index 38615a947..04d147381 100644
--- a/tests/unit/test_model.py
+++ b/tests/unit/test_model.py
@@ -18,6 +18,7 @@ import random
 
 import fixtures
 import testtools
+import yaml
 
 from zuul import model
 from zuul import configloader
@@ -32,15 +33,15 @@ class TestJob(BaseTestCase):
         self.project = model.Project('project', None)
         self.context = model.SourceContext(self.project, 'master',
                                            'test', True)
+        self.start_mark = yaml.Mark('name', 0, 0, 0, '', 0)
 
     @property
     def job(self):
         tenant = model.Tenant('tenant')
         layout = model.Layout()
-        project = model.Project('project', None)
-        context = model.SourceContext(project, 'master', 'test', True)
         job = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'job',
             'irrelevant-files': [
                 '^docs/.*$'
@@ -143,10 +144,10 @@ class TestJob(BaseTestCase):
         layout.addPipeline(pipeline)
         queue = model.ChangeQueue(pipeline)
         project = model.Project('project', None)
-        context = model.SourceContext(project, 'master', 'test', True)
 
         base = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'base',
             'timeout': 30,
             'pre-run': 'base-pre',
@@ -158,7 +159,8 @@ class TestJob(BaseTestCase):
         })
         layout.addJob(base)
         python27 = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'parent': 'base',
             'pre-run': 'py27-pre',
@@ -171,7 +173,8 @@ class TestJob(BaseTestCase):
         })
         layout.addJob(python27)
         python27diablo = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'branches': [
                 'stable/diablo'
@@ -188,7 +191,8 @@ class TestJob(BaseTestCase):
         layout.addJob(python27diablo)
 
         python27essex = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'branches': [
                 'stable/essex'
@@ -199,7 +203,8 @@ class TestJob(BaseTestCase):
         layout.addJob(python27essex)
 
         project_config = configloader.ProjectParser.fromYaml(tenant, layout, [{
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'project',
             'gate': {
                 'jobs': [
@@ -296,18 +301,18 @@ class TestJob(BaseTestCase):
     def test_job_auth_inheritance(self):
         tenant = model.Tenant('tenant')
         layout = model.Layout()
-        project = model.Project('project', None)
-        context = model.SourceContext(project, 'master', 'test', True)
 
         base = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'base',
             'timeout': 30,
         })
         layout.addJob(base)
         pypi_upload_without_inherit = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'pypi-upload-without-inherit',
                 'parent': 'base',
                 'timeout': 40,
@@ -320,7 +325,8 @@ class TestJob(BaseTestCase):
         layout.addJob(pypi_upload_without_inherit)
         pypi_upload_with_inherit = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'pypi-upload-with-inherit',
                 'parent': 'base',
                 'timeout': 40,
@@ -334,7 +340,8 @@ class TestJob(BaseTestCase):
         layout.addJob(pypi_upload_with_inherit)
         pypi_upload_with_inherit_false = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'pypi-upload-with-inherit-false',
                 'parent': 'base',
                 'timeout': 40,
@@ -348,21 +355,24 @@ class TestJob(BaseTestCase):
         layout.addJob(pypi_upload_with_inherit_false)
         in_repo_job_without_inherit = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'in-repo-job-without-inherit',
                 'parent': 'pypi-upload-without-inherit',
             })
         layout.addJob(in_repo_job_without_inherit)
         in_repo_job_with_inherit = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'in-repo-job-with-inherit',
                 'parent': 'pypi-upload-with-inherit',
             })
         layout.addJob(in_repo_job_with_inherit)
         in_repo_job_with_inherit_false = configloader.JobParser.fromYaml(
             tenant, layout, {
-                '_source_context': context,
+                '_source_context': self.context,
+                '_start_mark': self.start_mark,
                 'name': 'in-repo-job-with-inherit-false',
                 'parent': 'pypi-upload-with-inherit-false',
             })
@@ -381,24 +391,25 @@ class TestJob(BaseTestCase):
         pipeline = model.Pipeline('gate', layout)
         layout.addPipeline(pipeline)
         queue = model.ChangeQueue(pipeline)
-        project = model.Project('project', None)
-        context = model.SourceContext(project, 'master', 'test', True)
 
         base = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'base',
             'timeout': 30,
         })
         layout.addJob(base)
         python27 = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'parent': 'base',
             'timeout': 40,
         })
         layout.addJob(python27)
         python27diablo = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'branches': [
                 'stable/diablo'
@@ -408,7 +419,8 @@ class TestJob(BaseTestCase):
         layout.addJob(python27diablo)
 
         project_config = configloader.ProjectParser.fromYaml(tenant, layout, [{
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'project',
             'gate': {
                 'jobs': [
@@ -418,7 +430,7 @@ class TestJob(BaseTestCase):
         }])
         layout.addProjectConfig(project_config)
 
-        change = model.Change(project)
+        change = model.Change(self.project)
         change.branch = 'master'
         item = queue.enqueueChange(change)
         item.current_build_set.layout = layout
@@ -455,16 +467,17 @@ class TestJob(BaseTestCase):
         layout.addPipeline(pipeline)
         queue = model.ChangeQueue(pipeline)
         project = model.Project('project', None)
-        context = model.SourceContext(project, 'master', 'test', True)
 
         base = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'base',
             'timeout': 30,
         })
         layout.addJob(base)
         python27 = configloader.JobParser.fromYaml(tenant, layout, {
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'python27',
             'parent': 'base',
             'timeout': 40,
@@ -473,7 +486,8 @@ class TestJob(BaseTestCase):
         layout.addJob(python27)
 
         project_config = configloader.ProjectParser.fromYaml(tenant, layout, [{
-            '_source_context': context,
+            '_source_context': self.context,
+            '_start_mark': self.start_mark,
             'name': 'project',
             'gate': {
                 'jobs': [
@@ -504,6 +518,7 @@ class TestJob(BaseTestCase):
 
         base = configloader.JobParser.fromYaml(tenant, layout, {
             '_source_context': base_context,
+            '_start_mark': self.start_mark,
             'name': 'base',
         })
         layout.addJob(base)
@@ -513,6 +528,7 @@ class TestJob(BaseTestCase):
                                             'test', True)
         base2 = configloader.JobParser.fromYaml(tenant, layout, {
             '_source_context': other_context,
+            '_start_mark': self.start_mark,
             'name': 'base',
         })
         with testtools.ExpectedException(
diff --git a/zuul/configloader.py b/zuul/configloader.py
index 2c31341cf..b788237ac 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -17,6 +17,7 @@ import logging
 import six
 import yaml
 import pprint
+import textwrap
 
 import voluptuous as vs
 
@@ -44,6 +45,10 @@ class ConfigurationSyntaxError(Exception):
     pass
 
 
+def indent(s):
+    return '\n'.join(['  ' + x for x in s.split('\n')])
+
+
 @contextmanager
 def configuration_exceptions(stanza, conf):
     try:
@@ -51,28 +56,51 @@ def configuration_exceptions(stanza, conf):
     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:
+        start_mark = conf.pop('_start_mark')
+        intro = textwrap.fill(textwrap.dedent("""\
+        Zuul encountered a syntax error while parsing its configuration in the
+        repo {repo} on branch {branch}.  The error was:""".format(
+            repo=context.project.name,
+            branch=context.branch,
+        )))
 
-  {error}
+        m = textwrap.dedent("""\
+        {intro}
 
-The offending content was a {stanza} stanza with the content:
+        {error}
 
-{content}
-"""
-        m = m.format(repo=context.project.name,
-                     branch=context.branch,
-                     error=str(e),
+        The error appears in a {stanza} stanza with the content:
+
+        {content}
+
+        {start_mark}""")
+
+        m = m.format(intro=intro,
+                     error=indent(str(e)),
                      stanza=stanza,
-                     content=pprint.pformat(conf))
+                     content=indent(pprint.pformat(conf)),
+                     start_mark=str(start_mark))
         raise ConfigurationSyntaxError(m)
 
 
 class ZuulSafeLoader(yaml.SafeLoader):
+    zuul_node_types = frozenset(('job', 'nodeset', 'pipeline',
+                                 'project', 'project-template'))
+
     def __init__(self, stream, context):
         super(ZuulSafeLoader, self).__init__(stream)
         self.name = str(context)
+        self.zuul_context = context
+
+    def construct_mapping(self, node, deep=False):
+        r = super(ZuulSafeLoader, self).construct_mapping(node, deep)
+        keys = frozenset(r.keys())
+        if len(keys) == 1 and keys.intersection(self.zuul_node_types):
+            d = r.values()[0]
+            if isinstance(d, dict):
+                d['_start_mark'] = node.start_mark
+                d['_source_context'] = self.zuul_context
+        return r
 
 
 def safe_load_yaml(stream, context):
@@ -104,6 +132,7 @@ class NodeSetParser(object):
         nodeset = {vs.Required('name'): str,
                    vs.Required('nodes'): [node],
                    '_source_context': model.SourceContext,
+                   '_start_mark': yaml.Mark,
                    }
 
         return vs.Schema(nodeset)
@@ -160,6 +189,7 @@ class JobParser(object):
                'post-run': to_list(str),
                'run': str,
                '_source_context': model.SourceContext,
+               '_start_mark': yaml.Mark,
                'roles': to_list(role),
                'repos': to_list(str),
                'vars': dict,
@@ -310,6 +340,7 @@ class ProjectTemplateParser(object):
                 'merge', 'merge-resolve',
                 'cherry-pick'),
             '_source_context': model.SourceContext,
+            '_start_mark': yaml.Mark,
         }
 
         for p in layout.pipelines.values():
@@ -325,6 +356,7 @@ class ProjectTemplateParser(object):
         conf = copy.deepcopy(conf)
         project_template = model.ProjectConfig(conf['name'])
         source_context = conf['_source_context']
+        start_mark = conf['_start_mark']
         for pipeline in layout.pipelines.values():
             conf_pipeline = conf.get(pipeline.name)
             if not conf_pipeline:
@@ -334,11 +366,12 @@ class ProjectTemplateParser(object):
             project_pipeline.queue_name = conf_pipeline.get('queue')
             project_pipeline.job_tree = ProjectTemplateParser._parseJobTree(
                 tenant, layout, conf_pipeline.get('jobs', []),
-                source_context)
+                source_context, start_mark)
         return project_template
 
     @staticmethod
-    def _parseJobTree(tenant, layout, conf, source_context, tree=None):
+    def _parseJobTree(tenant, layout, conf, source_context,
+                      start_mark, tree=None):
         if not tree:
             tree = model.JobTree(None)
         for conf_job in conf:
@@ -354,6 +387,7 @@ class ProjectTemplateParser(object):
                     # We are overriding params, so make a new job def
                     attrs['name'] = jobname
                     attrs['_source_context'] = source_context
+                    attrs['_start_mark'] = start_mark
                     subtree = tree.addJob(JobParser.fromYaml(
                         tenant, layout, attrs))
                 else:
@@ -364,7 +398,8 @@ class ProjectTemplateParser(object):
                 if jobs:
                     # This is the root of a sub tree
                     ProjectTemplateParser._parseJobTree(
-                        tenant, layout, jobs, source_context, subtree)
+                        tenant, layout, jobs, source_context,
+                        start_mark, subtree)
             else:
                 raise Exception("Job must be a string or dictionary")
         return tree
@@ -381,6 +416,7 @@ class ProjectParser(object):
             'merge-mode': vs.Any('merge', 'merge-resolve',
                                  'cherry-pick'),
             '_source_context': model.SourceContext,
+            '_start_mark': yaml.Mark,
         }
 
         for p in layout.pipelines.values():
@@ -518,6 +554,7 @@ class PipelineParser(object):
                     'window-decrease-type': window_type,
                     'window-decrease-factor': window_factor,
                     '_source_context': model.SourceContext,
+                    '_start_mark': yaml.Mark,
                     }
         pipeline['trigger'] = vs.Required(
             PipelineParser.getDriverSchema('trigger', connections))
@@ -783,7 +820,7 @@ class TenantParser(object):
     def _parseConfigRepoLayout(data, source_context):
         # This is the top-level configuration for a tenant.
         config = model.UnparsedTenantConfig()
-        config.extend(safe_load_yaml(data, source_context), source_context)
+        config.extend(safe_load_yaml(data, source_context))
         return config
 
     @staticmethod
@@ -791,7 +828,7 @@ class TenantParser(object):
         # TODOv3(jeblair): this should implement some rules to protect
         # aspects of the config that should not be changed in-repo
         config = model.UnparsedTenantConfig()
-        config.extend(safe_load_yaml(data, source_context), source_context)
+        config.extend(safe_load_yaml(data, source_context))
         return config
 
     @staticmethod
diff --git a/zuul/model.py b/zuul/model.py
index 9118fd405..8cd357ac5 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -2051,7 +2051,7 @@ class UnparsedTenantConfig(object):
         r.nodesets = copy.deepcopy(self.nodesets)
         return r
 
-    def extend(self, conf, source_context=None):
+    def extend(self, conf):
         if isinstance(conf, UnparsedTenantConfig):
             self.pipelines.extend(conf.pipelines)
             self.jobs.extend(conf.jobs)
@@ -2066,10 +2066,6 @@ class UnparsedTenantConfig(object):
                             "a list of dictionaries (when parsing %s)" %
                             (conf,))
 
-        if source_context is None:
-            raise Exception("A source context must be provided "
-                            "(when parsing %s)" % (conf,))
-
         for item in conf:
             if not isinstance(item, dict):
                 raise Exception("Configuration items must be in the form of "
@@ -2080,7 +2076,6 @@ class UnparsedTenantConfig(object):
                                 "a single key (when parsing %s)" %
                                 (conf,))
             key, value = item.items()[0]
-            value['_source_context'] = source_context
             if key == 'project':
                 name = value['name']
                 self.projects.setdefault(name, []).append(value)