Merge "Fix weak dependencies to work with child_jobs"

10 files changed, 167 insertions, 22 deletions

diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-a.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-a.yaml
new file mode 100644
index 000000000..6e1b7d755
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-a.yaml
@@ -0,0 +1,10 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            child_jobs:
+              - data-return-d
+            log_url: http://example.com/test/log/url/
+          child:
+            valueA: 'yes'
diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-b.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-b.yaml
new file mode 100644
index 000000000..242a04616
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-b.yaml
@@ -0,0 +1,10 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            child_jobs:
+              - data-return-d
+            log_url: http://example.com/test/log/url/
+          child:
+            valueB: 'yes'
diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-c.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-c.yaml
new file mode 100644
index 000000000..0e934c7a8
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-c.yaml
@@ -0,0 +1,10 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            child_jobs:
+              - data-return-d
+            log_url: http://example.com/test/log/url/
+          child:
+            valueC: 'yes'
diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-cd.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-cd.yaml
new file mode 100644
index 000000000..727f46d0c
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-cd.yaml
@@ -0,0 +1,11 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            child_jobs:
+              - data-return-c
+              - data-return-d
+            log_url: http://example.com/test/log/url/
+          child:
+            value: CD
diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-d.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-d.yaml
new file mode 100644
index 000000000..905deb2f0
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/common-config/playbooks/data-return-d.yaml
@@ -0,0 +1,7 @@
+- hosts: localhost
+  tasks:
+    - name: Assert returned variables are valid
+      assert:
+        that:
+          - child.value == 'CD'
+          - child.valueC == 'yes'
diff --git a/tests/fixtures/config/data-return/git/common-config/zuul.yaml b/tests/fixtures/config/data-return/git/common-config/zuul.yaml
index 608cc835b..f9d21024a 100644
--- a/tests/fixtures/config/data-return/git/common-config/zuul.yaml
+++ b/tests/fixtures/config/data-return/git/common-config/zuul.yaml
@@ -37,6 +37,26 @@
     success-url: docs/index.html
     run: playbooks/data-return-relative.yaml
 
+- job:
+    name: data-return-a
+    run: playbooks/data-return-a.yaml
+
+- job:
+    name: data-return-b
+    run: playbooks/data-return-b.yaml
+
+- job:
+    name: data-return-c
+    run: playbooks/data-return-c.yaml
+
+- job:
+    name: data-return-d
+    run: playbooks/data-return-d.yaml
+
+- job:
+    name: data-return-cd
+    run: playbooks/data-return-cd.yaml
+
 # This child job will be skipped in the test case test_data_return_child_jobs.
 # In order to verify that this doesn't lead to node leaks attach a nodeset to
 # it. Each test case automatically verifies that there are no open node
@@ -110,3 +130,25 @@
         - data-return:
             dependencies:
               - several-zuul-return-child
+
+- project:
+    name: org/project-soft
+    check:
+      jobs:
+        - data-return-cd
+        - data-return-a:
+            dependencies:
+              - data-return-cd
+        - data-return-b:
+            dependencies:
+              - data-return-cd
+        - data-return-c:
+            dependencies:
+              - data-return-cd
+        - data-return-d:
+            dependencies:
+              - name: data-return-b
+                soft: true
+              - name: data-return-c
+                soft: true
+
diff --git a/tests/fixtures/config/data-return/git/org_project-soft/README b/tests/fixtures/config/data-return/git/org_project-soft/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/data-return/git/org_project-soft/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/data-return/main.yaml b/tests/fixtures/config/data-return/main.yaml
index 3d7a5c9c2..391517e5a 100644
--- a/tests/fixtures/config/data-return/main.yaml
+++ b/tests/fixtures/config/data-return/main.yaml
@@ -10,3 +10,4 @@
           - org/project2
           - org/project3
           - org/project4
+          - org/project-soft
diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index 5b3bc93e5..1cc29d60c 100644
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -3699,6 +3699,21 @@ class TestDataReturn(AnsibleZuulTestCase):
         self.assertIn('data-return : SKIPPED', A.messages[-1])
         self.assertIn('Build succeeded', A.messages[-1])
 
+    def test_data_return_skip_all_child_jobs_with_soft_dependencies(self):
+        A = self.fake_gerrit.addFakeChange('org/project-soft', 'master', 'A')
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='data-return-cd', result='SUCCESS', changes='1,1'),
+            dict(name='data-return-c', result='SUCCESS', changes='1,1'),
+            dict(name='data-return-d', result='SUCCESS', changes='1,1'),
+        ])
+        self.assertIn('- data-return-cd http://example.com/test/log/url/',
+                      A.messages[-1])
+        self.assertIn('data-return-a : SKIPPED', A.messages[-1])
+        self.assertIn('data-return-b : SKIPPED', A.messages[-1])
+        self.assertIn('Build succeeded', A.messages[-1])
+
     def test_several_zuul_return(self):
         A = self.fake_gerrit.addFakeChange('org/project4', 'master', 'A')
         self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
diff --git a/zuul/model.py b/zuul/model.py
index 76327d188..863806be0 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -1791,36 +1791,45 @@ class JobGraph(object):
     def getJobs(self):
         return list(self.jobs.values())  # Report in the order of layout cfg
 
-    def getDirectDependentJobs(self, parent_job):
+    def getDirectDependentJobs(self, parent_job, skip_soft=False):
         ret = set()
-        for dependent_name, parent_names in self._dependencies.items():
-            if parent_job in parent_names:
+        for dependent_name, parents in self._dependencies.items():
+            part = parent_job in parents \
+                and (not skip_soft or not parents[parent_job])
+            if part:
                 ret.add(dependent_name)
         return ret
 
-    def getDependentJobsRecursively(self, parent_job):
+    def getDependentJobsRecursively(self, parent_job, skip_soft=False):
         all_dependent_jobs = set()
         jobs_to_iterate = set([parent_job])
         while len(jobs_to_iterate) > 0:
             current_job = jobs_to_iterate.pop()
-            current_dependent_jobs = self.getDirectDependentJobs(current_job)
+            current_dependent_jobs = self.getDirectDependentJobs(current_job,
+                                                                 skip_soft)
             new_dependent_jobs = current_dependent_jobs - all_dependent_jobs
             jobs_to_iterate |= new_dependent_jobs
             all_dependent_jobs |= new_dependent_jobs
         return [self.jobs[name] for name in all_dependent_jobs]
 
-    def getParentJobsRecursively(self, dependent_job, layout=None):
+    def getParentJobsRecursively(self, dependent_job, layout=None,
+                                 skip_soft=False):
         return [self.jobs[name] for name in
                 self._getParentJobNamesRecursively(dependent_job,
-                                                   layout=layout)]
+                                                   layout=layout,
+                                                   skip_soft=skip_soft)]
 
     def _getParentJobNamesRecursively(self, dependent_job, soft=False,
-                                      layout=None):
+                                      layout=None, skip_soft=False):
         all_parent_jobs = set()
         jobs_to_iterate = set([(dependent_job, False)])
         while len(jobs_to_iterate) > 0:
             (current_job, current_soft) = jobs_to_iterate.pop()
             current_parent_jobs = self._dependencies.get(current_job)
+            if skip_soft:
+                hard_parent_jobs = \
+                    {d: s for d, s in current_parent_jobs.items() if not s}
+                current_parent_jobs = hard_parent_jobs
             if current_parent_jobs is None:
                 if soft or current_soft:
                     if layout:
@@ -2476,14 +2485,21 @@ class QueueItem(object):
             if self.item_ahead.isHoldingFollowingChanges():
                 return []
 
-        successful_job_names = set()
+        failed_job_names = set()  # Jobs that run and failed
+        ignored_job_names = set()  # Jobs that were skipped or canceled
+        unexecuted_job_names = set()  # Jobs that were not started yet
         jobs_not_started = set()
         for job in self.job_graph.getJobs():
             build = self.current_build_set.getBuild(job.name)
             if build:
                 if build.result == 'SUCCESS' or build.paused:
-                    successful_job_names.add(job.name)
+                    pass
+                elif build.result == 'SKIPPED':
+                    ignored_job_names.add(job.name)
+                else:  # elif build.result in ('FAILURE', 'CANCELED', ...):
+                    failed_job_names.add(job.name)
             else:
+                unexecuted_job_names.add(job.name)
                 jobs_not_started.add(job)
 
         # Attempt to run jobs in the order they appear in
@@ -2497,13 +2513,20 @@ class QueueItem(object):
             parent_builds_with_data = {}
             for parent_job in self.job_graph.getParentJobsRecursively(
                     job.name):
-                if parent_job.name not in successful_job_names:
+                if parent_job.name in unexecuted_job_names \
+                        or parent_job.name in failed_job_names:
                     all_parent_jobs_successful = False
                     break
                 parent_build = self.current_build_set.getBuild(parent_job.name)
                 if parent_build.result_data:
                     parent_builds_with_data[parent_job.name] = parent_build
 
+            for parent_job in self.job_graph.getParentJobsRecursively(
+                    job.name, skip_soft=True):
+                if parent_job.name in ignored_job_names:
+                    all_parent_jobs_successful = False
+                    break
+
             if all_parent_jobs_successful:
                 # Iterate in reverse order over all jobs of the graph (which is
                 # in sorted config order) and apply parent data of the jobs we
@@ -2537,15 +2560,20 @@ class QueueItem(object):
             if self.item_ahead.isHoldingFollowingChanges():
                 return []
 
-        successful_job_names = set()
+        failed_job_names = set()       # Jobs that run and failed
+        ignored_job_names = set()      # Jobs that were skipped or canceled
+        unexecuted_job_names = set()   # Jobs that were not started yet
         jobs_not_requested = set()
         for job in self.job_graph.getJobs():
             build = build_set.getBuild(job.name)
             if build and (build.result == 'SUCCESS' or build.paused):
-                successful_job_names.add(job.name)
-            elif build and build.result in ('SKIPPED', 'FAILURE', 'CANCELED'):
                 pass
+            elif build and build.result == 'SKIPPED':
+                ignored_job_names.add(job.name)
+            elif build and build.result in ('FAILURE', 'CANCELED'):
+                failed_job_names.add(job.name)
             else:
+                unexecuted_job_names.add(job.name)
                 nodeset = build_set.getJobNodeSet(job.name)
                 if nodeset is None:
                     req = build_set.getJobNodeRequest(job.name)
@@ -2562,7 +2590,13 @@ class QueueItem(object):
             all_parent_jobs_successful = True
             for parent_job in self.job_graph.getParentJobsRecursively(
                     job.name):
-                if parent_job.name not in successful_job_names:
+                if parent_job.name in unexecuted_job_names \
+                        or parent_job.name in failed_job_names:
+                    all_parent_jobs_successful = False
+                    break
+            for parent_job in self.job_graph.getParentJobsRecursively(
+                    job.name, skip_soft=True):
+                if parent_job.name in ignored_job_names:
                     all_parent_jobs_successful = False
                     break
             if all_parent_jobs_successful:
@@ -2590,20 +2624,24 @@ class QueueItem(object):
             if not zuul_return:
                 # If zuul.child_jobs exists and is empty, user want to skip all
                 # child jobs.
-                skipped += self.job_graph.getDependentJobsRecursively(
-                    build.job.name)
+                to_skip = self.job_graph.getDependentJobsRecursively(
+                    build.job.name, skip_soft=True)
+                skipped += to_skip
             else:
                 # We have list of jobs to run.
                 intersect_jobs = dependent_jobs.intersection(zuul_return)
 
                 for skip in (dependent_jobs - intersect_jobs):
-                    skipped.append(self.job_graph.jobs.get(skip))
-                    skipped += self.job_graph.getDependentJobsRecursively(
-                        skip)
+                    s = self.job_graph.jobs.get(skip)
+                    skipped.append(s)
+                    to_skip = self.job_graph.getDependentJobsRecursively(
+                        skip, skip_soft=True)
+                    skipped += to_skip
 
         elif build.result != 'SUCCESS' and not build.paused:
-            skipped += self.job_graph.getDependentJobsRecursively(
-                build.job.name)
+            to_skip = self.job_graph.getDependentJobsRecursively(
+                build.job.name, skip_soft=True)
+            skipped += to_skip
 
         for job in skipped:
             child_build = self.current_build_set.getBuild(job.name)