From a29b7e75af47631c4149491bfd84ef416cda3dc7 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 25 Oct 2023 17:24:19 -0700 Subject: [PATCH] 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 --- zuul/configloader.py | 64 +++++++++++++++++++++++------ zuul/driver/gerrit/gerrittrigger.py | 18 +++++--- zuul/driver/util.py | 2 +- zuul/lib/yamlutil.py | 21 ++++++++++ zuul/model.py | 19 +++++++++ 5 files changed, 106 insertions(+), 18 deletions(-) diff --git a/zuul/configloader.py b/zuul/configloader.py index f4b4c4ffd5..35e3ca010b 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -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: diff --git a/zuul/driver/gerrit/gerrittrigger.py b/zuul/driver/gerrit/gerrittrigger.py index 7207175ba7..ee317528e1 100644 --- a/zuul/driver/gerrit/gerrittrigger.py +++ b/zuul/driver/gerrit/gerrittrigger.py @@ -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'])] diff --git a/zuul/driver/util.py b/zuul/driver/util.py index fe0704679e..d277494b6f 100644 --- a/zuul/driver/util.py +++ b/zuul/driver/util.py @@ -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): diff --git a/zuul/lib/yamlutil.py b/zuul/lib/yamlutil.py index d32bc8e67c..3f235f4e21 100644 --- a/zuul/lib/yamlutil.py +++ b/zuul/lib/yamlutil.py @@ -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): diff --git a/zuul/model.py b/zuul/model.py index 8064d6e4dd..bb61c4a144 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -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,