diff --git a/tests/fixtures/config/broken/git/common-config/playbooks/job.yaml b/tests/fixtures/config/broken/git/common-config/playbooks/job.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/broken/git/common-config/playbooks/job.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/broken/git/common-config/zuul.yaml b/tests/fixtures/config/broken/git/common-config/zuul.yaml index abebd27974..50d0e22e12 100644 --- a/tests/fixtures/config/broken/git/common-config/zuul.yaml +++ b/tests/fixtures/config/broken/git/common-config/zuul.yaml @@ -15,6 +15,16 @@ name: base parent: null +- job: + name: central-test + run: playbooks/job.yaml + +- project: + name: common-config + check: + jobs: + - noop + - project: name: org/project2 check: diff --git a/tests/fixtures/config/broken/git/org_project/.zuul.yaml b/tests/fixtures/config/broken/git/org_project/.zuul.yaml index 1fc35b5184..7709812719 100644 --- a/tests/fixtures/config/broken/git/org_project/.zuul.yaml +++ b/tests/fixtures/config/broken/git/org_project/.zuul.yaml @@ -1,3 +1,4 @@ - project: check: - jobs: [] + jobs: + - central-test diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 3d26a229be..307dcf23b6 100755 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -2460,7 +2460,7 @@ class TestBrokenConfig(ZuulTestCase): "An error should have been stored") self.assertIn( "Zuul encountered a syntax error", - str(tenant.layout.loading_errors[0][1])) + str(tenant.layout.loading_errors[0].error)) def test_dynamic_ignore(self): # Verify dynamic config behaviors inside a tenant broken config @@ -2594,6 +2594,54 @@ class TestBrokenConfig(ZuulTestCase): self.assertHistory([ dict(name='project-test2', result='SUCCESS', changes='1,1')]) + def test_dynamic_fail_cross_repo(self): + # Verify dynamic config behaviors inside a tenant broken config + tenant = self.sched.abide.tenants.get('tenant-one') + # There is a configuration error + self.assertEquals( + len(tenant.layout.loading_errors), 1, + "An error should have been stored") + + # Inside a broken tenant configuration environment, remove a + # job used in another repo and verify that an error is + # reported despite the error being in a repo other than the + # change. + in_repo_conf = textwrap.dedent( + """ + - pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + - job: + name: base + parent: null + + - project: + name: common-config + check: + jobs: + - noop + """) + + file_dict = {'zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('common-config', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + self.assertEqual(A.reported, 1, + "A should report failure") + self.assertEqual(A.patchsets[0]['approvals'][0]['value'], "-1") + self.assertIn('Job central-test not defined', A.messages[0], + "A should have failed the check pipeline") + class TestProjectKeys(ZuulTestCase): # Test that we can generate project keys diff --git a/zuul/configloader.py b/zuul/configloader.py index 559e73c410..19acf6e26f 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -233,7 +233,7 @@ def configuration_exceptions(stanza, conf, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.append((context, m)) + accumulator.addError(context, start_mark, m) @contextmanager @@ -269,7 +269,7 @@ def reference_exceptions(stanza, obj, accumulator): content=indent(start_mark.snippet.rstrip()), start_mark=str(start_mark)) - accumulator.append((context, m)) + accumulator.addError(context, start_mark, m) class ZuulMark(object): @@ -1277,7 +1277,7 @@ class TenantParser(object): self._resolveShadowProjects(tenant, tpc) # We prepare a stack to store config loading issues - loading_errors = model.LoadingErrors(length=model.MAX_ERROR_LENGTH) + loading_errors = model.LoadingErrors() # Start by fetching any YAML needed by this tenant which isn't # already cached. Full reconfigurations start with an empty @@ -1572,7 +1572,7 @@ class TenantParser(object): r = safe_load_yaml(data, source_context) config.extend(r) except ConfigurationSyntaxError as e: - loading_errors.append((source_context, e)) + loading_errors.addError(source_context, None, e) return config def filterConfigProjectYAML(self, data): @@ -1601,8 +1601,9 @@ class TenantParser(object): try: pcontext.pragma_parser.fromYaml(config_pragma) except ConfigurationSyntaxError as e: - loading_errors.append( - (config_pragma['_source_context'], e)) + loading_errors.addError( + config_pragma['_source_context'], + config_pragma['_start_mark'], e) for config_pipeline in unparsed_config.pipelines: classes = self._getLoadClasses(tenant, config_pipeline) @@ -1893,8 +1894,8 @@ class ConfigLoader(object): "configuration loading" % ( len(tenant.layout.loading_errors), tenant.name)) # Log accumulated errors - for err in tenant.layout.loading_errors.errors: - self.log.warning(str(err[1])) + for err in tenant.layout.loading_errors.errors[:10]: + self.log.warning(err.error) return abide def reloadTenant(self, project_key_dir, abide, tenant): @@ -1915,8 +1916,8 @@ class ConfigLoader(object): "configuration re-loading" % ( len(new_tenant.layout.loading_errors), tenant.name)) # Log accumulated errors - for err in new_tenant.layout.loading_errors.errors: - self.log.warning(str(err[1])) + for err in new_tenant.layout.loading_errors.errors[:10]: + self.log.warning(err.error) return new_abide def _loadDynamicProjectData(self, config, project, @@ -1983,7 +1984,7 @@ class ConfigLoader(object): def createDynamicLayout(self, tenant, files, include_config_projects=False, scheduler=None, connections=None): - loading_errors = model.LoadingErrors(length=model.MAX_ERROR_LENGTH) + loading_errors = model.LoadingErrors() if include_config_projects: config = model.ParsedConfig() for project in tenant.config_projects: diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 464891bfa3..579273c2fc 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -419,6 +419,15 @@ class PipelineManager(object): self.sched.connections, self.sched, None) self.log.debug("Loading dynamic layout") + + parent_layout = None + parent = item.item_ahead + while not parent_layout and parent: + parent_layout = parent.layout + parent = parent.item_ahead + if not parent_layout: + parent_layout = item.pipeline.tenant.layout + (trusted_updates, untrusted_updates) = item.includesConfigUpdates() build_set = item.current_build_set trusted_layout_verified = False @@ -472,11 +481,13 @@ class PipelineManager(object): # the current item.change and only report # if one is found. for err in layout.loading_errors.errors: - context = err[0] - if context.project.name == item.change.project.name: - if context.branch == item.change.branch: - item.setConfigError(str(err[1])) - return None + econtext = err.key.context + if ((err.key not in + parent_layout.loading_errors.error_keys) or + (econtext.project == item.change.project.name and + econtext.branch == item.change.branch)): + item.setConfigError(err.error) + return None self.log.info( "Configuration syntax error not related to " "change context. Error won't be reported.") diff --git a/zuul/model.py b/zuul/model.py index 869662c678..caf909ff87 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -81,23 +81,70 @@ NODE_STATES = set([STATE_BUILDING, STATE_HOLD, STATE_DELETING]) -MAX_ERROR_LENGTH = 10 + +class ConfigurationErrorKey(object): + """A class which attempts to uniquely identify configuration errors + based on their file location. It's not perfect, but it's usually + sufficient to determine whether we should show an error to a user. + """ + + def __init__(self, context, mark, error_text): + self.context = context + self.mark = mark + self.error_text = error_text + elements = [] + if context: + elements.extend([ + context.project.canonical_name, + context.branch, + context.path, + ]) + else: + elements.extend([None, None, None]) + if mark: + elements.extend([ + mark.line, + mark.snippet, + ]) + else: + elements.extend([None, None]) + elements.append(error_text) + self._hash = hash('|'.join([str(x) for x in elements])) + + def __hash__(self): + return self._hash + + def __ne__(self, other): + return not self.__eq__(other) + + def __eq__(self, other): + if not isinstance(other, ConfigurationErrorKey): + return False + return (self.context == other.context and + self.mark.line == other.mark.line and + self.mark.snippet == other.mark.snippet and + self.error_text == other.error_text) + + +class ConfigurationError(object): + + """A configuration error""" + def __init__(self, context, mark, error): + self.error = str(error) + self.key = ConfigurationErrorKey(context, mark, self.error) class LoadingErrors(object): """A configuration errors accumalator attached to a layout object """ - def __init__(self, length): - self.length = length + def __init__(self): self.errors = [] + self.error_keys = set() - def append(self, error): - if len(self.errors) < self.length: - self.errors.append(error) - - def extend(self, errors): - for err in errors: - self.append(err) + def addError(self, context, mark, error): + e = ConfigurationError(context, mark, error) + self.errors.append(e) + self.error_keys.add(e.key) def __getitem__(self, index): return self.errors[index] @@ -2882,8 +2929,7 @@ class Layout(object): self.nodesets = {} self.secrets = {} self.semaphores = {} - self.loading_errors = LoadingErrors( - length=MAX_ERROR_LENGTH) + self.loading_errors = LoadingErrors() def getJob(self, name): if name in self.jobs: diff --git a/zuul/rpclistener.py b/zuul/rpclistener.py index d329cc6d20..a756bedd17 100644 --- a/zuul/rpclistener.py +++ b/zuul/rpclistener.py @@ -364,6 +364,6 @@ class RPCListener(object): output = [] for err in tenant.layout.loading_errors.errors: output.append({ - 'source_context': err[0].toDict(), - 'error': err[1]}) + 'source_context': err.key.context.toDict(), + 'error': err.error}) job.sendWorkComplete(json.dumps(output))