Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn on absolute paths used in turbo.json #3658

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Warn on absolute paths used in turbo.json #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f29e34a
Warn on absolute paths used in turbo.json
Feb 6, 2023
f0c81d5
Add an integation test for the warning
Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cli/integration_tests/invalid_turbo_json/absolute_path_warning.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Setup
$ . ${TESTDIR}/../setup.sh
$ . ${TESTDIR}/setup.sh $(pwd)

Expect warnings
$ ${TURBO} build -v --dry > /dev/null
[-0-9:.TWZ+]+ \[INFO] turbo: skipping turbod since we appear to be in a non-interactive context (re)
[0-9]{4}/[0-9]{2}/[0-9]{2} [-0-9:.TWZ+]+ \[WARNING] Using an absolute path in "outputs" \(/another/absolute/path\) will not work and will be an error in a future version (re)
[0-9]{4}/[0-9]{2}/[0-9]{2} [-0-9:.TWZ+]+ \[WARNING] Using an absolute path in "inputs" \(/some/absolute/path\) will not work and will be an error in a future version (re)
[0-9]{4}/[0-9]{2}/[0-9]{2} [-0-9:.TWZ+]+ \[WARNING] Using an absolute path in "globalDependencies" \(/an/absolute/path\) will not work and will be an error in a future version (re)
3 changes: 3 additions & 0 deletions cli/integration_tests/invalid_turbo_json/monorepo/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
node_modules/
.turbo
.npmrc
9 changes: 9 additions & 0 deletions cli/integration_tests/invalid_turbo_json/monorepo/apps/my-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "my-app",
"scripts": {
"build": "echo 'building'"
},
"dependencies": {
"util": "*"
}
}
7 changes: 7 additions & 0 deletions cli/integration_tests/invalid_turbo_json/monorepo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "monorepo",
"workspaces": [
"apps/**",
"packages/**"
]
}
6 changes: 6 additions & 0 deletions cli/integration_tests/invalid_turbo_json/monorepo/packages/util/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "util",
"scripts": {
"build": "echo 'building'"
}
}
10 changes: 10 additions & 0 deletions cli/integration_tests/invalid_turbo_json/monorepo/turbo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"$schema": "https://turbo.build/schema.json",
"globalDependencies": ["/an/absolute/path", "some/file"],
"pipeline": {
"build": {
"inputs": ["another/file", "/some/absolute/path"],
"outputs": ["/another/absolute/path", "a/relative/path"]
}
}
}
6 changes: 6 additions & 0 deletions cli/integration_tests/invalid_turbo_json/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/bash

SCRIPT_DIR=$(dirname ${BASH_SOURCE[0]})
TARGET_DIR=$1
cp -a ${SCRIPT_DIR}/monorepo/. ${TARGET_DIR}/
${SCRIPT_DIR}/../setup_git.sh ${TARGET_DIR}
23 changes: 20 additions & 3 deletions cli/internal/fs/turbo_json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"io/ioutil"
"log"
"os"
"path/filepath"
"sort"
"strings"

Expand Down Expand Up @@ -163,7 +164,7 @@ func LoadTurboConfig(dir turbopath.AbsoluteSystemPath, rootPackageJSON *PackageJ
}

var turboJSON *TurboJSON
turboFromFiles, err := ReadTurboConfig(dir.UntypedJoin(configFile))
turboFromFiles, err := readTurboConfig(dir.UntypedJoin(configFile))

if !includeSynthesizedFromRootPackageJSON && err != nil {
// If the file didn't exist, throw a custom error here instead of propagating
Expand Down Expand Up @@ -250,8 +251,8 @@ func (to TaskOutputs) Sort() TaskOutputs {
return TaskOutputs{Inclusions: inclusions, Exclusions: exclusions}
}

// ReadTurboConfig reads turbo.json from a provided path
func ReadTurboConfig(turboJSONPath turbopath.AbsoluteSystemPath) (*TurboJSON, error) {
// readTurboConfig reads turbo.json from a provided path
func readTurboConfig(turboJSONPath turbopath.AbsoluteSystemPath) (*TurboJSON, error) {
mehulkar marked this conversation as resolved.
Show resolved Hide resolved
// If the configFile exists, use that
if turboJSONPath.FileExists() {
turboJSON, err := readTurboJSON(turboJSONPath)
Expand Down Expand Up @@ -398,8 +399,14 @@ func (btd *BookkeepingTaskDefinition) UnmarshalJSON(data []byte) error {

for _, glob := range task.Outputs {
if strings.HasPrefix(glob, "!") {
if filepath.IsAbs(glob[1:]) {
log.Printf("[WARNING] Using an absolute path in \"outputs\" (%v) will not work and will be an error in a future version", glob)
}
exclusions = append(exclusions, glob[1:])
} else {
if filepath.IsAbs(glob) {
log.Printf("[WARNING] Using an absolute path in \"outputs\" (%v) will not work and will be an error in a future version", glob)
}
inclusions = append(inclusions, glob)
}
}
Expand Down Expand Up @@ -465,6 +472,12 @@ func (btd *BookkeepingTaskDefinition) UnmarshalJSON(data []byte) error {
// Note that we don't require Inputs to be sorted, we're going to
// hash the resulting files and sort that instead
btd.definedFields.Add("Inputs")
// TODO: during rust port, this should be moved to a post-parse validation step
for _, input := range task.Inputs {
if filepath.IsAbs(input) {
log.Printf("[WARNING] Using an absolute path in \"inputs\" (%v) will not work and will be an error in a future version", input)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish we had the infrastructure to make this an error now, and add a config to opt into the old (incorrect) behavior.

}
btd.TaskDefinition.Inputs = task.Inputs
}

Expand Down Expand Up @@ -551,11 +564,15 @@ func (c *TurboJSON) UnmarshalJSON(data []byte) error {
envVarDependencies.Add(value)
}

// TODO: In the rust port, warnings should be refactored to a post-parse validation step
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth we have a TurboJSON.Validate() method that can do this now. Since this isn't throwing an error though, it would have to be adapted slightly to emit warnings rather than errors.

for _, value := range raw.GlobalDependencies {
if strings.HasPrefix(value, envPipelineDelimiter) {
log.Printf("[DEPRECATED] Declaring an environment variable in \"globalDependencies\" is deprecated, found %s. Use the \"globalEnv\" key or use `npx @turbo/codemod migrate-env-var-dependencies`.\n", value)
envVarDependencies.Add(strings.TrimPrefix(value, envPipelineDelimiter))
} else {
if filepath.IsAbs(value) {
log.Printf("[WARNING] Using an absolute path in \"globalDependencies\" (%v) will not work and will be an error in a future version", value)
}
globalFileDependencies.Add(value)
}
}
Expand Down
10 changes: 5 additions & 5 deletions cli/internal/fs/turbo_json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func assertIsSorted(t *testing.T, arr []string, msg string) {

func Test_ReadTurboConfig(t *testing.T) {
testDir := getTestDir(t, "correct")
turboJSON, turboJSONReadErr := ReadTurboConfig(testDir.UntypedJoin("turbo.json"))
turboJSON, turboJSONReadErr := readTurboConfig(testDir.UntypedJoin("turbo.json"))

if turboJSONReadErr != nil {
t.Fatalf("invalid parse: %#v", turboJSONReadErr)
Expand Down Expand Up @@ -141,29 +141,29 @@ func Test_LoadTurboConfig_BothCorrectAndLegacy(t *testing.T) {

func Test_ReadTurboConfig_InvalidEnvDeclarations1(t *testing.T) {
testDir := getTestDir(t, "invalid-env-1")
_, turboJSONReadErr := ReadTurboConfig(testDir.UntypedJoin("turbo.json"))
_, turboJSONReadErr := readTurboConfig(testDir.UntypedJoin("turbo.json"))

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")
_, turboJSONReadErr := ReadTurboConfig(testDir.UntypedJoin("turbo.json"))
_, turboJSONReadErr := readTurboConfig(testDir.UntypedJoin("turbo.json"))
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_InvalidGlobalEnvDeclarations(t *testing.T) {
testDir := getTestDir(t, "invalid-global-env")
_, turboJSONReadErr := ReadTurboConfig(testDir.UntypedJoin("turbo.json"))
_, turboJSONReadErr := readTurboConfig(testDir.UntypedJoin("turbo.json"))
expectedErrorMsg := "turbo.json: You specified \"$QUX\" 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")
turboJSON, turboJSONReadErr := ReadTurboConfig(testDir.UntypedJoin("turbo.json"))
turboJSON, turboJSONReadErr := readTurboConfig(testDir.UntypedJoin("turbo.json"))

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