Merge "Fix dependency cycle false positive" into feature/zuulv3

4 files changed, 67 insertions, 4 deletions

diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py
index 732bc3d60..a8924b902 100644
--- a/tests/unit/test_gerrit_crd.py
+++ b/tests/unit/test_gerrit_crd.py
@@ -90,6 +90,40 @@ class TestGerritCRD(ZuulTestCase):
             'project-merge', 'org/project1').changes
         self.assertEqual(changes, '2,1 1,1')
 
+    def test_crd_gate_triangle(self):
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+        C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+        A.addApproval('Code-Review', 2)
+        B.addApproval('Code-Review', 2)
+        C.addApproval('Code-Review', 2)
+        A.addApproval('Approved', 1)
+        B.addApproval('Approved', 1)
+
+        # C-->B
+        #  \ /
+        #   v
+        #   A
+
+        # C Depends-On: A
+        C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            C.subject, A.data['url'])
+        # B Depends-On: A
+        B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            B.subject, A.data['url'])
+        # C git-depends on B
+        C.setDependsOn(B, 1)
+        self.fake_gerrit.addEvent(C.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        self.assertEqual(A.reported, 2)
+        self.assertEqual(B.reported, 2)
+        self.assertEqual(C.reported, 2)
+        self.assertEqual(A.data['status'], 'MERGED')
+        self.assertEqual(B.data['status'], 'MERGED')
+        self.assertEqual(C.data['status'], 'MERGED')
+        self.assertEqual(self.history[-1].changes, '1,1 2,1 3,1')
+
     def test_crd_branch(self):
         "Test cross-repo dependencies in multiple branches"
 
@@ -257,6 +291,7 @@ class TestGerritCRD(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
         A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
+        B.addApproval('Approved', 1)
 
         # A -> B -> A (via commit-depends)
 
@@ -522,6 +557,30 @@ class TestGerritCRD(ZuulTestCase):
         for job in self.history:
             self.assertEqual(len(job.changes.split()), 1)
 
+    def test_crd_check_triangle(self):
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+        C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+
+        # C-->B
+        #  \ /
+        #   v
+        #   A
+
+        # C Depends-On: A
+        C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            C.subject, A.data['url'])
+        # B Depends-On: A
+        B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            B.subject, A.data['url'])
+        # C git-depends on B
+        C.setDependsOn(B, 1)
+        self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertEqual(C.reported, 1)
+        self.assertEqual(self.history[0].changes, '1,1 2,1 3,1')
+
     @simple_layout('layouts/three-projects.yaml')
     def test_crd_check_transitive(self):
         "Test transitive cross-repo dependencies"
diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py
index c711e4d95..90c93ecae 100644
--- a/tests/unit/test_gerrit_legacy_crd.py
+++ b/tests/unit/test_gerrit_legacy_crd.py
@@ -260,6 +260,7 @@ class TestGerritLegacyCRD(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
         A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
+        B.addApproval('Approved', 1)
 
         # A -> B -> A (via commit-depends)
 
diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py
index 20b376d6a..5ad761196 100644
--- a/zuul/manager/dependent.py
+++ b/zuul/manager/dependent.py
@@ -141,13 +141,16 @@ class DependentPipelineManager(PipelineManager):
 
     def enqueueChangesAhead(self, change, quiet, ignore_requirements,
                             change_queue, history=None):
-        if history and change.number in history:
+        if history and change in history:
             # detected dependency cycle
             self.log.warn("Dependency cycle detected")
             return False
         if hasattr(change, 'number'):
             history = history or []
-            history.append(change.number)
+            history = history + [change]
+        else:
+            # Don't enqueue dependencies ahead of a non-change ref.
+            return True
 
         ret = self.checkForChangesNeededBy(change, change_queue)
         if ret in [True, False]:
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index 0c2baf010..9da40d582 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -34,13 +34,13 @@ class IndependentPipelineManager(PipelineManager):
 
     def enqueueChangesAhead(self, change, quiet, ignore_requirements,
                             change_queue, history=None):
-        if history and change.number in history:
+        if history and change in history:
             # detected dependency cycle
             self.log.warn("Dependency cycle detected")
             return False
         if hasattr(change, 'number'):
             history = history or []
-            history.append(change.number)
+            history = history + [change]
         else:
             # Don't enqueue dependencies ahead of a non-change ref.
             return True