Provide config error information for dependency cycles

When a dependency cycle causes a config error and reports, we omit information
about the error itself since the same message is used for all changes in the
cycle, and it may be confusing to leave a config error message on a change that
is not directly responsible for the error.

However, this leaves something to be desired in that the user must then
closely examine each change, or look for line comments, to determine the source
of the error.

To improve this, we will now generate the reporting message with the actual
change that is being reported in mind.  This will allow us to leave an extended
message on the change that directly caused the error, and an abbreviated one on
the other changes in the cycle.

To further help users get to the right change, we will also annotate the list
of changes in the cycle to indicate which ones have config errors.

The result will look something like this for a change in a cycle
without a config error (this report has no file comments):

    This change is part of a dependency cycle that failed.

    Related changes:
      - http://localhost:55777/1
      - http://localhost:55777/2 (config error)

And like this for the other change with the error (this report
would have file comments if any were generated, but not in the
 case of this specific error):

    Zuul encountered a syntax error while parsing its
    configuration in the repo org/project1 on branch master.  The
    problem was:

      Configuration item has more than one key.  Each zuul.yaml
      configuration file must be a list of dictionaries with a
      single key, for example:

      - job:
          name: foo

      - project:
          name: bar

      Ensure that every item in the list is a dictionary with only
      one key (in this example, 'job' and 'project').  This error
      may be caused by insufficient indentation of the keys under
      the configuration item ('name' in this example).

      The incorrect values are around:

      foo: null
      project: null

    Related changes:
      - http://localhost:55777/1
      - http://localhost:55777/2 (config error)

Change-Id: Ib1804786406dd841533cc3fab31af37ec3449b69

tests/unit/test_circular_dependencies.py

@@ -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"

zuul/driver/gerrit/gerritreporter.py

@@ -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 = ''
 

zuul/driver/gitlab/gitlabreporter.py

@@ -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):

zuul/driver/pagure/pagurereporter.py

@@ -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):

zuul/reporter/__init__.py

@@ -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