Allow new warnings when errors exist
If a configuration error existed for a project on one branch and a change was proposed to update the config on another branch, that would activate a code path in the manager which attempts to determine whether errors are relevant. An error (or warning) is relevant if it is not in a parent change, and is on the same project+branch as the current patch. This is pretty generous. This means that if a patch touches Zuul configuration with a warning, all warnings on that branch must be updated. This was not the intended behavior. To correct that, we no longer consider warnings in any of the places where we check that a queue item is failing due to configuration errors. An existing test is updated to include sufficient setup to trigger the case where a new valid configuration is added to a project with existing errors and warnings. A new test case is added to show that we can add new deprecations as well, and that they are reported to users as warnings. Change-Id: Id901a540fce7be6fedae668390418aca06a950af
This commit is contained in:
James E. Blair
parent
4effa487f5
commit
267f675533
@ -5235,10 +5235,12 @@ class TestValidateWarnings(ZuulTestCase):
|
||||
|
||||
|
||||
class TestValidateWarningsAcceptable(ZuulTestCase):
|
||||
# Test we don't fail new configs when warnings exist
|
||||
# Test we don't fail new configs when warnings and errors exist
|
||||
|
||||
@simple_layout('layouts/pcre-deprecation.yaml')
|
||||
def test_validate_warning_new_config(self):
|
||||
# Test that we can add new valid configuration despite the
|
||||
# existence of existing errors and warnings.
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
errors = tenant.layout.loading_errors
|
||||
self.assertEqual(len(errors), 3)
|
||||
@ -5246,7 +5248,47 @@ class TestValidateWarningsAcceptable(ZuulTestCase):
|
||||
for error in errors:
|
||||
self.assertEqual(error.severity, SEVERITY_WARNING)
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
# Put an error on a different branch. This ensures that we
|
||||
# will always have configuration errors, but they are not
|
||||
# relevant, so should not affect further changes.
|
||||
self.create_branch('org/project', 'stable/queens')
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project', 'stable/queens'))
|
||||
self.waitUntilSettled()
|
||||
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- nonexistent-job
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.yaml': in_repo_conf}
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'stable/queens', 'A',
|
||||
files=file_dict)
|
||||
A.setMerged()
|
||||
self.fake_gerrit.addEvent(A.getChangeMergedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Put an existing warning on this branch.
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: broken-job
|
||||
branches: ^(?!invalid).*$
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.d/broken.yaml': in_repo_conf}
|
||||
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B',
|
||||
files=file_dict)
|
||||
B.setMerged()
|
||||
self.fake_gerrit.addEvent(B.getChangeMergedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Make sure we can add a new job with an existing error on
|
||||
# another branch and warning on this one.
|
||||
conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
@ -5256,13 +5298,77 @@ class TestValidateWarningsAcceptable(ZuulTestCase):
|
||||
name: org/project
|
||||
check:
|
||||
jobs:
|
||||
- check-job
|
||||
- new-job
|
||||
""")
|
||||
|
||||
file_dict = {'.zuul.yaml': conf}
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
|
||||
file_dict = {'zuul.d/new.yaml': conf}
|
||||
C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C',
|
||||
files=file_dict)
|
||||
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name='check-job', result='SUCCESS', changes='3,1'),
|
||||
# New job runs despite warnings existing in the config.
|
||||
dict(name='new-job', result='SUCCESS', changes='3,1'),
|
||||
], ordered=False)
|
||||
|
||||
@simple_layout('layouts/pcre-deprecation.yaml')
|
||||
def test_validate_new_warning(self):
|
||||
# Test that we can add new deprecated configuration.
|
||||
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
errors = tenant.layout.loading_errors
|
||||
self.assertEqual(len(errors), 3)
|
||||
# Make note of the warnings in our config.
|
||||
for error in errors:
|
||||
self.assertEqual(error.severity, SEVERITY_WARNING)
|
||||
|
||||
# Put an error on a different branch. This ensures that we
|
||||
# will always have configuration errors, but they are not
|
||||
# relevant, so should not affect further changes.
|
||||
self.create_branch('org/project', 'stable/queens')
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project', 'stable/queens'))
|
||||
self.waitUntilSettled()
|
||||
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- nonexistent-job
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.yaml': in_repo_conf}
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'stable/queens', 'A',
|
||||
files=file_dict)
|
||||
A.setMerged()
|
||||
self.fake_gerrit.addEvent(A.getChangeMergedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Add a new configuration warning.
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: new-job
|
||||
branches: ^(?!invalid).*$
|
||||
|
||||
- project:
|
||||
name: org/project
|
||||
check:
|
||||
jobs:
|
||||
- check-job
|
||||
- new-job
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.yaml': conf}
|
||||
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B',
|
||||
files=file_dict)
|
||||
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# check-job and new-job
|
||||
@ -5271,10 +5377,11 @@ class TestValidateWarningsAcceptable(ZuulTestCase):
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertIn('encountered a deprecated syntax', B.messages[-1])
|
||||
self.assertHistory([
|
||||
dict(name='check-job', result='SUCCESS', changes='1,1'),
|
||||
dict(name='check-job', result='SUCCESS', changes='2,1'),
|
||||
# New job runs despite warnings existing in the config.
|
||||
dict(name='new-job', result='SUCCESS', changes='1,1'),
|
||||
dict(name='new-job', result='SUCCESS', changes='2,1'),
|
||||
], ordered=False)
|
||||
|
||||
|
||||
|
||||
@ -347,7 +347,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
def reportNormalBuildsetEnd(self, build_set, action, final, result=None):
|
||||
# Report a buildset end if there are jobs or errors
|
||||
if ((build_set.job_graph and len(build_set.job_graph.jobs) > 0) or
|
||||
build_set.config_errors or
|
||||
build_set.has_blocking_errors or
|
||||
build_set.unable_to_merge):
|
||||
self.sql.reportBuildsetEnd(build_set, action,
|
||||
final, result)
|
||||
@ -504,7 +504,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
# Similarly, reset the item state.
|
||||
if item.current_build_set.unable_to_merge:
|
||||
item.setUnableToMerge()
|
||||
if item.current_build_set.config_errors:
|
||||
if item.current_build_set.has_blocking_errors:
|
||||
item.setConfigErrors(item.getConfigErrors())
|
||||
if item.dequeued_needing_change:
|
||||
item.setDequeuedNeedingChange(item.dequeued_needing_change)
|
||||
@ -1459,7 +1459,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
# Next, if a change ahead has a broken config, then so does
|
||||
# this one. Record that and don't do anything else.
|
||||
if (item.item_ahead and item.item_ahead.current_build_set and
|
||||
item.item_ahead.current_build_set.config_errors):
|
||||
item.item_ahead.current_build_set.has_blocking_errors):
|
||||
msg = "This change depends on a change "\
|
||||
"with an invalid configuration.\n"
|
||||
item.setConfigError(msg)
|
||||
@ -1516,7 +1516,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
|
||||
# If the change can not be merged or has config errors, don't
|
||||
# run jobs.
|
||||
if build_set.unable_to_merge or build_set.config_errors:
|
||||
if build_set.unable_to_merge or build_set.has_blocking_errors:
|
||||
# Find our layout since the reporter will need it to
|
||||
# determine if the project is in the pipeline.
|
||||
self.getLayout(item)
|
||||
@ -1673,7 +1673,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
reported_start=True)
|
||||
if item.current_build_set.unable_to_merge:
|
||||
failing_reasons.append("it has a merge conflict")
|
||||
if item.current_build_set.config_errors:
|
||||
if item.current_build_set.has_blocking_errors:
|
||||
failing_reasons.append("it has an invalid configuration")
|
||||
if ready and self.provisionNodes(item):
|
||||
changed = True
|
||||
@ -2185,7 +2185,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
action = 'no-jobs'
|
||||
actions = self.pipeline.no_jobs_actions
|
||||
item.setReportedResult('NO_JOBS')
|
||||
elif item.getConfigErrors(warnings=False):
|
||||
elif item.current_build_set.has_blocking_errors:
|
||||
log.debug("Invalid config for change %s", item.change)
|
||||
action = 'config-error'
|
||||
actions = self.pipeline.config_error_actions
|
||||
|
||||
@ -4353,6 +4353,14 @@ class BuildSet(zkobject.ZKObject):
|
||||
_path=path)
|
||||
self.config_errors = el
|
||||
|
||||
@property
|
||||
def has_blocking_errors(self):
|
||||
if not self.config_errors:
|
||||
return False
|
||||
errs = filter_severity(self.config_errors.errors,
|
||||
errors=True, warnings=False)
|
||||
return bool(errs)
|
||||
|
||||
def setMergeRepoState(self, repo_state):
|
||||
if self.merge_repo_state is not None:
|
||||
raise Exception("Merge repo state can not be updated")
|
||||
@ -5179,7 +5187,7 @@ class QueueItem(zkobject.ZKObject):
|
||||
return True
|
||||
|
||||
def areAllJobsComplete(self):
|
||||
if (self.current_build_set.config_errors or
|
||||
if (self.current_build_set.has_blocking_errors or
|
||||
self.current_build_set.unable_to_merge):
|
||||
return True
|
||||
if not self.hasJobGraph():
|
||||
|
||||
@ -148,6 +148,10 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
|
||||
action = action or self._action
|
||||
ret = self._getFormatter(action)(item, with_jobs)
|
||||
|
||||
config_warnings = item.getConfigErrors(errors=False, warnings=True)
|
||||
if config_warnings:
|
||||
ret += '\nWarning:\n ' + config_warnings[0].error + '\n'
|
||||
|
||||
if item.current_build_set.warning_messages:
|
||||
warning = '\n '.join(item.current_build_set.warning_messages)
|
||||
ret += '\nWarning:\n ' + warning + '\n'
|
||||
@ -210,7 +214,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
|
||||
msg, self._formatItemReportOtherBundleItems(item))
|
||||
elif item.didMergerFail():
|
||||
msg = item.pipeline.merge_conflict_message
|
||||
elif item.getConfigErrors(errors=True, warnings=False):
|
||||
elif item.current_build_set.has_blocking_errors:
|
||||
msg = str(item.getConfigErrors(
|
||||
errors=True, warnings=False)[0].error)
|
||||
else:
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user