zuul/zuul
Code Issues Proposed changes

Merge "Provide config error information for dependency cycles"

Browse Source
This commit is contained in:
Zuul
2024-11-04 19:48:24 +00:00
committed by Gerrit Code Review
parent f9e5ece633 c4c13b1ff4
commit e66d06bde5
5 changed files with 119 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
tests/unit/test_circular_dependencies.py
View File

@ -4143,6 +4143,43 @@ class TestGerritCircularDependencies(ZuulTestCase):
dict(name="project-job", result="ABORTED", changes="1,1 2,1"),
], ordered=False)
def test_circular_dependency_config_error(self):
# Use an error that does not leave a file comment to better
# exercise the change matching function.
use_job = textwrap.dedent(
"""
- foo:
project:
""")
A = self.fake_gerrit.addFakeChange("org/project", "master", "A")
B = self.fake_gerrit.addFakeChange("org/project1", "master", "B",
files={"zuul.yaml": use_job})
# A <-> B (via commit-depends)
A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
A.subject, B.data["url"]
)
B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
B.subject, A.data["url"]
)
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([])
self.assertEqual(len(A.patchsets[-1]["approvals"]), 1)
self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified")
self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "-1")
self.assertEqual(len(B.patchsets[-1]["approvals"]), 1)
self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified")
self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "-1")
# Only report the error message to B
self.assertNotIn("more than one key", A.messages[-1])
self.assertIn("more than one key", B.messages[-1])
# Indicate that B has an error
self.assertIn("/2 (config error)", A.messages[-1])
self.assertIn("/2 (config error)", B.messages[-1])
class TestGithubCircularDependencies(ZuulTestCase):
config_file = "zuul-gerrit-github.conf"

2
zuul/driver/gerrit/gerritreporter.py
View File

@ -69,7 +69,7 @@ class GerritReporter(BaseReporter):
comments = self.getFileComments(item, change)
if self._create_comment:
message = self._formatItemReport(item)
message = self._formatItemReport(item, change=change)
else:
message = ''

2
zuul/driver/gitlab/gitlabreporter.py
View File

@ -76,7 +76,7 @@ class GitlabReporter(BaseReporter):
if phase2 and self._merge:
self.mergeMR(item, change)
if not change.is_merged:
msg = self._formatItemReportMergeConflict(item)
msg = self._formatItemReportMergeConflict(item, change)
self.addMRComment(item, change, msg)
def addMRComment(self, item, change, comment=None):

2
zuul/driver/pagure/pagurereporter.py
View File

@ -80,7 +80,7 @@ class PagureReporter(BaseReporter):
if phase2 and self._merge:
self.mergePull(item, change)
if not change.is_merged:
msg = self._formatItemReportMergeConflict(item)
msg = self._formatItemReportMergeConflict(item, change)
self.addPullComment(item, change, msg)
def _formatItemReportJobs(self, item):

114
zuul/reporter/__init__.py
View File

@ -60,6 +60,19 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
def postConfig(self):
"""Run tasks after configuration is reloaded"""
def _configErrorMatchesChange(self, err, change):
context = err.key.context
if not context:
return False
if context.project_canonical_name != \
change.project.canonical_name:
return False
if not hasattr(change, 'branch'):
return False
if context.branch != change.branch:
return False
return True
def addConfigurationErrorComments(self, item, change, comments):
"""Add file comments for configuration errors.
@ -75,17 +88,12 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
for err in item.getConfigErrors():
context = err.key.context
mark = err.key.mark
if not (context and mark and err.short_error):
continue
if context.project_canonical_name != \
change.project.canonical_name:
continue
if not hasattr(change, 'branch'):
continue
if context.branch != change.branch:
if not self._configErrorMatchesChange(err, change):
continue
if not (mark and err.short_error):
return False
if context.path not in change.files:
continue
return False
existing_comments = comments.setdefault(context.path, [])
existing_comments.append(dict(line=mark.end_line,
message=err.short_error,
@ -147,11 +155,12 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
}
return format_methods[action]
def _formatItemReport(self, item, with_jobs=True, action=None):
def _formatItemReport(self, item, change=None,
with_jobs=True, action=None):
"""Format a report from the given items. Usually to provide results to
a reporter taking free-form text."""
action = action or self._action
ret = self._getFormatter(action)(item, with_jobs)
ret = self._getFormatter(action)(item, change, with_jobs)
config_warnings = item.getConfigErrors(errors=False, warnings=True)
if config_warnings:
@ -170,7 +179,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
return ret
def _formatItemReportEnqueue(self, item, with_jobs=True):
def _formatItemReportEnqueue(self, item, change, with_jobs=True):
if status_url := self.connection.sched.globals.web_status_url:
status_url = item.formatUrlPattern(status_url)
@ -182,7 +191,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
item_url=item.formatItemUrl(),
status_url=status_url)
def _formatItemReportStart(self, item, with_jobs=True):
def _formatItemReportStart(self, item, change, with_jobs=True):
if status_url := self.connection.sched.globals.web_status_url:
status_url = item.formatUrlPattern(status_url)
@ -194,7 +203,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
item_url=item.formatItemUrl(),
status_url=status_url)
def _formatItemReportSuccess(self, item, with_jobs=True):
def _formatItemReportSuccess(self, item, change, with_jobs=True):
msg = item.pipeline.success_message
if with_jobs:
item_url = item.formatItemUrl()
@ -203,24 +212,43 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
msg += '\n\n' + self._formatItemReportJobs(item)
return msg
def _formatItemReportFailure(self, item, with_jobs=True):
def _formatItemReportFailure(self, item, change, with_jobs=True):
if len(item.changes) > 1:
change_text = 'These changes'
_this_change = 'These changes'
_depends = 'depend'
_is = 'are'
else:
change_text = 'This change'
_this_change = 'This change'
_depends = 'depends'
_is = 'is'
if item.dequeued_needing_change:
msg = f'{change_text} depends on a change that failed to merge.\n'
msg = (f'{_this_change} {_depends} on a change '
'that failed to merge.\n')
if isinstance(item.dequeued_needing_change, str):
msg += '\n' + item.dequeued_needing_change + '\n'
elif item.dequeued_missing_requirements:
msg = (f'{change_text} is unable to merge '
msg = (f'{_this_change} {_is} unable to merge '
'due to a missing merge requirement.\n')
elif len(item.changes) > 1:
msg = f'{change_text} is part of a dependency cycle that failed.\n'
# No plural phrasing here; we are specifically talking
# about this change as part of a cycle.
msg = ('This change is part of a dependency cycle '
'that failed.\n\n')
# Attempt to provide relevant config error information
# since we will not add it below.
changes_with_errors = self._getChangesWithErrors(item)
if change in changes_with_errors:
msg += str(item.getConfigErrors(
errors=True, warnings=False)[0].error)
change_annotations = {c: ' (config error)'
for c in changes_with_errors}
if with_jobs:
msg = '{}\n\n{}'.format(msg, self._formatItemReportJobs(item))
msg = "{}\n\n{}".format(
msg, self._formatItemReportOtherChanges(item))
msg = '{}\n{}'.format(msg, self._formatItemReportJobs(item))
msg = "{}\n{}".format(
msg, self._formatItemReportOtherChanges(item,
change_annotations))
elif item.didMergerFail():
msg = item.pipeline.merge_conflict_message
elif item.current_build_set.has_blocking_errors:
@ -235,20 +263,33 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
msg += '\n\n' + self._formatItemReportJobs(item)
return msg
def _formatItemReportMergeConflict(self, item, with_jobs=True):
def _getChangesWithErrors(self, item):
# Attempt to determine whether this change is the source of a
# configuration error. This is best effort.
if not item.current_build_set.has_blocking_errors:
return []
ret = []
for change in item.changes:
for err in item.getConfigErrors():
if self._configErrorMatchesChange(err, change):
ret.append(change)
return ret
def _formatItemReportMergeConflict(self, item, change, with_jobs=True):
return item.pipeline.merge_conflict_message
def _formatItemReportMergeFailure(self, item, with_jobs=True):
def _formatItemReportMergeFailure(self, item, change, with_jobs=True):
return 'This change was not merged by the code review system.\n'
def _formatItemReportConfigError(self, item, with_jobs=True):
def _formatItemReportConfigError(self, item, change, with_jobs=True):
if item.getConfigErrors():
msg = str(item.getConfigErrors()[0].error)
else:
msg = "Unknown configuration error"
return msg
def _formatItemReportNoJobs(self, item, with_jobs=True):
def _formatItemReportNoJobs(self, item, change, with_jobs=True):
if status_url := self.connection.sched.globals.web_status_url:
status_url = item.formatUrlPattern(status_url)
@ -260,23 +301,26 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
item_url=item.formatItemUrl(),
status_url=status_url)
def _formatItemReportDisabled(self, item, with_jobs=True):
def _formatItemReportDisabled(self, item, change, with_jobs=True):
if item.current_build_set.result == 'SUCCESS':
return self._formatItemReportSuccess(item)
return self._formatItemReportSuccess(item, change)
elif item.current_build_set.result == 'FAILURE':
return self._formatItemReportFailure(item)
return self._formatItemReportFailure(item, change)
else:
return self._formatItemReport(item)
return self._formatItemReport(item, change)
def _formatItemReportDequeue(self, item, with_jobs=True):
def _formatItemReportDequeue(self, item, change, with_jobs=True):
msg = item.pipeline.dequeue_message
if with_jobs:
msg += '\n\n' + self._formatItemReportJobs(item)
return msg
def _formatItemReportOtherChanges(self, item):
return "Related changes:\n{}\n".format("\n".join(
f' - {c.url}' for c in item.changes))
def _formatItemReportOtherChanges(self, item, change_annotations):
change_lines = []
for change in item.changes:
annotation = change_annotations.get(change, '')
change_lines.append(f' - {change.url}{annotation}')
return "Related changes:\n{}\n".format("\n".join(change_lines))
def _getItemReportJobsFields(self, item):
# Extract the report elements from an item