From 4b10e95195e5552f5c0b5dc38bafcc402f60fd92 Mon Sep 17 00:00:00 2001
From: Brad Rydzewski <brad.rydzewski@gmail.com>
Date: Wed, 19 Aug 2020 10:43:59 -0400
Subject: [PATCH] ensure invalid dependecy fails linter with helpful error

---
 engine/linter/linter.go                | 37 +++++++++++++++++++-------
 engine/linter/linter_test.go           |  6 +++++
 engine/linter/testdata/missing_dep.yml | 33 ++++++-----------------
 3 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/engine/linter/linter.go b/engine/linter/linter.go
index 58921a7..a1175ac 100644
--- a/engine/linter/linter.go
+++ b/engine/linter/linter.go
@@ -19,15 +19,6 @@ import (
 // have the same name.
 var ErrDuplicateStepName = errors.New("linter: duplicate step names")
 
-// ErrMissingDependency is returned when a Pipeline step
-// defines dependencies that are invlid or unknown.
-var ErrMissingDependency = errors.New("linter: invalid or unknown step dependency")
-
-// ErrCyclicalDependency is returned when a Pipeline step
-// defines a cyclical dependency, which would result in an
-// infinite execution loop.
-var ErrCyclicalDependency = errors.New("linter: cyclical step dependency detected")
-
 // Opts provides linting options.
 type Opts struct {
 	Trusted bool
@@ -80,13 +71,28 @@ func checkPipeline(pipeline *resource.Pipeline, trusted bool) error {
 
 func checkSteps(pipeline *resource.Pipeline, trusted bool) error {
 	steps := append(pipeline.Services, pipeline.Steps...)
+	names := map[string]struct{}{}
+	if !pipeline.Clone.Disable {
+		names["clone"] = struct{}{}
+	}
 	for _, step := range steps {
 		if step == nil {
 			return errors.New("linter: nil step")
 		}
+
+		// unique list of names
+		_, ok := names[step.Name]
+		if ok {
+			return ErrDuplicateStepName
+		}
+		names[step.Name] = struct{}{}
+
 		if err := checkStep(step, trusted); err != nil {
 			return err
 		}
+		if err := checkDeps(step, names); err != nil {
+			return err
+		}
 	}
 	return nil
 }
@@ -171,3 +177,16 @@ func checkEmptyDirVolume(volume *resource.VolumeEmptyDir, trusted bool) error {
 	}
 	return nil
 }
+
+func checkDeps(step *resource.Step, deps map[string]struct{}) error {
+	for _, dep := range step.DependsOn {
+		_, ok := deps[dep]
+		if !ok {
+			return fmt.Errorf("linter: unknown step dependency detected: %s references %s", step.Name, dep)
+		}
+		if step.Name == dep {
+			return fmt.Errorf("linter: cyclical step dependency detected: %s", dep)
+		}
+	}
+	return nil
+}
diff --git a/engine/linter/linter_test.go b/engine/linter/linter_test.go
index ac58537..09d9c71 100644
--- a/engine/linter/linter_test.go
+++ b/engine/linter/linter_test.go
@@ -197,6 +197,12 @@ func TestLint(t *testing.T) {
 		// 	invalid: true,
 		// 	message: "linter: invalid or missing name",
 		// },
+
+		{
+			path:    "testdata/missing_dep.yml",
+			invalid: true,
+			message: "linter: unknown step dependency detected: test references foo",
+		},
 	}
 	for _, test := range tests {
 		name := path.Base(test.path)
diff --git a/engine/linter/testdata/missing_dep.yml b/engine/linter/testdata/missing_dep.yml
index 9088b62..3bd99cd 100644
--- a/engine/linter/testdata/missing_dep.yml
+++ b/engine/linter/testdata/missing_dep.yml
@@ -1,35 +1,18 @@
 ---
 kind: pipeline
 type: docker
-name: amd64
+name: default
 
 steps:
+- name: build
+  image: golang
+  commands:
+  - go build
+
 - name: test
   image: golang
   commands:
   - go build
   - go test
-
-services:
-- name: database
-  image: redis
-  ports:
-  - 6379
-
----
-kind: pipeline
-name: arm
-
-platform:
-  arch: arm
-
-steps:
-- name: test
-  image: golang
-  commands:
-  - go build
-  - go test
-
-depends_on:
-- foo
-...
\ No newline at end of file
+  depends_on:
+  - foo
\ No newline at end of file