Add detail to "depends on a change that failed to merge"

The report message "This change depends on a change that failed to
merge" (and a similar change for circular dependency bundles) is
famously vague.  To help users identify the actual problem, include
URLs for which change(s) caused the problem so that users may more
easily resolve the issue.

Change-Id: Id8b9f8cf2c108703e9209e30bdc9a3933f074652

6 files changed, 136 insertions, 65 deletions

diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py
index 2223008d5..292941c13 100644
--- a/tests/unit/test_circular_dependencies.py
+++ b/tests/unit/test_circular_dependencies.py
@@ -13,6 +13,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this software.  If not, see <http://www.gnu.org/licenses/>.
 
+import re
 import textwrap
 
 from zuul.model import PromoteEvent
@@ -599,8 +600,23 @@ class TestGerritCircularDependencies(ZuulTestCase):
 
         self.assertIn("depends on a change that failed to merge",
                       A.messages[-1])
+        self.assertTrue(re.search(r'Change http://localhost:\d+/2 is needed',
+                        A.messages[-1]))
+        self.assertFalse(re.search('Change .*? can not be merged',
+                         A.messages[-1]))
+
         self.assertIn("bundle that failed.", B.messages[-1])
+        self.assertFalse(re.search('Change http://localhost:.*? is needed',
+                         B.messages[-1]))
+        self.assertFalse(re.search('Change .*? can not be merged',
+                         B.messages[-1]))
+
         self.assertIn("bundle that failed.", C.messages[-1])
+        self.assertFalse(re.search('Change http://localhost:.*? is needed',
+                         C.messages[-1]))
+        self.assertFalse(re.search('Change .*? can not be merged',
+                         C.messages[-1]))
+
         self.assertEqual(A.data["status"], "NEW")
         self.assertEqual(B.data["status"], "NEW")
         self.assertEqual(C.data["status"], "NEW")
@@ -2441,3 +2457,21 @@ class TestGithubCircularDependencies(ZuulTestCase):
         self.assertEqual(len(B.comments), 2)
         self.assertFalse(A.is_merged)
         self.assertFalse(B.is_merged)
+
+        self.assertIn("part of a bundle that can not merge",
+                      A.comments[-1])
+        self.assertTrue(
+            re.search("Change https://github.com/gh/project/pull/1 "
+                      "can not be merged",
+                      A.comments[-1]))
+        self.assertFalse(re.search('Change .*? is needed',
+                                   A.comments[-1]))
+
+        self.assertIn("part of a bundle that can not merge",
+                      B.comments[-1])
+        self.assertTrue(
+            re.search("Change https://github.com/gh/project/pull/1 "
+                      "can not be merged",
+                      B.comments[-1]))
+        self.assertFalse(re.search('Change .*? is needed',
+                                   B.comments[-1]))
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index a66f5ad22..31c4b54d7 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -379,9 +379,16 @@ class PipelineManager(metaclass=ABCMeta):
                              dependency_graph=None):
         return True
 
-    def checkForChangesNeededBy(self, change, change_queue, event,
+    def getMissingNeededChanges(self, change, change_queue, event,
                                 dependency_graph=None):
-        return True
+        """Check that all needed changes are ahead in the queue.
+
+        Return a list of any that are missing.  If it is not possible
+        to correct the missing changes, "abort" will be true.
+
+        :returns: (abort, needed_changes)
+        """
+        return False, []
 
     def getFailingDependentItems(self, item, nnfi):
         return None
@@ -690,9 +697,11 @@ class PipelineManager(metaclass=ABCMeta):
 
         return queue_config.dependencies_by_topic
 
-    def canMergeCycle(self, bundle):
-        """Check if the cycle still fulfills the pipeline's ready criteria."""
-        return True
+    def getNonMergeableCycleChanges(self, bundle):
+
+        """Return changes in the cycle that do not fulfill
+        the pipeline's ready criteria."""
+        return []
 
     def updateBundle(self, item, change_queue, cycle):
         if not cycle:
@@ -1474,9 +1483,9 @@ class PipelineManager(metaclass=ABCMeta):
                 item.change, item.event)
         else:
             meets_reqs = True
-        needs_met = self.checkForChangesNeededBy(item.change, change_queue,
-                                                 item.event) is True
-        if not (meets_reqs and needs_met):
+        abort, needs_changes = self.getMissingNeededChanges(
+            item.change, change_queue, item.event)
+        if not (meets_reqs and not needs_changes):
             # It's not okay to enqueue this change, we should remove it.
             log.info("Dequeuing change %s because "
                      "it can no longer merge" % item.change)
@@ -1486,7 +1495,12 @@ class PipelineManager(metaclass=ABCMeta):
             elif not meets_reqs:
                 item.setDequeuedMissingRequirements()
             else:
-                item.setDequeuedNeedingChange()
+                clist = ', '.join([c.url for c in needs_changes])
+                if len(needs_changes) > 1:
+                    msg = f'Changes {clist} are needed.'
+                else:
+                    msg = f'Change {clist} is needed.'
+                item.setDequeuedNeedingChange(msg)
             if item.live:
                 try:
                     self.reportItem(item)
@@ -1558,17 +1572,22 @@ class PipelineManager(metaclass=ABCMeta):
             )
             # Before starting to merge the cycle items, make sure they
             # can still be merged, to reduce the chance of a partial merge.
-            if (
-                can_report
-                and not item.bundle.started_reporting
-                and not self.canMergeCycle(item.bundle)
-            ):
-                item.bundle.cannot_merge = True
-                failing_reasons.append("cycle can not be merged")
-                log.debug(
-                    "Dequeuing item %s because cycle can no longer merge",
-                    item
-                )
+            if can_report and not item.bundle.started_reporting:
+                non_mergeable_cycle_changes = self.getNonMergeableCycleChanges(
+                    item.bundle)
+                if non_mergeable_cycle_changes:
+                    clist = ', '.join([
+                        c.url for c in non_mergeable_cycle_changes])
+                    if len(non_mergeable_cycle_changes) > 1:
+                        msg = f'Changes {clist} can not be merged.'
+                    else:
+                        msg = f'Change {clist} can not be merged.'
+                    item.bundle.cannot_merge = msg
+                    failing_reasons.append("cycle can not be merged")
+                    log.debug(
+                        "Dequeuing item %s because cycle can no longer merge",
+                        item
+                    )
             item.bundle.started_reporting = can_report
 
         if can_report:
diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py
index d4d4f05dd..db1bf48b1 100644
--- a/zuul/manager/dependent.py
+++ b/zuul/manager/dependent.py
@@ -56,8 +56,10 @@ class DependentPipelineManager(SharedQueuePipelineManager):
             return False
         return True
 
-    def canMergeCycle(self, bundle):
-        """Check if the cycle still fulfills the pipeline's ready criteria."""
+    def getNonMergeableCycleChanges(self, bundle):
+        """Return changes in the cycle that do not fulfill
+        the pipeline's ready criteria."""
+        changes = []
         for item in bundle.items:
             source = item.change.project.source
             if not source.canMerge(
@@ -68,8 +70,8 @@ class DependentPipelineManager(SharedQueuePipelineManager):
             ):
                 log = get_annotated_logger(self.log, item.event)
                 log.debug("Change %s can no longer be merged", item.change)
-                return False
-        return True
+                changes.append(item.change)
+        return changes
 
     def enqueueChangesBehind(self, change, event, quiet, ignore_requirements,
                              change_queue, history=None,
@@ -149,13 +151,17 @@ class DependentPipelineManager(SharedQueuePipelineManager):
             # Don't enqueue dependencies ahead of a non-change ref.
             return True
 
-        ret = self.checkForChangesNeededBy(change, change_queue, event,
-                                           dependency_graph=dependency_graph,
-                                           warnings=warnings)
-        if ret in [True, False]:
-            return ret
-        log.debug("  Changes %s must be merged ahead of %s", ret, change)
-        for needed_change in ret:
+        abort, needed_changes = self.getMissingNeededChanges(
+            change, change_queue, event,
+            dependency_graph=dependency_graph,
+            warnings=warnings)
+        if abort:
+            return False
+        if not needed_changes:
+            return True
+        log.debug("  Changes %s must be merged ahead of %s",
+                  needed_changes, change)
+        for needed_change in needed_changes:
             # If the change is already in the history, but the change also has
             # a git level dependency, we need to enqueue it before the current
             # change.
@@ -169,7 +175,7 @@ class DependentPipelineManager(SharedQueuePipelineManager):
                     return False
         return True
 
-    def checkForChangesNeededBy(self, change, change_queue, event,
+    def getMissingNeededChanges(self, change, change_queue, event,
                                 dependency_graph=None, warnings=None):
         log = get_annotated_logger(self.log, event)
 
@@ -178,11 +184,12 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         log.debug("Checking for changes needed by %s:" % change)
         if not hasattr(change, 'needs_changes'):
             log.debug("  %s does not support dependencies", type(change))
-            return True
+            return False, []
         if not change.needs_changes:
             log.debug("  No changes needed")
-            return True
+            return False, []
         changes_needed = []
+        abort = False
         # Ignore supplied change_queue
         with self.getChangeQueue(change, event) as change_queue:
             for needed_change in self.resolveChangeReferences(
@@ -212,10 +219,12 @@ class DependentPipelineManager(SharedQueuePipelineManager):
                         log.debug("  " + msg)
                         if warnings is not None:
                             warnings.append(msg)
-                        return False
+                        changes_needed.append(needed_change)
+                        abort = True
                 if not needed_change.is_current_patchset:
                     log.debug("  Needed change is not the current patchset")
-                    return False
+                    changes_needed.append(needed_change)
+                    abort = True
                 if self.isChangeAlreadyInQueue(needed_change, change_queue):
                     log.debug("  Needed change is already ahead in the queue")
                     continue
@@ -229,10 +238,9 @@ class DependentPipelineManager(SharedQueuePipelineManager):
                 # The needed change can't be merged.
                 log.debug("  Change %s is needed but can not be merged",
                           needed_change)
-                return False
-        if changes_needed:
-            return changes_needed
-        return True
+                changes_needed.append(needed_change)
+                abort = True
+        return abort, changes_needed
 
     def getFailingDependentItems(self, item, nnfi):
         if not hasattr(item.change, 'needs_changes'):
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index 7cd14ffd5..b70e9184b 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -49,12 +49,16 @@ class IndependentPipelineManager(PipelineManager):
             # Don't enqueue dependencies ahead of a non-change ref.
             return True
 
-        ret = self.checkForChangesNeededBy(change, change_queue, event,
-                                           dependency_graph=dependency_graph)
-        if ret in [True, False]:
-            return ret
-        log.debug("  Changes %s must be merged ahead of %s" % (ret, change))
-        for needed_change in ret:
+        abort, needed_changes = self.getMissingNeededChanges(
+            change, change_queue, event,
+            dependency_graph=dependency_graph)
+        if abort:
+            return False
+        if not needed_changes:
+            return True
+        log.debug("  Changes %s must be merged ahead of %s" % (
+            needed_changes, change))
+        for needed_change in needed_changes:
             # This differs from the dependent pipeline by enqueuing
             # changes ahead as "not live", that is, not intended to
             # have jobs run.  Also, pipeline requirements are always
@@ -69,22 +73,23 @@ class IndependentPipelineManager(PipelineManager):
                     return False
         return True
 
-    def checkForChangesNeededBy(self, change, change_queue, event,
+    def getMissingNeededChanges(self, change, change_queue, event,
                                 dependency_graph=None):
         log = get_annotated_logger(self.log, event)
 
         if self.pipeline.ignore_dependencies:
-            return True
+            return False, []
         log.debug("Checking for changes needed by %s:" % change)
         # Return true if okay to proceed enqueing this change,
         # false if the change should not be enqueued.
         if not hasattr(change, 'needs_changes'):
             log.debug("  %s does not support dependencies" % type(change))
-            return True
+            return False, []
         if not change.needs_changes:
             log.debug("  No changes needed")
-            return True
+            return False, []
         changes_needed = []
+        abort = False
         for needed_change in self.resolveChangeReferences(
                 change.needs_changes):
             log.debug("  Change %s needs change %s:" % (
@@ -108,9 +113,7 @@ class IndependentPipelineManager(PipelineManager):
                 continue
             # This differs from the dependent pipeline check in not
             # verifying that the dependent change is mergable.
-        if changes_needed:
-            return changes_needed
-        return True
+        return abort, changes_needed
 
     def dequeueItem(self, item):
         super(IndependentPipelineManager, self).dequeueItem(item)
diff --git a/zuul/model.py b/zuul/model.py
index 963332826..0d889f557 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -4237,7 +4237,7 @@ class QueueItem(zkobject.ZKObject):
             pipeline=None,
             queue=None,
             change=None,  # a ref
-            dequeued_needing_change=False,
+            dequeued_needing_change=None,
             dequeued_missing_requirements=False,
             current_build_set=None,
             item_ahead=None,
@@ -4405,11 +4405,14 @@ class QueueItem(zkobject.ZKObject):
         self.current_build_set.updateAttributes(
             self.pipeline.manager.current_context, result=result)
 
-    def warning(self, msg):
+    def warning(self, msgs):
         with self.current_build_set.activeContext(
                 self.pipeline.manager.current_context):
-            self.current_build_set.warning_messages.append(msg)
-        self.log.info(msg)
+            if not isinstance(msgs, list):
+                msgs = [msgs]
+            for msg in msgs:
+                self.current_build_set.warning_messages.append(msg)
+                self.log.info(msg)
 
     def freezeJobGraph(self, layout, context,
                        skip_file_matcher,
@@ -4583,7 +4586,7 @@ class QueueItem(zkobject.ZKObject):
 
     def cannotMergeBundle(self):
         if self.bundle:
-            return self.bundle.cannot_merge
+            return bool(self.bundle.cannot_merge)
         return False
 
     def didMergerFail(self):
@@ -4595,7 +4598,7 @@ class QueueItem(zkobject.ZKObject):
         return []
 
     def wasDequeuedNeedingChange(self):
-        return self.dequeued_needing_change
+        return bool(self.dequeued_needing_change)
 
     def wasDequeuedMissingRequirements(self):
         return self.dequeued_missing_requirements
@@ -5118,10 +5121,10 @@ class QueueItem(zkobject.ZKObject):
         self.setResult(fakebuild)
         return fakebuild
 
-    def setDequeuedNeedingChange(self):
+    def setDequeuedNeedingChange(self, msg):
         self.updateAttributes(
             self.pipeline.manager.current_context,
-            dequeued_needing_change=True)
+            dequeued_needing_change=msg)
         self._setAllJobsSkipped()
 
     def setDequeuedMissingRequirements(self):
@@ -5494,7 +5497,7 @@ class Bundle:
         self.items = []
         self.started_reporting = False
         self.failed_reporting = False
-        self.cannot_merge = False
+        self.cannot_merge = None
 
     def __repr__(self):
         return '<Bundle 0x{:x} {}'.format(id(self), self.items)
diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py
index 5723316a3..9cd60bff6 100644
--- a/zuul/reporter/__init__.py
+++ b/zuul/reporter/__init__.py
@@ -191,9 +191,13 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
 
     def _formatItemReportFailure(self, item, with_jobs=True):
         if item.cannotMergeBundle():
-            msg = 'This change is part of a bundle that failed to merge.\n'
+            msg = 'This change is part of a bundle that can not merge.\n'
+            if isinstance(item.bundle.cannot_merge, str):
+                msg += '\n' + item.bundle.cannot_merge + '\n'
         elif item.dequeued_needing_change:
             msg = '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 = ('This change is unable to merge '
                    'due to a missing merge requirement.\n')
@@ -250,8 +254,8 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
     def _formatItemReportOtherBundleItems(self, item):
         related_changes = item.pipeline.manager.resolveChangeReferences(
             item.change.needs_changes)
-        return "Related changes:\n{}".format("\n".join(
-            c.url for c in related_changes if c is not item.change))
+        return "Related changes:\n{}\n".format("\n".join(
+            f'  - {c.url}' for c in related_changes if c is not item.change))
 
     def _getItemReportJobsFields(self, item):
         # Extract the report elements from an item