Merge "ConfigItem*Exception : add failure context"

requirements.txt

@@ -6,7 +6,7 @@ virtualenv!=20.0.0,!=20.0.1,>20
 
 python-dateutil
 github3.py>=1.1.0
-PyYAML>=3.1.0
+PyYAML>=5.1.0
 paramiko>=2.0.1
 GitPython>=2.1.8
 python-daemon>=2.0.4

tests/unit/test_v3.py

@@ -16,9 +16,10 @@ import io
 import json
 import logging
 import os
+import sys
 import textwrap
 import gc
-from unittest import skip
+from unittest import skip, skipIf
 
 import paramiko
 
@@ -1532,11 +1533,13 @@ class TestInRepoConfig(ZuulTestCase):
                          "A should report failure")
         self.assertIn('not a list', A.messages[0],
                       "A should have a syntax error reported")
+        self.assertIn('job: foo', A.messages[0],
+                      "A should display the failing list")
 
     def test_yaml_dict_error(self):
         in_repo_conf = textwrap.dedent(
             """
-            - job
+            - job_not_a_dict
             """)
 
         file_dict = {'.zuul.yaml': in_repo_conf}
@@ -1551,6 +1554,8 @@ class TestInRepoConfig(ZuulTestCase):
                          "A should report failure")
         self.assertIn('not a dictionary', A.messages[0],
                       "A should have a syntax error reported")
+        self.assertIn('job_not_a_dict', A.messages[0],
+                      "A should list the bad key")
 
     def test_yaml_duplicate_key_error(self):
         in_repo_conf = textwrap.dedent(
@@ -1592,6 +1597,41 @@ class TestInRepoConfig(ZuulTestCase):
                          "A should report failure")
         self.assertIn('has more than one key', A.messages[0],
                       "A should have a syntax error reported")
+        self.assertIn("job: null\n  name: project-test2", A.messages[0],
+                      "A should have the failing section displayed")
+
+    # This is non-deterministic without default dict ordering, which
+    # happended with python 3.7.
+    @skipIf(sys.version_info < (3, 7), "non-deterministic on < 3.7")
+    def test_yaml_error_truncation_message(self):
+        in_repo_conf = textwrap.dedent(
+            """
+            - job:
+              name: project-test2
+              this: is
+              a: long
+              set: of
+              keys: that
+              should: be
+              truncated: ok
+            """)
+
+        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, 1,
+                         "A should report failure")
+        self.assertIn('has more than one key', A.messages[0],
+                      "A should have a syntax error reported")
+        self.assertIn("job: null\n  name: project-test2", A.messages[0],
+                      "A should have the failing section displayed")
+        self.assertIn("...", A.messages[0],
+                      "A should have the failing section truncated")
 
     def test_yaml_unknown_error(self):
         in_repo_conf = textwrap.dedent(
@@ -1612,6 +1652,8 @@ class TestInRepoConfig(ZuulTestCase):
                          "A should report failure")
         self.assertIn('not recognized', A.messages[0],
                       "A should have a syntax error reported")
+        self.assertIn('foobar:\n    foo: bar', A.messages[0],
+                      "A should report the bad keys")
 
     def test_invalid_job_secret_var_name(self):
         in_repo_conf = textwrap.dedent(

zuul/model.py

@@ -26,6 +26,7 @@ import urllib.parse
 import textwrap
 import types
 import itertools
+import yaml
 
 import jsonpath_rw
 
@@ -3526,81 +3527,103 @@ class ProjectMetadata(object):
         self.default_branch = None
 
 
-class ConfigItemNotListError(Exception):
+# TODO(ianw) : this would clearly be better if it recorded the
-    def __init__(self):
+# original file and made line-relative comments, however the contexts
-        message = textwrap.dedent("""\
+# the subclasses are raised in don't have that info currently, so this
-        Configuration file is not a list.  Each zuul.yaml configuration
+# is a best-effort to show you something that clues you into the
-        file must be a list of items, for example:
+# error.
+class ConfigItemErrorException(Exception):
+    def __init__(self, conf):
+        super(ConfigItemErrorException, self).__init__(
+            self.message + self._generate_extract(conf))
 
-        - job:
+    def _generate_extract(self, conf):
-            name: foo
+        context = textwrap.dedent("""\
 
-        - project:
+        The incorrect values are around:
-            name: bar
 
-        Ensure that every item starts with "- " so that it is parsed as a
-        YAML list.
         """)
-        super(ConfigItemNotListError, self).__init__(message)
+        # Not sorting the keys makes it look closer to what is in the
+        # file and works best with >= Python 3.7 where dicts are
+        # ordered by default.  If this is a foreign config file or
+        # something the dump might be really long; hence the
+        # truncation.
+        extract = yaml.dump(conf, sort_keys=False)
+        lines = extract.split('\n')
+        if len(lines) > 5:
+            lines = lines[0:4]
+            lines.append('...')
+
+        return context + '\n'.join(lines)
 
 
-class ConfigItemNotDictError(Exception):
+class ConfigItemNotListError(ConfigItemErrorException):
-    def __init__(self):
+    message = textwrap.dedent("""\
-        message = textwrap.dedent("""\
+    Configuration file is not a list.  Each zuul.yaml configuration
-        Configuration item is not a dictionary.  Each zuul.yaml
+    file must be a list of items, for example:
-        configuration file must be a list of dictionaries, for
-        example:
 
-        - job:
+    - job:
-            name: foo
+        name: foo
 
-        - project:
+    - project:
-            name: bar
+        name: bar
 
-        Ensure that every item in the list is a dictionary with one
+    Ensure that every item starts with "- " so that it is parsed as a
-        key (in this example, 'job' and 'project').
+    YAML list.
-        """)
+    """)
-        super(ConfigItemNotDictError, self).__init__(message)
 
 
-class ConfigItemMultipleKeysError(Exception):
+class ConfigItemNotDictError(ConfigItemErrorException):
-    def __init__(self):
+    message = textwrap.dedent("""\
-        message = textwrap.dedent("""\
+    Configuration item is not a dictionary.  Each zuul.yaml
-        Configuration item has more than one key.  Each zuul.yaml
+    configuration file must be a list of dictionaries, for
-        configuration file must be a list of dictionaries with a
+    example:
-        single key, for example:
 
-        - job:
+    - job:
-            name: foo
+        name: foo
 
-        - project:
+    - project:
-            name: bar
+        name: bar
 
-        Ensure that every item in the list is a dictionary with only
+    Ensure that every item in the list is a dictionary with one
-        one key (in this example, 'job' and 'project').  This error
+    key (in this example, 'job' and 'project').
-        may be caused by insufficient indentation of the keys under
+    """)
-        the configuration item ('name' in this example).
-        """)
-        super(ConfigItemMultipleKeysError, self).__init__(message)
 
 
-class ConfigItemUnknownError(Exception):
+class ConfigItemMultipleKeysError(ConfigItemErrorException):
-    def __init__(self):
+    message = textwrap.dedent("""\
-        message = textwrap.dedent("""\
+    Configuration item has more than one key.  Each zuul.yaml
-        Configuration item not recognized.  Each zuul.yaml
+    configuration file must be a list of dictionaries with a
-        configuration file must be a list of dictionaries, for
+    single key, for example:
-        example:
 
-        - job:
+    - job:
-            name: foo
+        name: foo
 
-        - project:
+    - project:
-            name: bar
+        name: bar
 
-        The dictionary keys must match one of the configuration item
+    Ensure that every item in the list is a dictionary with only
-        types recognized by zuul (for example, 'job' or 'project').
+    one key (in this example, 'job' and 'project').  This error
-        """)
+    may be caused by insufficient indentation of the keys under
-        super(ConfigItemUnknownError, self).__init__(message)
+    the configuration item ('name' in this example).
+    """)
+
+
+class ConfigItemUnknownError(ConfigItemErrorException):
+    message = textwrap.dedent("""\
+    Configuration item not recognized.  Each zuul.yaml
+    configuration file must be a list of dictionaries, for
+    example:
+
+    - job:
+        name: foo
+
+    - project:
+        name: bar
+
+    The dictionary keys must match one of the configuration item
+    types recognized by zuul (for example, 'job' or 'project').
+    """)
 
 
 class UnparsedAbideConfig(object):
@@ -3622,13 +3645,13 @@ class UnparsedAbideConfig(object):
             return
 
         if not isinstance(conf, list):
-            raise ConfigItemNotListError()
+            raise ConfigItemNotListError(conf)
 
         for item in conf:
             if not isinstance(item, dict):
-                raise ConfigItemNotDictError()
+                raise ConfigItemNotDictError(item)
             if len(item.keys()) > 1:
-                raise ConfigItemMultipleKeysError()
+                raise ConfigItemMultipleKeysError(item)
             key, value = list(item.items())[0]
             if key == 'tenant':
                 self.tenants.append(value)
@@ -3637,7 +3660,7 @@ class UnparsedAbideConfig(object):
             elif key == 'admin-rule':
                 self.admin_rules.append(value)
             else:
-                raise ConfigItemUnknownError()
+                raise ConfigItemUnknownError(item)
 
 
 class UnparsedConfig(object):
@@ -3698,13 +3721,13 @@ class UnparsedConfig(object):
             return
 
         if not isinstance(conf, list):
-            raise ConfigItemNotListError()
+            raise ConfigItemNotListError(conf)
 
         for item in conf:
             if not isinstance(item, dict):
-                raise ConfigItemNotDictError()
+                raise ConfigItemNotDictError(item)
             if len(item.keys()) > 1:
-                raise ConfigItemMultipleKeysError()
+                raise ConfigItemMultipleKeysError(item)
             key, value = list(item.items())[0]
             if key == 'project':
                 self.projects.append(value)
@@ -3723,7 +3746,7 @@ class UnparsedConfig(object):
             elif key == 'pragma':
                 self.pragmas.append(value)
             else:
-                raise ConfigItemUnknownError()
+                raise ConfigItemUnknownError(item)
 
 
 class ParsedConfig(object):