From 79c74e94236ca31f0e9f0600981097c96a9cbb61 Mon Sep 17 00:00:00 2001
From: Simone Gotti <simone.gotti@gmail.com>
Date: Mon, 12 Aug 2019 23:11:19 +0200
Subject: [PATCH] config: fix check on task and parents with common deps

---
 internal/config/config.go            |  5 +--
 internal/config/config_test.go       | 65 ++++++++++++++++++++++++++++
 internal/runconfig/runconfig.go      |  5 +--
 internal/runconfig/runconfig_test.go | 57 ++++++++++++++++++++++++
 4 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/internal/config/config.go b/internal/config/config.go
index 2a594a1..3bb34b2 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -769,12 +769,11 @@ func checkConfig(config *Config) error {
 		for _, task := range run.Tasks {
 			parents := getTaskParents(run, task)
 			for _, parent := range parents {
-				allParents := getAllTaskParents(run, task)
 				allParentParents := getAllTaskParents(run, parent)
-				for _, p := range allParents {
+				for _, p := range parents {
 					for _, pp := range allParentParents {
 						if p.Name == pp.Name {
-							return errors.Errorf("task %s and its dependency %s have both a dependency on task %s", task.Name, parent.Name, p.Name)
+							return errors.Errorf("task %q and its dependency %q have both a dependency on task %q", task.Name, parent.Name, p.Name)
 						}
 					}
 				}
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index f15d3d8..a06f4e0 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -133,6 +133,71 @@ func TestParseConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "test task parent same dep a -> b -> c, a -> c",
+			in: `
+                runs:
+                  - name: run01
+                    tasks:
+                      - name: task01
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                      - name: task02
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                        depends:
+                          - task01
+                      - name: task03
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                        depends:
+                          - task02
+                          - task01
+                `,
+			err: errors.Errorf("task %q and its dependency %q have both a dependency on task %q", "task03", "task02", "task01"),
+		},
+		{
+			name: "test task parent same dep a -> b -> c -> d, a -> d",
+			in: `
+                runs:
+                  - name: run01
+                    tasks:
+                      - name: task01
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                      - name: task02
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                        depends:
+                          - task01
+                      - name: task03
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                        depends:
+                          - task02
+                      - name: task04
+                        runtime:
+                          type: pod
+                          containers:
+                            - image: busybox
+                        depends:
+                          - task03
+                          - task01
+                `,
+			err: errors.Errorf("task %q and its dependency %q have both a dependency on task %q", "task04", "task03", "task01"),
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/internal/runconfig/runconfig.go b/internal/runconfig/runconfig.go
index 31506e7..46fc709 100644
--- a/internal/runconfig/runconfig.go
+++ b/internal/runconfig/runconfig.go
@@ -316,12 +316,11 @@ func CheckRunConfigTasks(rcts map[string]*rstypes.RunConfigTask) error {
 	for _, t := range rcts {
 		parents := GetParents(rcts, t)
 		for _, parent := range parents {
-			allParents := GetAllParents(rcts, t)
 			allParentParents := GetAllParents(rcts, parent)
-			for _, p := range allParents {
+			for _, p := range parents {
 				for _, pp := range allParentParents {
 					if p.ID == pp.ID {
-						return errors.Errorf("task %s and its parent %s have both a dependency on task %s", t.Name, parent.Name, p.Name)
+						return errors.Errorf("task %q and its parent %q have both a dependency on task %q", t.Name, parent.Name, p.Name)
 					}
 				}
 			}
diff --git a/internal/runconfig/runconfig_test.go b/internal/runconfig/runconfig_test.go
index c2a6040..725be67 100644
--- a/internal/runconfig/runconfig_test.go
+++ b/internal/runconfig/runconfig_test.go
@@ -590,6 +590,63 @@ func TestCheckRunConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "test task parent same dep a -> b -> c, a -> c",
+			in: []task{
+				{
+					ID:    "1",
+					Level: -1,
+				},
+				{
+					ID:    "2",
+					Level: -1,
+					Depends: map[string]*rstypes.RunConfigTaskDepend{
+						"1": &rstypes.RunConfigTaskDepend{TaskID: "1"},
+					},
+				},
+				{
+					ID:    "3",
+					Level: -1,
+					Depends: map[string]*rstypes.RunConfigTaskDepend{
+						"2": &rstypes.RunConfigTaskDepend{TaskID: "2"},
+						"1": &rstypes.RunConfigTaskDepend{TaskID: "1"},
+					},
+				},
+			},
+			err: errors.Errorf("task %q and its parent %q have both a dependency on task %q", "task3", "task2", "task1"),
+		},
+		{
+			name: "test task parent same dep a -> b -> c -> d, a -> d",
+			in: []task{
+				{
+					ID:    "1",
+					Level: -1,
+				},
+				{
+					ID:    "2",
+					Level: -1,
+					Depends: map[string]*rstypes.RunConfigTaskDepend{
+						"1": &rstypes.RunConfigTaskDepend{TaskID: "1"},
+					},
+				},
+				{
+					ID:    "3",
+					Level: -1,
+					Depends: map[string]*rstypes.RunConfigTaskDepend{
+						"2": &rstypes.RunConfigTaskDepend{TaskID: "2"},
+					},
+				},
+				{
+					ID:    "4",
+					Level: -1,
+					Depends: map[string]*rstypes.RunConfigTaskDepend{
+						"3": &rstypes.RunConfigTaskDepend{TaskID: "3"},
+						"1": &rstypes.RunConfigTaskDepend{TaskID: "1"},
+					},
+				},
+			},
+			err: errors.Errorf("task %q and its parent %q have both a dependency on task %q", "task4", "task3", "task1"),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {