From 1a477dc9941bb8199838eb759e91b7f5be54809a Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 6 Jun 2023 14:39:13 -0700 Subject: [PATCH] Add configuration warnings This adds the ability for the configuration loader to accumulate warning-level configuration errors. Additionally, the driver-specific triggers can do the same. An initial pair of warnings is added for two recently deprecated attributes on the gerrit trigger. Change-Id: I61f856b3aea3e0ad3147c52c7aa724f40734d1f5 --- tests/unit/test_requirements.py | 28 ++++++++++ zuul/configloader.py | 87 ++++++++++++++++++++++++++--- zuul/driver/__init__.py | 4 +- zuul/driver/gerrit/gerrittrigger.py | 23 +++++++- zuul/driver/git/gittrigger.py | 3 +- zuul/driver/github/githubtrigger.py | 3 +- zuul/driver/gitlab/gitlabtrigger.py | 3 +- zuul/driver/pagure/paguretrigger.py | 3 +- zuul/driver/timer/timertrigger.py | 3 +- zuul/driver/zuul/zuultrigger.py | 3 +- zuul/model.py | 11 ++-- zuul/trigger/__init__.py | 3 +- 12 files changed, 154 insertions(+), 20 deletions(-) diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py index c5dca56cd2..06c040b4f4 100644 --- a/tests/unit/test_requirements.py +++ b/tests/unit/test_requirements.py @@ -197,6 +197,20 @@ class TestRequirementsVote1(ZuulTestCase): self.assertEqual(len(self.history), 1) self.assertEqual(self.history[0].name, job) + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + self.assertEqual(len(tenant.layout.loading_errors), 1) + self.assertEqual(tenant.layout.loading_errors[0].name, + 'Gerrit require-approval Deprecation') + self.assertEqual(tenant.layout.loading_errors[0].severity, + 'warning') + self.assertIn('require-approval', + tenant.layout.loading_errors[0].short_error) + self.assertIn('require-approval', + tenant.layout.loading_errors[0].error) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.context) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.mark) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.error_text) + class TestRequirementsVote2(ZuulTestCase): """Requirements with a voting requirement""" @@ -256,6 +270,20 @@ class TestRequirementsVote2(ZuulTestCase): self.assertEqual(len(self.history), 2) self.assertEqual(self.history[1].name, job) + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + self.assertEqual(len(tenant.layout.loading_errors), 1) + self.assertEqual(tenant.layout.loading_errors[0].name, + 'Gerrit require-approval Deprecation') + self.assertEqual(tenant.layout.loading_errors[0].severity, + 'warning') + self.assertIn('require-approval', + tenant.layout.loading_errors[0].short_error) + self.assertIn('require-approval', + tenant.layout.loading_errors[0].error) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.context) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.mark) + self.assertIsNotNone(tenant.layout.loading_errors[0].key.error_text) + class TestRequirementsState(ZuulTestCase): """Requirements with simple state requirement""" diff --git a/zuul/configloader.py b/zuul/configloader.py index 28426ca03e..91e65633d1 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import abc import collections from contextlib import contextmanager from concurrent.futures import ThreadPoolExecutor, as_completed @@ -74,6 +75,10 @@ def check_config_path(path): "allowed in extra-config-paths") +def indent(s): + return '\n'.join([' ' + x for x in s.split('\n')]) + + class ConfigurationSyntaxError(Exception): pass @@ -271,8 +276,50 @@ class YAMLDuplicateKeyError(ConfigurationSyntaxError): super(YAMLDuplicateKeyError, self).__init__(m) -def indent(s): - return '\n'.join([' ' + x for x in s.split('\n')]) +class ConfigurationSyntaxWarning(object, metaclass=abc.ABCMeta): + zuul_error_name = 'Unknown Configuration Warning' + zuul_error_severity = model.SEVERITY_WARNING + zuul_error_message = 'Unknown Configuration Warning' + + @abc.abstractmethod + def makeConfigurationError(self, stanza, conf): + "Return a ConfigurationError object" + + +class DeprecationWarning(ConfigurationSyntaxWarning): + def makeConfigurationError(self, stanza, conf): + context = conf.get('_source_context') + start_mark = conf.get('_start_mark') + intro = textwrap.fill(textwrap.dedent("""\ + Zuul encountered a deprecated syntax while parsing its + configuration in the repo {repo} on branch {branch}. The + problem was:""".format( + repo=context.project_name, + branch=context.branch, + ))) + + m = textwrap.dedent("""\ + {intro} + + {error} + + The problem appears in the following {stanza} stanza: + + {content} + + {start_mark}""") + + m = m.format(intro=intro, + error=indent(self.zuul_error_message), + stanza=stanza, + content=indent(start_mark.snippet.rstrip()), + start_mark=str(start_mark)) + + return model.ConfigurationError( + context, start_mark, m, + short_error=self.zuul_error_message, + severity=self.zuul_error_severity, + name=self.zuul_error_name) @contextmanager @@ -297,7 +344,7 @@ def project_configuration_exceptions(context, accumulator): m = m.format(intro=intro, error=indent(str(e))) - accumulator.addError( + accumulator.makeError( context, None, m, short_error=str(e), severity=getattr(e, 'zuul_error_severity', model.SEVERITY_ERROR), @@ -324,7 +371,7 @@ def early_configuration_exceptions(context, accumulator): m = m.format(intro=intro, error=indent(str(e))) - accumulator.addError( + accumulator.makeError( context, None, m, short_error=str(e), severity=getattr(e, 'zuul_error_severity', model.SEVERITY_ERROR), @@ -365,7 +412,7 @@ def configuration_exceptions(stanza, conf, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.addError( + accumulator.makeError( context, start_mark, m, short_error=str(e), severity=getattr(e, 'zuul_error_severity', model.SEVERITY_ERROR), @@ -405,13 +452,36 @@ def reference_exceptions(stanza, obj, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.addError( + accumulator.makeError( context, start_mark, m, short_error=str(e), severity=getattr(e, 'zuul_error_severity', model.SEVERITY_ERROR), name=getattr(e, 'zuul_error_name', 'Unknown')) +class LocalAccumulator: + """An error accumulator that wraps another accumulator (like + LoadingErrors) while holding local context information. + """ + def __init__(self, accumulator, stanza, conf): + self.accumulator = accumulator + self.stanza = stanza + self.conf = conf + + def addError(self, error): + """Add an error to the accumulator with the local context info. + + This method currently expects that the input error object is a + subclass of ConfigurationSyntaxWarning and has an addError + method that returns a fully populated ConfigurationError. + Currently all "error" severity level errors are handled by + exception handlers, so this method doesn't expect them. + """ + + e = error.makeConfigurationError(self.stanza, self.conf) + self.accumulator.addError(e) + + class ZuulSafeLoader(yaml.EncryptedLoader): zuul_node_types = frozenset(('job', 'nodeset', 'secret', 'pipeline', 'project', 'project-template', @@ -1518,6 +1588,8 @@ class PipelineParser(object): source.getRejectFilters(reject_config)) seen_connections.add(source_name) + local_accumulator = LocalAccumulator(self.pcontext.loading_errors, + 'pipeline', conf) for connection_name, trigger_config in conf.get('trigger').items(): if self.pcontext.tenant.allowed_triggers is not None and \ connection_name not in self.pcontext.tenant.allowed_triggers: @@ -1527,7 +1599,8 @@ class PipelineParser(object): pipeline.triggers.append(trigger) manager.event_filters.extend( trigger.getEventFilters(connection_name, - conf['trigger'][connection_name])) + conf['trigger'][connection_name], + local_accumulator)) seen_connections.add(connection_name) pipeline.connections = list(seen_connections) diff --git a/zuul/driver/__init__.py b/zuul/driver/__init__.py index 863e5a4a26..81eddd9bde 100644 --- a/zuul/driver/__init__.py +++ b/zuul/driver/__init__.py @@ -129,7 +129,7 @@ class TriggerInterface(object, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def getTrigger(self, connection, config=None): + def getTrigger(self, connection, config=None, loading_errors=None): """Create and return a new trigger object. This method is required by the interface. @@ -142,6 +142,8 @@ class TriggerInterface(object, metaclass=abc.ABCMeta): or None. :arg dict config: The configuration information supplied along with the trigger in the layout. + :arg dict loading_errors: A LoadingErrors instance to accumulate + errors or warnings. :returns: A new trigger object. :rtype: :py:class:`~zuul.trigger.BaseTrigger` diff --git a/zuul/driver/gerrit/gerrittrigger.py b/zuul/driver/gerrit/gerrittrigger.py index dff5dc32ca..fdca7fc823 100644 --- a/zuul/driver/gerrit/gerrittrigger.py +++ b/zuul/driver/gerrit/gerrittrigger.py @@ -18,13 +18,27 @@ 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 +from zuul.configloader import DeprecationWarning + + +class GerritRequireApprovalDeprecation(DeprecationWarning): + zuul_error_name = 'Gerrit require-approval Deprecation' + zuul_error_message = """The 'require-approval' trigger attribute +is deprecated. Use 'require' instead.""" + + +class GerritRejectApprovalDeprecation(DeprecationWarning): + zuul_error_name = 'Gerrit reject-approval Deprecation' + zuul_error_message = """The 'reject-approval' trigger attribute +is deprecated. Use 'reject' instead.""" class GerritTrigger(BaseTrigger): name = 'gerrit' log = logging.getLogger("zuul.GerritTrigger") - def getEventFilters(self, connection_name, trigger_conf): + def getEventFilters(self, connection_name, trigger_conf, + error_accumulator): efilters = [] for trigger in to_list(trigger_conf): approvals = {} @@ -42,6 +56,13 @@ class GerritTrigger(BaseTrigger): if not usernames: usernames = to_list(trigger.get('username_filter')) ignore_deletes = trigger.get('ignore-deletes', True) + if 'require-approval' in trigger: + error_accumulator.addError( + GerritRequireApprovalDeprecation()) + if 'reject-approval' in trigger: + error_accumulator.addError( + GerritRejectApprovalDeprecation()) + f = GerritEventFilter( connection_name=connection_name, trigger=self, diff --git a/zuul/driver/git/gittrigger.py b/zuul/driver/git/gittrigger.py index 9405ed3b54..76d687595b 100644 --- a/zuul/driver/git/gittrigger.py +++ b/zuul/driver/git/gittrigger.py @@ -23,7 +23,8 @@ class GitTrigger(BaseTrigger): name = 'git' log = logging.getLogger("zuul.GitTrigger") - def getEventFilters(self, connection_name, trigger_conf): + def getEventFilters(self, connection_name, trigger_conf, + error_accumulator): efilters = [] for trigger in to_list(trigger_conf): f = GitEventFilter( diff --git a/zuul/driver/github/githubtrigger.py b/zuul/driver/github/githubtrigger.py index 5072fda430..7922697b00 100644 --- a/zuul/driver/github/githubtrigger.py +++ b/zuul/driver/github/githubtrigger.py @@ -35,7 +35,8 @@ class GithubTrigger(BaseTrigger): super().__init__(driver, connection, config=config) - def getEventFilters(self, connection_name, trigger_config): + def getEventFilters(self, connection_name, trigger_config, + error_accumulator): efilters = [] for trigger in to_list(trigger_config): f = GithubEventFilter( diff --git a/zuul/driver/gitlab/gitlabtrigger.py b/zuul/driver/gitlab/gitlabtrigger.py index 0fdc2ff2fe..a5c9282f24 100644 --- a/zuul/driver/gitlab/gitlabtrigger.py +++ b/zuul/driver/gitlab/gitlabtrigger.py @@ -23,7 +23,8 @@ class GitlabTrigger(BaseTrigger): name = 'gitlab' log = logging.getLogger("zuul.trigger.GitlabTrigger") - def getEventFilters(self, connection_name, trigger_config): + def getEventFilters(self, connection_name, trigger_config, + error_accumulator): efilters = [] for trigger in to_list(trigger_config): f = GitlabEventFilter( diff --git a/zuul/driver/pagure/paguretrigger.py b/zuul/driver/pagure/paguretrigger.py index e65a4a42fa..686df6e7f9 100644 --- a/zuul/driver/pagure/paguretrigger.py +++ b/zuul/driver/pagure/paguretrigger.py @@ -23,7 +23,8 @@ class PagureTrigger(BaseTrigger): name = 'pagure' log = logging.getLogger("zuul.trigger.PagureTrigger") - def getEventFilters(self, connection_name, trigger_config): + def getEventFilters(self, connection_name, trigger_config, + error_accumulator): efilters = [] for trigger in to_list(trigger_config): f = PagureEventFilter( diff --git a/zuul/driver/timer/timertrigger.py b/zuul/driver/timer/timertrigger.py index a521904992..0e8cd4f0d2 100644 --- a/zuul/driver/timer/timertrigger.py +++ b/zuul/driver/timer/timertrigger.py @@ -23,7 +23,8 @@ from zuul.driver.util import to_list class TimerTrigger(BaseTrigger): name = 'timer' - def getEventFilters(self, connection_name, trigger_conf): + def getEventFilters(self, connection_name, trigger_conf, + error_accumulator): efilters = [] for trigger in to_list(trigger_conf): f = TimerEventFilter(connection_name=connection_name, diff --git a/zuul/driver/zuul/zuultrigger.py b/zuul/driver/zuul/zuultrigger.py index b189d401b8..a33430509a 100644 --- a/zuul/driver/zuul/zuultrigger.py +++ b/zuul/driver/zuul/zuultrigger.py @@ -29,7 +29,8 @@ class ZuulTrigger(BaseTrigger): self._handle_parent_change_enqueued_events = False self._handle_project_change_merged_events = False - def getEventFilters(self, connection_name, trigger_conf): + def getEventFilters(self, connection_name, trigger_conf, + error_accumulator): efilters = [] for trigger in to_list(trigger_conf): f = ZuulEventFilter( diff --git a/zuul/model.py b/zuul/model.py index 47f9443eca..7f83b3a0df 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -334,14 +334,17 @@ class LoadingErrors(object): self.errors = [] self.error_keys = set() - def addError(self, context, mark, error, short_error=None, - severity=None, name=None): + def makeError(self, context, mark, error, short_error=None, + severity=None, name=None): e = ConfigurationError(context, mark, error, short_error=short_error, severity=severity, name=name) - self.errors.append(e) - self.error_keys.add(e.key) + self.addError(e) + + def addError(self, error): + self.errors.append(error) + self.error_keys.add(error.key) def __getitem__(self, index): return self.errors[index] diff --git a/zuul/trigger/__init__.py b/zuul/trigger/__init__.py index 4fb29fffaf..59ef35a47a 100644 --- a/zuul/trigger/__init__.py +++ b/zuul/trigger/__init__.py @@ -26,7 +26,8 @@ class BaseTrigger(object, metaclass=abc.ABCMeta): self.config = config or {} @abc.abstractmethod - def getEventFilters(self, connection_name, trigger_conf): + def getEventFilters(self, connection_name, trigger_conf, + error_accumulator): """Return a list of EventFilter's for the scheduler to match against. """