Reduce config error context
We include the yaml text of the entire configuration object in
many of our configuration error reports. This can be quite large
which is not always helpful to the user, and can cause operational
problems with large numbers of errors of large objects.
To address that, let's only include a few lines. But let's also
try to make those lines useful by centering the actual attribute
that caused the error in our snippet.
To do this, we need to keep the line number of all of our configuration
attribute keys. This is accomplished by subclassing str and adding
a "line" attribute so that it behaves exactly like a string most
of the time, but when we go to assemble a configuration error, we can
check to see if we have a line number, and if so, zoom in on that line.
Example:
Zuul encountered a deprecated syntax while parsing its configuration
in the repo org/common-config on branch master. The problem was:
All regular expressions must conform to RE2 syntax, but an
expression using the deprecated Perl-style syntax has been detected.
Adjust the configuration to conform to RE2 syntax.
The RE2 syntax error is: invalid perl operator: (?!
The problem appears in the "branches" attribute
of the "org/project" project stanza:
...
name: org/project
check:
jobs:
- check-job:
branches: ^(?!invalid).*$
gate:
jobs:
- check-job
post:
...
in "org/common-config/zuul.yaml@master", line 84
This reduction is currently only performed for deprecation warnings but
can be extended to the rest of the configuration errors in subsequent
changes.
Change-Id: I9d9cb286dacee86d54ea854df48d7a4dd37f5f12
This commit is contained in:
James E. Blair
parent
d781291dbf
commit
a29b7e75af
|
|
@ -319,14 +319,33 @@ class ConfigurationSyntaxWarning(object, metaclass=abc.ABCMeta):
|
|||
zuul_error_message = 'Unknown Configuration Warning'
|
||||
|
||||
@abc.abstractmethod
|
||||
def makeConfigurationError(self, stanza, conf):
|
||||
def makeConfigurationError(self, stanza, conf, attr=None):
|
||||
"Return a ConfigurationError object"
|
||||
|
||||
|
||||
class DeprecationWarning(ConfigurationSyntaxWarning):
|
||||
def makeConfigurationError(self, stanza, conf):
|
||||
def makeConfigurationError(self, stanza, conf, attr=None):
|
||||
context = conf.get('_source_context')
|
||||
start_mark = conf.get('_start_mark')
|
||||
snippet = None
|
||||
name = conf.get('name')
|
||||
line = start_mark.line
|
||||
if name:
|
||||
name = f'"{name}"'
|
||||
else:
|
||||
name = 'following'
|
||||
if attr is not None:
|
||||
pointer = (
|
||||
f'The problem appears in the "{attr}" attribute\n'
|
||||
f'of the {name} {stanza} stanza:'
|
||||
)
|
||||
line = getattr(attr, 'line', line)
|
||||
else:
|
||||
pointer = (
|
||||
f'The problem appears in the the {name} {stanza} stanza:'
|
||||
)
|
||||
snippet = start_mark.getLineSnippet(line).rstrip()
|
||||
location = start_mark.getLineLocation(line)
|
||||
intro = textwrap.fill(textwrap.dedent("""\
|
||||
Zuul encountered a deprecated syntax while parsing its
|
||||
configuration in the repo {repo} on branch {branch}. The
|
||||
|
|
@ -340,17 +359,17 @@ class DeprecationWarning(ConfigurationSyntaxWarning):
|
|||
|
||||
{error}
|
||||
|
||||
The problem appears in the following {stanza} stanza:
|
||||
{pointer}
|
||||
|
||||
{content}
|
||||
|
||||
{start_mark}""")
|
||||
{location}""")
|
||||
|
||||
m = m.format(intro=intro,
|
||||
error=indent(self.zuul_error_message),
|
||||
stanza=stanza,
|
||||
content=indent(start_mark.snippet.rstrip()),
|
||||
start_mark=str(start_mark))
|
||||
pointer=pointer,
|
||||
content=indent(snippet),
|
||||
location=location)
|
||||
|
||||
return model.ConfigurationError(
|
||||
context, start_mark, m,
|
||||
|
|
@ -517,10 +536,16 @@ class LocalAccumulator:
|
|||
"""An error accumulator that wraps another accumulator (like
|
||||
LoadingErrors) while holding local context information.
|
||||
"""
|
||||
def __init__(self, accumulator, stanza, conf):
|
||||
def __init__(self, accumulator, stanza, conf, attr=None):
|
||||
self.accumulator = accumulator
|
||||
self.stanza = stanza
|
||||
self.conf = conf
|
||||
self.attr = attr
|
||||
|
||||
def extend(self, attr=None):
|
||||
"""Return a new accumulator that extends this one with additional
|
||||
info"""
|
||||
return LocalAccumulator(self.accumulator, self.stanza, self.conf, attr)
|
||||
|
||||
def addError(self, error):
|
||||
"""Add an error to the accumulator with the local context info.
|
||||
|
|
@ -532,10 +557,20 @@ class LocalAccumulator:
|
|||
exception handlers, so this method doesn't expect them.
|
||||
"""
|
||||
|
||||
e = error.makeConfigurationError(self.stanza, self.conf)
|
||||
e = error.makeConfigurationError(self.stanza, self.conf, self.attr)
|
||||
self.accumulator.addError(e)
|
||||
|
||||
|
||||
def get_conf_attr(conf, attr, accumulator=None):
|
||||
found = None
|
||||
for k in conf.keys():
|
||||
if k == attr:
|
||||
found = k
|
||||
break
|
||||
acc = accumulator.extend(found) if accumulator else None
|
||||
return acc, conf.get(found)
|
||||
|
||||
|
||||
class ZuulSafeLoader(yaml.EncryptedLoader):
|
||||
zuul_node_types = frozenset(('job', 'nodeset', 'secret', 'pipeline',
|
||||
'project', 'project-template',
|
||||
|
|
@ -569,6 +604,8 @@ class ZuulSafeLoader(yaml.EncryptedLoader):
|
|||
raise YAMLDuplicateKeyError(k.value, node, self.zuul_context,
|
||||
mark)
|
||||
keys.add(k.value)
|
||||
if k.tag == 'tag:yaml.org,2002:str':
|
||||
k.value = yaml.ZuulConfigKey(k.value, node.start_mark.line)
|
||||
r = super(ZuulSafeLoader, self).construct_mapping(node, deep)
|
||||
keys = frozenset(r.keys())
|
||||
if len(keys) == 1 and keys.intersection(self.zuul_node_types):
|
||||
|
|
@ -659,7 +696,8 @@ class PragmaParser(object):
|
|||
if bm is not None:
|
||||
source_context.implied_branch_matchers = bm
|
||||
|
||||
branches = conf.get('implied-branches')
|
||||
acc, branches = get_conf_attr(conf, 'implied-branches',
|
||||
error_accumulator)
|
||||
if branches is not None:
|
||||
# This is a BranchMatcher (not an ImpliedBranchMatcher)
|
||||
# because as user input, we allow/expect this to be
|
||||
|
|
@ -667,7 +705,7 @@ class PragmaParser(object):
|
|||
# (automatically generated from source file branches) are
|
||||
# ImpliedBranchMatchers.
|
||||
source_context.implied_branches = [
|
||||
change_matcher.BranchMatcher(make_regex(x, error_accumulator))
|
||||
change_matcher.BranchMatcher(make_regex(x, acc))
|
||||
for x in as_list(branches)]
|
||||
|
||||
|
||||
|
|
@ -1219,9 +1257,11 @@ class JobParser(object):
|
|||
|
||||
branches = None
|
||||
if 'branches' in conf:
|
||||
acc, conf_branches = get_conf_attr(conf, 'branches',
|
||||
error_accumulator)
|
||||
branches = [
|
||||
change_matcher.BranchMatcher(
|
||||
make_regex(x, error_accumulator))
|
||||
make_regex(x, acc))
|
||||
for x in as_list(conf['branches'])
|
||||
]
|
||||
elif not project_pipeline:
|
||||
|
|
|
|||
|
|
@ -18,7 +18,13 @@ import voluptuous as v
|
|||
from zuul.trigger import BaseTrigger
|
||||
from zuul.driver.gerrit.gerritmodel import GerritEventFilter
|
||||
from zuul.driver.gerrit import gerritsource
|
||||
from zuul.driver.util import scalar_or_list, to_list, make_regex, ZUUL_REGEX
|
||||
from zuul.driver.util import (
|
||||
scalar_or_list,
|
||||
to_list,
|
||||
make_regex,
|
||||
ZUUL_REGEX,
|
||||
get_conf_attr,
|
||||
)
|
||||
from zuul.configloader import DeprecationWarning
|
||||
|
||||
|
||||
|
|
@ -58,11 +64,13 @@ class GerritTrigger(BaseTrigger):
|
|||
usernames = to_list(trigger.get('username_filter'))
|
||||
ignore_deletes = trigger.get('ignore-deletes', True)
|
||||
if 'require-approval' in trigger:
|
||||
error_accumulator.addError(
|
||||
GerritRequireApprovalDeprecation())
|
||||
acc, _ = get_conf_attr(trigger, 'require-approval',
|
||||
error_accumulator)
|
||||
acc.addError(GerritRequireApprovalDeprecation())
|
||||
if 'reject-approval' in trigger:
|
||||
error_accumulator.addError(
|
||||
GerritRejectApprovalDeprecation())
|
||||
acc, _ = get_conf_attr(trigger, 'reject-approval',
|
||||
error_accumulator)
|
||||
acc.addError(GerritRejectApprovalDeprecation())
|
||||
|
||||
types = [make_regex(x, error_accumulator)
|
||||
for x in to_list(trigger['event'])]
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@
|
|||
|
||||
import voluptuous as vs
|
||||
|
||||
from zuul.configloader import ZUUL_REGEX, make_regex # noqa
|
||||
from zuul.configloader import ZUUL_REGEX, make_regex, get_conf_attr # noqa
|
||||
|
||||
|
||||
def time_to_seconds(s):
|
||||
|
|
|
|||
|
|
@ -77,6 +77,24 @@ class EncryptedPKCS1_OAEP:
|
|||
private_key).decode('utf8')
|
||||
|
||||
|
||||
class ZuulConfigKey(str):
|
||||
def __new__(cls, s, line):
|
||||
self = super().__new__(cls, s)
|
||||
self.line = line
|
||||
return self
|
||||
|
||||
def __copy__(self):
|
||||
return ZuulConfigKey(self, self.line)
|
||||
|
||||
def __deepcopy__(self, memo):
|
||||
return self.__copy__()
|
||||
|
||||
@classmethod
|
||||
def to_yaml(cls, dumper, data):
|
||||
return yaml.representer.SafeRepresenter.represent_str(
|
||||
dumper, str(data))
|
||||
|
||||
|
||||
def safe_load(stream, *args, **kwargs):
|
||||
return yaml.load(stream, *args, Loader=SafeLoader, **kwargs)
|
||||
|
||||
|
|
@ -102,6 +120,9 @@ EncryptedLoader.add_constructor(EncryptedPKCS1_OAEP.yaml_tag,
|
|||
EncryptedDumper.add_representer(
|
||||
types.MappingProxyType,
|
||||
yaml.representer.SafeRepresenter.represent_dict)
|
||||
EncryptedDumper.add_representer(
|
||||
ZuulConfigKey,
|
||||
ZuulConfigKey.to_yaml)
|
||||
|
||||
|
||||
def encrypted_dump(data, *args, **kwargs):
|
||||
|
|
|
|||
|
|
@ -174,6 +174,25 @@ class ZuulMark:
|
|||
return (self.line == other.line and
|
||||
self.snippet == other.snippet)
|
||||
|
||||
line_snippet_context = 4
|
||||
|
||||
def getLineSnippet(self, line):
|
||||
start = max(line - self.line - self.line_snippet_context, 0)
|
||||
end = start + (self.line_snippet_context * 2) + 1
|
||||
all_lines = self.snippet.splitlines()
|
||||
lines = all_lines[start:end]
|
||||
if start > 0:
|
||||
lines.insert(0, '...')
|
||||
if end < len(all_lines):
|
||||
lines.append('...')
|
||||
return '\n'.join(lines)
|
||||
|
||||
def getLineLocation(self, line):
|
||||
return ' in "{name}", line {line}'.format(
|
||||
name=self.name,
|
||||
line=line + 1,
|
||||
)
|
||||
|
||||
def serialize(self):
|
||||
return {
|
||||
"name": self.name,
|
||||
|
|
|
|||
Loading…
Reference in New Issue