Merge "Consider queue settings for topic dependencies"

13 files changed, 218 insertions, 27 deletions

diff --git a/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml
index 44f26411c..b36f71cc7 100644
--- a/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml
+++ b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml
@@ -89,6 +89,12 @@
 - job:
     name: project3-job
 
+- job:
+    name: project5-job-t1
+
+- job:
+    name: project6-job-t1
+
 - project:
     name: common-config
     queue: integrated
@@ -144,3 +150,16 @@
     gate:
       jobs:
         - project3-job
+
+- project:
+    name: org/project5
+    queue: integrated
+    check:
+      jobs:
+        - project5-job-t1
+
+- project:
+    name: org/project6
+    check:
+      jobs:
+        - project6-job-t1
diff --git a/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml
new file mode 100644
index 000000000..ed97d539c
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml
@@ -0,0 +1 @@
+---
diff --git a/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml
new file mode 100644
index 000000000..be30f91d5
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml
@@ -0,0 +1,96 @@
+- queue:
+    name: integrated
+    allow-circular-dependencies: true
+    dependencies-by-topic: true
+
+- pipeline:
+    name: check
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+      github:
+        - event: pull_request
+          action:
+            - opened
+            - changed
+            - reopened
+            - edited
+    success:
+      gerrit:
+        Verified: 1
+      github:
+        status: success
+    failure:
+      gerrit:
+        Verified: -1
+      github:
+        status: failure
+
+- pipeline:
+    name: gate
+    manager: dependent
+    success-message: Build succeeded (gate).
+    require:
+      gerrit:
+        approval:
+          - Approved: 1
+      github:
+        label: approved
+    trigger:
+      gerrit:
+        - event: comment-added
+          approval:
+            - Approved: 1
+      github:
+        - event: pull_request
+          action: edited
+        - event: pull_request
+          action: labeled
+          label: approved
+    success:
+      gerrit:
+        Verified: 2
+        submit: true
+      github:
+        merge: true
+    failure:
+      gerrit:
+        Verified: -2
+      github: {}
+    start:
+      gerrit:
+        Verified: 0
+      github: {}
+    precedence: high
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/run.yaml
+
+- job:
+    name: project5-job-t2
+
+- job:
+    name: project6-job-t2
+
+- project:
+    name: org/project5
+    queue: integrated
+    check:
+      jobs:
+        - project5-job-t2
+    gate:
+      jobs:
+        - project5-job-t2
+
+- project:
+    name: org/project6
+    queue: integrated
+    check:
+      jobs:
+        - project6-job-t2
+    gate:
+      jobs:
+        - project6-job-t2
diff --git a/tests/fixtures/config/circular-dependencies/git/org_project5/README b/tests/fixtures/config/circular-dependencies/git/org_project5/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/org_project5/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/circular-dependencies/git/org_project6/README b/tests/fixtures/config/circular-dependencies/git/org_project6/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/org_project6/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/circular-dependencies/main.yaml b/tests/fixtures/config/circular-dependencies/main.yaml
index 8083a582b..d03b087f9 100644
--- a/tests/fixtures/config/circular-dependencies/main.yaml
+++ b/tests/fixtures/config/circular-dependencies/main.yaml
@@ -10,6 +10,8 @@
           - org/project1
           - org/project2
           - org/project3
+          - org/project5
+          - org/project6
       github:
         untrusted-projects:
           - gh/project
@@ -21,5 +23,9 @@
     name: tenant-two
     source:
       gerrit:
+        config-projects:
+          - common-tenant-two
         untrusted-projects:
           - org/project4
+          - org/project5
+          - org/project6
diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py
index 31fb4fd28..74d2cedf0 100644
--- a/tests/unit/test_circular_dependencies.py
+++ b/tests/unit/test_circular_dependencies.py
@@ -2283,6 +2283,55 @@ class TestGerritCircularDependencies(ZuulTestCase):
             dict(name="test-job", result="SUCCESS", changes="2,1 1,2"),
         ], ordered=False)
 
+    def test_deps_by_topic_multi_tenant(self):
+        A = self.fake_gerrit.addFakeChange('org/project5', "master", "A",
+                                           topic='test-topic')
+        B = self.fake_gerrit.addFakeChange('org/project6', "master", "B",
+                                           topic='test-topic')
+
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        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")
+
+        # We're about to add approvals to changes without adding the
+        # triggering events to Zuul, so that we can be sure that it is
+        # enqueuing the changes based on dependencies, not because of
+        # triggering events.  Since it will have the changes cached
+        # already (without approvals), we need to clear the cache
+        # first.
+        for connection in self.scheds.first.connections.connections.values():
+            connection.maintainCache([], max_age=0)
+
+        A.addApproval("Code-Review", 2)
+        B.addApproval("Code-Review", 2)
+        A.addApproval("Approved", 1)
+        self.fake_gerrit.addEvent(B.addApproval("Approved", 1))
+        self.waitUntilSettled()
+
+        self.assertEqual(A.reported, 4)
+        self.assertEqual(B.reported, 4)
+        self.assertEqual(A.data["status"], "MERGED")
+        self.assertEqual(B.data["status"], "MERGED")
+
+        self.assertHistory([
+            # Check
+            dict(name="project5-job-t1", result="SUCCESS", changes="1,1"),
+            dict(name="project6-job-t1", result="SUCCESS", changes="2,1"),
+            dict(name="project5-job-t2", result="SUCCESS", changes="2,1 1,1"),
+            dict(name="project6-job-t2", result="SUCCESS", changes="1,1 2,1"),
+            # Gate
+            dict(name="project5-job-t2", result="SUCCESS", changes="1,1 2,1"),
+            dict(name="project6-job-t2", result="SUCCESS", changes="1,1 2,1"),
+        ], ordered=False)
+
 
 class TestGithubCircularDependencies(ZuulTestCase):
     config_file = "zuul-gerrit-github.conf"
diff --git a/zuul/driver/zuul/__init__.py b/zuul/driver/zuul/__init__.py
index 6dd6ff1b9..5051f99e5 100644
--- a/zuul/driver/zuul/__init__.py
+++ b/zuul/driver/zuul/__init__.py
@@ -23,6 +23,7 @@ from zuul.driver.zuul.zuulmodel import ZuulTriggerEvent
 from zuul.driver.zuul import zuulmodel
 from zuul.driver.zuul import zuultrigger
 from zuul.lib.logutil import get_annotated_logger
+from zuul.model import Change
 
 PARENT_CHANGE_ENQUEUED = 'parent-change-enqueued'
 PROJECT_CHANGE_MERGED = 'project-change-merged'
@@ -121,7 +122,7 @@ class ZuulDriver(Driver, TriggerInterface):
         log = get_annotated_logger(self.log, event)
 
         log.debug("Checking for changes needing %s:" % change)
-        if not hasattr(change, 'needed_by_changes'):
+        if not isinstance(change, Change):
             log.debug("  %s does not support dependencies" % type(change))
             return
 
@@ -129,7 +130,8 @@ class ZuulDriver(Driver, TriggerInterface):
         # numbers of github installations.  This can be improved later
         # with persistent storage of dependency information.
         needed_by_changes = set(
-            pipeline.manager.resolveChangeReferences(change.needed_by_changes))
+            pipeline.manager.resolveChangeReferences(
+                change.getNeededByChanges()))
         for source in self.sched.connections.getSources():
             log.debug("  Checking source: %s",
                       source.connection.connection_name)
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index 538da2c10..c94899782 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -228,9 +228,16 @@ class PipelineManager(metaclass=ABCMeta):
                 change = source.getChange(key)
                 if change is None:
                     self.log.error("Unable to resolve change from key %s", key)
-                if (isinstance(change, model.Change)
-                        and change.commit_needs_changes is None):
-                    self.updateCommitDependencies(change, None, event=None)
+
+                if isinstance(change, model.Change):
+                    update_commit_dependencies = (
+                        change.commit_needs_changes is None)
+                    update_topic_dependencies = (
+                        change.topic_needs_changes is None
+                        and self.useDependenciesByTopic(change.project))
+                    if (update_commit_dependencies
+                            or update_topic_dependencies):
+                        self.updateCommitDependencies(change, None, event=None)
                 self._change_cache[change.cache_key] = change
             resolved_changes.append(change)
         return resolved_changes
@@ -246,8 +253,9 @@ class PipelineManager(metaclass=ABCMeta):
                 active_layout_uuids.add(item.layout_uuid)
 
             if isinstance(item.change, model.Change):
-                referenced_change_keys.update(item.change.needs_changes)
-                referenced_change_keys.update(item.change.needed_by_changes)
+                referenced_change_keys.update(item.change.getNeedsChanges(
+                    self.useDependenciesByTopic(item.change.project)))
+                referenced_change_keys.update(item.change.getNeededByChanges())
 
         # Clean up unused layouts in the cache
         unused_layouts = set(self._layout_cache.keys()) - active_layout_uuids
@@ -574,7 +582,7 @@ class PipelineManager(metaclass=ABCMeta):
                 return True
 
             cycle = []
-            if hasattr(change, "needs_changes"):
+            if isinstance(change, model.Change):
                 cycle = self.cycleForChange(change, dependency_graph, event)
                 if cycle and not self.canProcessCycle(change.project):
                     log.info("Dequeing change %s since at least one project "
diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py
index f84f52dad..4af541dfd 100644
--- a/zuul/manager/dependent.py
+++ b/zuul/manager/dependent.py
@@ -80,7 +80,7 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         history = history if history is not None else []
 
         log.debug("Checking for changes needing %s:" % change)
-        if not hasattr(change, 'needed_by_changes'):
+        if not isinstance(change, model.Change):
             log.debug("  %s does not support dependencies" % type(change))
             return
 
@@ -90,7 +90,7 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         sources = {p.source for p in projects}
 
         needed_by_changes = self.resolveChangeReferences(
-            change.needed_by_changes)
+            change.getNeededByChanges())
         log.debug("  Previously known following changes: %s",
                   needed_by_changes)
         seen = set(needed_by_changes)
@@ -184,10 +184,11 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         # Return true if okay to proceed enqueing this change,
         # false if the change should not be enqueued.
         log.debug("Checking for changes needed by %s:" % change)
-        if not hasattr(change, 'needs_changes'):
+        if not isinstance(change, model.Change):
             log.debug("  %s does not support dependencies", type(change))
             return False, []
-        if not change.needs_changes:
+        if not change.getNeedsChanges(
+                self.useDependenciesByTopic(change.project)):
             log.debug("  No changes needed")
             return False, []
         changes_needed = []
@@ -195,7 +196,8 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         # Ignore supplied change_queue
         with self.getChangeQueue(change, event) as change_queue:
             for needed_change in self.resolveChangeReferences(
-                    change.needs_changes):
+                    change.getNeedsChanges(
+                        self.useDependenciesByTopic(change.project))):
                 log.debug("  Change %s needs change %s:" % (
                     change, needed_change))
                 if needed_change.is_merged:
@@ -245,13 +247,15 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         return abort, changes_needed
 
     def getFailingDependentItems(self, item, nnfi):
-        if not hasattr(item.change, 'needs_changes'):
+        if not isinstance(item.change, model.Change):
             return None
-        if not item.change.needs_changes:
+        if not item.change.getNeedsChanges(
+                self.useDependenciesByTopic(item.change.project)):
             return None
         failing_items = set()
         for needed_change in self.resolveChangeReferences(
-                item.change.needs_changes):
+                item.change.getNeedsChanges(
+                    self.useDependenciesByTopic(item.change.project))):
             needed_item = self.getItemForChange(needed_change)
             if not needed_item:
                 continue
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index b70e9184b..aeeff5adc 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -82,16 +82,18 @@ class IndependentPipelineManager(PipelineManager):
         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'):
+        if not isinstance(change, model.Change):
             log.debug("  %s does not support dependencies" % type(change))
             return False, []
-        if not change.needs_changes:
+        if not change.getNeedsChanges(
+                self.useDependenciesByTopic(change.project)):
             log.debug("  No changes needed")
             return False, []
         changes_needed = []
         abort = False
         for needed_change in self.resolveChangeReferences(
-                change.needs_changes):
+                change.getNeedsChanges(
+                    self.useDependenciesByTopic(change.project))):
             log.debug("  Change %s needs change %s:" % (
                 change, needed_change))
             if needed_change.is_merged:
diff --git a/zuul/model.py b/zuul/model.py
index c2da4f65c..3658a0289 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -6162,18 +6162,17 @@ class Change(Branch):
             return True
         return False
 
-    @property
-    def needs_changes(self):
+    def getNeedsChanges(self, include_topic_needs=False):
         commit_needs_changes = self.commit_needs_changes or []
-        topic_needs_changes = self.topic_needs_changes or []
+        topic_needs_changes = (
+            include_topic_needs and self.topic_needs_changes) or []
         r = OrderedDict()
         for x in (self.git_needs_changes + self.compat_needs_changes +
                   commit_needs_changes + topic_needs_changes):
             r[x] = None
         return tuple(r.keys())
 
-    @property
-    def needed_by_changes(self):
+    def getNeededByChanges(self):
         r = OrderedDict()
         for x in (self.git_needed_by_changes + self.compat_needed_by_changes):
             r[x] = None
@@ -6197,8 +6196,9 @@ class Change(Branch):
            so far.  Will be updated with additional changes by this method.
         """
         related.add(self.cache_stat.key)
-        for reference in itertools.chain(self.needs_changes,
-                                         self.needed_by_changes):
+        for reference in itertools.chain(
+                self.getNeedsChanges(include_topic_needs=True),
+                self.getNeededByChanges()):
             key = ChangeKey.fromReference(reference)
             if key not in related:
                 source = sched.connections.getSource(key.connection_name)
diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py
index 9a868eac4..090bbfbb8 100644
--- a/zuul/reporter/__init__.py
+++ b/zuul/reporter/__init__.py
@@ -261,7 +261,9 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
 
     def _formatItemReportOtherBundleItems(self, item):
         related_changes = item.pipeline.manager.resolveChangeReferences(
-            item.change.needs_changes)
+            item.change.getNeedsChanges(
+                item.pipeline.manager.useDependenciesByTopic(
+                    item.change.project)))
         return "Related changes:\n{}\n".format("\n".join(
             f'  - {c.url}' for c in related_changes if c is not item.change))