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

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