diff --git a/requirements.txt b/requirements.txt index 20778a1fd0..313790c181 100644 --- a/requirements.txt +++ b/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 diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 3dd8710566..3f8315e402 100644 --- a/tests/unit/test_v3.py +++ b/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( diff --git a/zuul/model.py b/zuul/model.py index 1d34af4339..3935a9b491 100644 --- a/zuul/model.py +++ b/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): - def __init__(self): - message = textwrap.dedent("""\ - Configuration file is not a list. Each zuul.yaml configuration - file must be a list of items, for example: +# TODO(ianw) : this would clearly be better if it recorded the +# original file and made line-relative comments, however the contexts +# the subclasses are raised in don't have that info currently, so this +# is a best-effort to show you something that clues you into the +# error. +class ConfigItemErrorException(Exception): + def __init__(self, conf): + super(ConfigItemErrorException, self).__init__( + self.message + self._generate_extract(conf)) - - job: - name: foo + def _generate_extract(self, conf): + context = textwrap.dedent("""\ - - project: - name: bar + The incorrect values are around: - 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): - def __init__(self): - message = textwrap.dedent("""\ - Configuration item is not a dictionary. Each zuul.yaml - configuration file must be a list of dictionaries, for - example: +class ConfigItemNotListError(ConfigItemErrorException): + message = textwrap.dedent("""\ + Configuration file is not a list. Each zuul.yaml configuration + file must be a list of items, for example: - - job: - name: foo + - job: + name: foo - - project: - name: bar + - project: + name: bar - Ensure that every item in the list is a dictionary with one - key (in this example, 'job' and 'project'). - """) - super(ConfigItemNotDictError, self).__init__(message) + Ensure that every item starts with "- " so that it is parsed as a + YAML list. + """) -class ConfigItemMultipleKeysError(Exception): - def __init__(self): - message = textwrap.dedent("""\ - Configuration item has more than one key. Each zuul.yaml - configuration file must be a list of dictionaries with a - single key, for example: +class ConfigItemNotDictError(ConfigItemErrorException): + message = textwrap.dedent("""\ + Configuration item is not a dictionary. Each zuul.yaml + configuration file must be a list of dictionaries, for + example: - - job: - name: foo + - job: + name: foo - - project: - name: bar + - project: + name: bar - Ensure that every item in the list is a dictionary with only - one key (in this example, 'job' and 'project'). This error - may be caused by insufficient indentation of the keys under - the configuration item ('name' in this example). - """) - super(ConfigItemMultipleKeysError, self).__init__(message) + Ensure that every item in the list is a dictionary with one + key (in this example, 'job' and 'project'). + """) -class ConfigItemUnknownError(Exception): - def __init__(self): - message = textwrap.dedent("""\ - Configuration item not recognized. Each zuul.yaml - configuration file must be a list of dictionaries, for - example: +class ConfigItemMultipleKeysError(ConfigItemErrorException): + message = textwrap.dedent("""\ + Configuration item has more than one key. Each zuul.yaml + configuration file must be a list of dictionaries with a + single key, for example: - - job: - name: foo + - job: + name: foo - - project: - name: bar + - project: + name: bar - The dictionary keys must match one of the configuration item - types recognized by zuul (for example, 'job' or 'project'). - """) - super(ConfigItemUnknownError, self).__init__(message) + Ensure that every item in the list is a dictionary with only + one key (in this example, 'job' and 'project'). This error + may be caused by insufficient indentation of the keys under + 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):