Repository: jwplayer/buildpipe-buildkite-plugin (language: python)

Issue:
depends_on and key fail to be unique
[Dependencies](https://buildkite.com/docs/pipelines/dependencies) currently do no work when multiple projects are affected. This is due to Buildpipe no making the `key` unique causing a collision.


Discussion / hints:
Small example:
```
projects:
- label: ProjectA
  path: a
- label: ProjectB
  path: b

steps:
- label: StepA
  key: StepAKey
  command:
  - echo A
- label: StepB
  command:
  - echo B
  depends_on:
  - StepA
```
Currently, if we were to instantiate a pipeline including `ProjectA` and `ProjectB`, we'd have two steps with the `StepA` key. This is not accepted by Buildkite.

I believe the easy solution is to append the project labels to the step keys (if the key is there), similarly to what we do with the step labels.
Similarly, we should then add the project label to all the items in the `depends_on` list in the steps.

This would end up generating something like this:
```
steps:
- label: StepA ProjectA
  key: StepAKey ProjectA
  command:
  - echo A
- label: StepB ProjectA
  command:
  - echo B
  depends_on:
  - StepA ProjectA
- label: StepA ProjectB
  key: StepAKey ProjectB
  command:
  - echo A
- label: StepB ProjectB
  command:
  - echo B
  depends_on:
  - StepA ProjectB
```

A potential issue is the `key` syntax. I'm not sure if there are any limitations there.
[The docs](https://buildkite.com/docs/pipelines/command-step) only mention that it's not allowed to look like a UUID for some reason. Using a space as a separator, similar to what we do for labels, should take care of that.
Worst case, we'll have to add keys to projects as well.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9b669c0..1eb4722 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+0.9.3 (boyntoni)
+----------------
+
+1. Generate unique keys for project steps, and add support for depends_on.
+
+
 0.9.2 (mowies)
 --------------
 1. Add option to set pipeline and project scoped environment variables.
diff --git a/README.md b/README.md
index a7e3b29..514e3c2 100644
--- a/README.md
+++ b/README.md
@@ -15,7 +15,7 @@ Example
 steps:
   - label: ":buildkite:"
     plugins:
-      - jwplayer/buildpipe#v0.9.2:
+      - jwplayer/buildpipe#v0.9.3:
           dynamic_pipeline: dynamic_pipeline.yml
 ```
 
diff --git a/hooks/command b/hooks/command
index 6458408..3e928bc 100755
--- a/hooks/command
+++ b/hooks/command
@@ -1,7 +1,7 @@
 #!/bin/bash
 set -euo pipefail
 
-buildpipe_version="${BUILDKITE_PLUGIN_BUILDPIPE_VERSION:-0.9.2}"
+buildpipe_version="${BUILDKITE_PLUGIN_BUILDPIPE_VERSION:-0.9.3}"
 is_test="${BUILDKITE_PLUGIN_BUILDPIPE_TEST_MODE:-false}"
 
 if [[ "$is_test" == "false" ]]; then
diff --git a/pipeline.go b/pipeline.go
index ed8e1e6..4e65f6b 100644
--- a/pipeline.go
+++ b/pipeline.go
@@ -14,22 +14,47 @@ type Pipeline struct {
 	Steps []interface{} `yaml:"steps"`
 }
 
-func generateProjectSteps(step interface{}, projects []Project) []interface{} {
+func generateProjectSteps(steps []interface{}, step interface{}, projects []Project) []interface{} {
 	projectSteps := make([]interface{}, 0)
+
 	for _, project := range projects {
 		stepCopy := deepcopy.Copy(step)
 		stepCopyMap := stepCopy.(map[interface{}]interface{})
 
 		if project.checkProjectRules(stepCopyMap) {
+			// Unique project level label
 			stepCopyMap["label"] = fmt.Sprintf("%s %s", stepCopyMap["label"], project.Label)
+
+			// Set default buildpipe environment variables and
+			// set project env vars as step env vars
 			env := stepCopyMap["env"].(map[interface{}]interface{})
 			env["BUILDPIPE_PROJECT_LABEL"] = project.Label
 			env["BUILDPIPE_PROJECT_PATH"] = project.getMainPath()
-
 			for envVarName, envVarValue := range project.Env {
 				env[envVarName] = envVarValue
 			}
 
+			// Unique project level key, if present
+			if val, ok := stepCopyMap["key"]; ok {
+				stepCopyMap["key"] = fmt.Sprintf("%s %s", val, project.Label)
+			}
+
+			// If the step includes a depends_on clause, we need to validate whether each dependency
+			// is a project-scoped step. If so, the dependency has the current project name added
+			// to it to match the unique key given above.
+			if val, ok := stepCopyMap["depends_on"]; ok {
+				dependencyList := val.([]interface{})
+
+				for i, dependency := range dependencyList {
+					depStr := dependency.(string)
+
+					if step := findStepByKey(steps, depStr); step != nil {
+						if isProjectScopeStep(step) {
+							dependencyList[i] = fmt.Sprintf("%s %s", depStr, project.Label)
+						}
+					}
+				}
+			}
 			projectSteps = append(projectSteps, stepCopy)
 		}
 	}
@@ -37,6 +62,31 @@ func generateProjectSteps(step interface{}, projects []Project) []interface{} {
 	return projectSteps
 }
 
+func isProjectScopeStep(step map[interface{}]interface{}) bool {
+	if env, ok := step["env"].(map[interface{}]interface{}); ok {
+		if value, ok := env["BUILDPIPE_SCOPE"]; ok {
+			return value == "project"
+		}
+	}
+	return false
+}
+
+func findStepByKey(steps []interface{}, stepKey string) map[interface{}]interface{} {
+	for _, step := range steps {
+		// skip wait commands
+		stepMap, ok := step.(map[interface{}]interface{})
+		if !ok {
+			continue
+		}
+		// grab key if it has one and check whether it is project scoped
+		foundStepKey, ok := stepMap["key"]
+		if ok && stepKey == foundStepKey {
+			return stepMap
+		}
+	}
+	return nil
+}
+
 func generatePipeline(steps []interface{}, pipelineEnv map[string]string, projects []Project) *Pipeline {
 	generatedSteps := make([]interface{}, 0)
 
@@ -63,7 +113,7 @@ func generatePipeline(steps []interface{}, pipelineEnv map[string]string, projec
 
 		value, ok := env["BUILDPIPE_SCOPE"]
 		if ok && value == "project" {
-			projectSteps := generateProjectSteps(step, projects)
+			projectSteps := generateProjectSteps(steps, step, projects)
 			generatedSteps = append(generatedSteps, projectSteps...)
 		} else {
 			generatedSteps = append(generatedSteps, step)


Test patch:
diff --git a/tests/dynamic_pipeline.yml b/tests/dynamic_pipeline.yml
index dc07526..961c7b4 100644
--- a/tests/dynamic_pipeline.yml
+++ b/tests/dynamic_pipeline.yml
@@ -12,12 +12,17 @@ projects:
       - project2/
       - project1 # you can trigger a project using multiple paths
   - label: project3
-    skip: # you can skip a list of projects
+    skip: # you can skip a list of steps for a project
       - test
       - deploy-stg
     path: project3/somedir/ # subpaths can also be triggered
 steps: # the same schema as regular buildkite pipeline steps
+  - label: bootstrap # a non project scoped step (to test depends_on handling)
+    key: bootstrap
+    command:
+      - make bootstrap
   - label: test
+    key: test
     env:
       BUILDPIPE_SCOPE: project # this variable ensures a test step is generated for each project
     command:
@@ -35,6 +40,9 @@ steps: # the same schema as regular buildkite pipeline steps
       - make publish-image
     agents:
       - queue=build
+    depends_on:
+      - bootstrap # the rendered template should not include the project name for a non-project step
+      - test # the rendered template should include the project name for a project-scoped step
   - wait
   - label: tag
     branches: "master"
@@ -42,6 +50,7 @@ steps: # the same schema as regular buildkite pipeline steps
       - make tag-release
   - wait
   - label: deploy-stg
+    key: deploy-stg
     branches: "master"
     concurrency: 1
     concurrency_group: deploy-stg
@@ -58,6 +67,8 @@ steps: # the same schema as regular buildkite pipeline steps
     branches: "master"
     concurrency: 1
     concurrency_group: deploy-prd
+    depends_on:
+      - deploy-stg
     env:
       BUILDPIPE_SCOPE: project
     command:
diff --git a/tests/post-command.bats b/tests/post-command.bats
index 1cf8926..b4c97cd 100755
--- a/tests/post-command.bats
+++ b/tests/post-command.bats
@@ -34,6 +34,12 @@ teardown() {
 
   assert_output --partial << EOM
 steps:
+- command:
+  - make bootstrap
+  env:
+    TEST_ENV_PIPELINE: test-pipeline
+  key: bootstrap
+  label: bootstrap
 - command:
   - cd \$\$BUILDPIPE_PROJECT_PATH
   - make test
@@ -43,6 +49,7 @@ steps:
     BUILDPIPE_SCOPE: project
     TEST_ENV_PIPELINE: test-pipeline
     TEST_ENV_PROJECT: test-project
+  key: test project1
   label: test project1
 - wait
 - agents:
@@ -52,6 +59,9 @@ steps:
   - cd \$\$BUILDPIPE_PROJECT_PATH
   - make build
   - make publish-image
+  depends_on:
+  - bootstrap
+  - test project1
   env:
     BUILDPIPE_PROJECT_LABEL: project1
     BUILDPIPE_PROJECT_PATH: project1/
@@ -67,6 +77,9 @@ steps:
   - cd \$\$BUILDPIPE_PROJECT_PATH
   - make build
   - make publish-image
+  depends_on:
+  - bootstrap
+  - test project2
   env:
     BUILDPIPE_PROJECT_LABEL: project2
     BUILDPIPE_PROJECT_PATH: project2/
@@ -93,6 +106,7 @@ steps:
     BUILDPIPE_PROJECT_PATH: project2/
     BUILDPIPE_SCOPE: project
     TEST_ENV_PIPELINE: test-pipeline
+  key: deploy-stg project2
   label: deploy-stg project2
 - wait
 - block: ':rocket: Release!'
@@ -104,6 +118,8 @@ steps:
   - make deploy-prod
   concurrency: 1
   concurrency_group: deploy-prd
+  depends_on:
+  - deploy-stg project2
   env:
     BUILDPIPE_PROJECT_LABEL: project2
     BUILDPIPE_PROJECT_PATH: project2/