Skip to content

Commit

Permalink
Add ability to declare a env key in each pipeline task (#1970)
Browse files Browse the repository at this point in the history
The `env` key takes on some of the responsibilities of the dependsOn
config key, but notably does not accept strings that start with "$".

The goal of this change is to separate file dependencies from environmental
dependencies, so we can continue to improve cache partitioning.
  • Loading branch information
mehulkar committed Sep 15, 2022
1 parent d50ec2a commit b7abeb0
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 3 deletions.
3 changes: 3 additions & 0 deletions cli/internal/fs/testdata/invalid-env-1/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "test-repo"
}
8 changes: 8 additions & 0 deletions cli/internal/fs/testdata/invalid-env-1/turbo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"pipeline": {
"task1": {
// all invalid value
"env": ["$A", "$B"]
}
}
}
3 changes: 3 additions & 0 deletions cli/internal/fs/testdata/invalid-env-2/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "test-repo"
}
8 changes: 8 additions & 0 deletions cli/internal/fs/testdata/invalid-env-2/turbo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"pipeline": {
"task1": {
// Mixed values
"env": ["$A", "B"]
}
}
}
3 changes: 3 additions & 0 deletions cli/internal/fs/testdata/legacy-env/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "test-repo"
}
31 changes: 31 additions & 0 deletions cli/internal/fs/testdata/legacy-env/turbo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// mocked test comment
{
"pipeline": {
// Only legacy declaration
"task1": {
"dependsOn": ["$A"]
},
// Only new declaration
"task2": {
"env": ["A"]
},
// Same var declared in both
"task3": {
"dependsOn": ["$A"],
"env": ["A"]
},
// Different vars declared in both
"task4": {
"dependsOn": ["$A"],
"env": ["B"]
},

// some edge cases
"task6": { "env": ["A", "B", "C"], "dependsOn": ["$D", "$E", "$F"] },
"task7": { "env": ["A", "B", "C"], "dependsOn": ["$A", "$B", "$C"] },
"task8": { "env": ["A", "B", "C"], "dependsOn": ["A", "B", "C"] },
"task9": { "env": [], "dependsOn": ["$A"] },
"task10": { "env": ["A", "A"], "dependsOn": ["$A", "$A"] },
"task11": { "env": ["A", "A"], "dependsOn": ["$B", "$B"] }
}
}
21 changes: 18 additions & 3 deletions cli/internal/fs/turbo_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type pipelineJSON struct {
DependsOn []string `json:"dependsOn,omitempty"`
Inputs []string `json:"inputs,omitempty"`
OutputMode util.TaskOutputMode `json:"outputMode,omitempty"`
Env []string `json:"env,omitempty"`
}

// Pipeline is a struct for deserializing .pipeline in configFile
Expand Down Expand Up @@ -108,7 +109,6 @@ func readTurboJSON(path turbopath.AbsolutePath) (*TurboJSON, error) {
}
err = jsonc.Unmarshal(data, &turboJSON)
if err != nil {
println("error unmarshalling", err.Error())
return nil, err
}
return turboJSON, nil
Expand Down Expand Up @@ -162,18 +162,33 @@ func (c *TaskDefinition) UnmarshalJSON(data []byte) error {
} else {
c.ShouldCache = *rawPipeline.Cache
}
c.EnvVarDependencies = []string{}

envVarDependencies := make(util.Set)
c.TopologicalDependencies = []string{}
c.TaskDependencies = []string{}

for _, dependency := range rawPipeline.DependsOn {
if strings.HasPrefix(dependency, envPipelineDelimiter) {
c.EnvVarDependencies = append(c.EnvVarDependencies, strings.TrimPrefix(dependency, envPipelineDelimiter))
envVarDependencies.Add(strings.TrimPrefix(dependency, envPipelineDelimiter))
} else if strings.HasPrefix(dependency, topologicalPipelineDelimiter) {
c.TopologicalDependencies = append(c.TopologicalDependencies, strings.TrimPrefix(dependency, topologicalPipelineDelimiter))
} else {
c.TaskDependencies = append(c.TaskDependencies, dependency)
}
}

// Append env key into EnvVarDependencies
for _, value := range rawPipeline.Env {
if strings.HasPrefix(value, envPipelineDelimiter) {
// Hard error to help people specify this correctly during migration.
// TODO: Remove this error after we have run summary.
return fmt.Errorf("You specified \"%s\" in the \"env\" key. You should not prefix your environment variables with \"$\"", value)
}

envVarDependencies.Add(value)
}

c.EnvVarDependencies = envVarDependencies.UnsafeListOfStrings()
c.Inputs = rawPipeline.Inputs
c.OutputMode = rawPipeline.OutputMode
return nil
Expand Down
70 changes: 70 additions & 0 deletions cli/internal/fs/turbo_json_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package fs

import (
"os"
"sort"
"strings"
"testing"

Expand Down Expand Up @@ -134,6 +135,70 @@ func Test_ReadTurboConfig_BothCorrectAndLegacy(t *testing.T) {
assert.Equal(t, rootPackageJSON.LegacyTurboConfig == nil, true)
}

func Test_ReadTurboConfig_InvalidEnvDeclarations1(t *testing.T) {
testDir := getTestDir(t, "invalid-env-1")

packageJSONPath := testDir.Join("package.json")
rootPackageJSON, pkgJSONReadErr := ReadPackageJSON(packageJSONPath)

if pkgJSONReadErr != nil {
t.Fatalf("invalid parse: %#v", pkgJSONReadErr)
}

_, turboJSONReadErr := ReadTurboConfig(testDir, rootPackageJSON)

expectedErrorMsg := "turbo.json: You specified \"$A\" in the \"env\" key. You should not prefix your environment variables with \"$\""

assert.EqualErrorf(t, turboJSONReadErr, expectedErrorMsg, "Error should be: %v, got: %v", expectedErrorMsg, turboJSONReadErr)
}

func Test_ReadTurboConfig_InvalidEnvDeclarations2(t *testing.T) {
testDir := getTestDir(t, "invalid-env-2")

packageJSONPath := testDir.Join("package.json")
rootPackageJSON, pkgJSONReadErr := ReadPackageJSON(packageJSONPath)

if pkgJSONReadErr != nil {
t.Fatalf("invalid parse: %#v", pkgJSONReadErr)
}

_, turboJSONReadErr := ReadTurboConfig(testDir, rootPackageJSON)

expectedErrorMsg := "turbo.json: You specified \"$A\" in the \"env\" key. You should not prefix your environment variables with \"$\""

assert.EqualErrorf(t, turboJSONReadErr, expectedErrorMsg, "Error should be: %v, got: %v", expectedErrorMsg, turboJSONReadErr)
}

func Test_ReadTurboConfig_EnvDeclarations(t *testing.T) {
testDir := getTestDir(t, "legacy-env")

packageJSONPath := testDir.Join("package.json")
rootPackageJSON, pkgJSONReadErr := ReadPackageJSON(packageJSONPath)

if pkgJSONReadErr != nil {
t.Fatalf("invalid parse: %#v", pkgJSONReadErr)
}

turboJSON, turboJSONReadErr := ReadTurboConfig(testDir, rootPackageJSON)

if turboJSONReadErr != nil {
t.Fatalf("invalid parse: %#v", turboJSONReadErr)
}

pipeline := turboJSON.Pipeline

assert.EqualValues(t, sortedArray(pipeline["task1"].EnvVarDependencies), sortedArray([]string{"A"}))
assert.EqualValues(t, sortedArray(pipeline["task2"].EnvVarDependencies), sortedArray([]string{"A"}))
assert.EqualValues(t, sortedArray(pipeline["task3"].EnvVarDependencies), sortedArray([]string{"A"}))
assert.EqualValues(t, sortedArray(pipeline["task4"].EnvVarDependencies), sortedArray([]string{"A", "B"}))
assert.EqualValues(t, sortedArray(pipeline["task6"].EnvVarDependencies), sortedArray([]string{"A", "B", "C", "D", "E", "F"}))
assert.EqualValues(t, sortedArray(pipeline["task7"].EnvVarDependencies), sortedArray([]string{"A", "B", "C"}))
assert.EqualValues(t, sortedArray(pipeline["task8"].EnvVarDependencies), sortedArray([]string{"A", "B", "C"}))
assert.EqualValues(t, sortedArray(pipeline["task9"].EnvVarDependencies), sortedArray([]string{"A"}))
assert.EqualValues(t, sortedArray(pipeline["task10"].EnvVarDependencies), sortedArray([]string{"A"}))
assert.EqualValues(t, sortedArray(pipeline["task11"].EnvVarDependencies), sortedArray([]string{"A", "B"}))
}

// Helpers
func validateOutput(t *testing.T, actual Pipeline, expected map[string]TaskDefinition) {
// check top level keys
Expand Down Expand Up @@ -172,3 +237,8 @@ func getTestDir(t *testing.T, testName string) turbopath.AbsolutePath {

return cwd.Join("testdata", testName)
}

func sortedArray(arr []string) []string {
sort.Strings(arr)
return arr
}

0 comments on commit b7abeb0

Please sign in to comment.