Fix ability to import the CLI repository as module (#1671)

## Changes While investigating #1629, I found that Go doesn't allow characters outside the set documented at https://pkg.go.dev/golang.org/x/mod/module#CheckFilePath. To fix this, I changed the relevant test case to create the fixtures it needs instead of loading it from the `testdata` directory (in `renderer_test.go`). Some test cases in `config_test.go` depended on templated paths without needing to do so. In the process of fixing this, I refactored these tests slightly to reduce dependencies between them. This change also adds a test case to ensure that all files in the repository are allowed to be part of a module (per the earlier `CheckFilePath` function). Fixes #1629. ## Tests I manually confirmed I could import the repository as a Go module.
databricks · Aug 12, 2024 · ad8e61c · ad8e61c
1 parent a38e16c
commit ad8e61c
Show file tree

Hide file tree

Showing 16 changed files with 296 additions and 75 deletions.
diff --git a/internal/testutil/copy.go b/internal/testutil/copy.go
@@ -0,0 +1,48 @@
+package testutil
+
+import (
+	"io"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// CopyDirectory copies the contents of a directory to another directory.
+// The destination directory is created if it does not exist.
+func CopyDirectory(t *testing.T, src, dst string) {
+	err := filepath.WalkDir(src, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+
+		rel, err := filepath.Rel(src, path)
+		require.NoError(t, err)
+
+		if d.IsDir() {
+			return os.MkdirAll(filepath.Join(dst, rel), 0755)
+		}
+
+		// Copy the file to the temporary directory
+		in, err := os.Open(path)
+		if err != nil {
+			return err
+		}
+
+		defer in.Close()
+
+		out, err := os.Create(filepath.Join(dst, rel))
+		if err != nil {
+			return err
+		}
+
+		defer out.Close()
+
+		_, err = io.Copy(out, in)
+		return err
+	})
+
+	require.NoError(t, err)
+}
diff --git a/libs/template/config_test.go b/libs/template/config_test.go
@@ -3,110 +3,153 @@ package template
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"testing"
 	"text/template"
 
-	"github.com/databricks/cli/cmd/root"
 	"github.com/databricks/cli/libs/jsonschema"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func testConfig(t *testing.T) *config {
-	c, err := newConfig(context.Background(), "./testdata/config-test-schema/test-schema.json")
-	require.NoError(t, err)
-	return c
-}
-
 func TestTemplateConfigAssignValuesFromFile(t *testing.T) {
-	c := testConfig(t)
+	testDir := "./testdata/config-assign-from-file"
 
-	err := c.assignValuesFromFile("./testdata/config-assign-from-file/config.json")
-	assert.NoError(t, err)
+	ctx := context.Background()
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
 
-	assert.Equal(t, int64(1), c.values["int_val"])
-	assert.Equal(t, float64(2), c.values["float_val"])
-	assert.Equal(t, true, c.values["bool_val"])
-	assert.Equal(t, "hello", c.values["string_val"])
+	err = c.assignValuesFromFile(filepath.Join(testDir, "config.json"))
+	if assert.NoError(t, err) {
+		assert.Equal(t, int64(1), c.values["int_val"])
+		assert.Equal(t, float64(2), c.values["float_val"])
+		assert.Equal(t, true, c.values["bool_val"])
+		assert.Equal(t, "hello", c.values["string_val"])
+	}
 }
 
-func TestTemplateConfigAssignValuesFromFileForInvalidIntegerValue(t *testing.T) {
-	c := testConfig(t)
+func TestTemplateConfigAssignValuesFromFileDoesNotOverwriteExistingConfigs(t *testing.T) {
+	testDir := "./testdata/config-assign-from-file"
 
-	err := c.assignValuesFromFile("./testdata/config-assign-from-file-invalid-int/config.json")
-	assert.EqualError(t, err, "failed to load config from file ./testdata/config-assign-from-file-invalid-int/config.json: failed to parse property int_val: cannot convert \"abc\" to an integer")
-}
+	ctx := context.Background()
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
 
-func TestTemplateConfigAssignValuesFromFileDoesNotOverwriteExistingConfigs(t *testing.T) {
-	c := testConfig(t)
 	c.values = map[string]any{
 		"string_val": "this-is-not-overwritten",
 	}
 
-	err := c.assignValuesFromFile("./testdata/config-assign-from-file/config.json")
-	assert.NoError(t, err)
+	err = c.assignValuesFromFile(filepath.Join(testDir, "config.json"))
+	if assert.NoError(t, err) {
+		assert.Equal(t, int64(1), c.values["int_val"])
+		assert.Equal(t, float64(2), c.values["float_val"])
+		assert.Equal(t, true, c.values["bool_val"])
+		assert.Equal(t, "this-is-not-overwritten", c.values["string_val"])
+	}
+}
+
+func TestTemplateConfigAssignValuesFromFileForInvalidIntegerValue(t *testing.T) {
+	testDir := "./testdata/config-assign-from-file-invalid-int"
+
+	ctx := context.Background()
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
 
-	assert.Equal(t, int64(1), c.values["int_val"])
-	assert.Equal(t, float64(2), c.values["float_val"])
-	assert.Equal(t, true, c.values["bool_val"])
-	assert.Equal(t, "this-is-not-overwritten", c.values["string_val"])
+	err = c.assignValuesFromFile(filepath.Join(testDir, "config.json"))
+	assert.EqualError(t, err, fmt.Sprintf("failed to load config from file %s: failed to parse property int_val: cannot convert \"abc\" to an integer", filepath.Join(testDir, "config.json")))
 }
 
 func TestTemplateConfigAssignValuesFromFileFiltersPropertiesNotInTheSchema(t *testing.T) {
-	c := testConfig(t)
+	testDir := "./testdata/config-assign-from-file-unknown-property"
 
-	err := c.assignValuesFromFile("./testdata/config-assign-from-file-unknown-property/config.json")
+	ctx := context.Background()
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
+
+	err = c.assignValuesFromFile(filepath.Join(testDir, "config.json"))
 	assert.NoError(t, err)
 
 	// assert only the known property is loaded
 	assert.Len(t, c.values, 1)
 	assert.Equal(t, "i am a known property", c.values["string_val"])
 }
 
-func TestTemplateConfigAssignDefaultValues(t *testing.T) {
-	c := testConfig(t)
+func TestTemplateConfigAssignValuesFromDefaultValues(t *testing.T) {
+	testDir := "./testdata/config-assign-from-default-value"
 
 	ctx := context.Background()
-	ctx = root.SetWorkspaceClient(ctx, nil)
-	helpers := loadHelpers(ctx)
-	r, err := newRenderer(ctx, nil, helpers, "./testdata/template-in-path/template", "./testdata/template-in-path/library", t.TempDir())
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
+
+	r, err := newRenderer(ctx, nil, nil, "./testdata/empty/template", "./testdata/empty/library", t.TempDir())
 	require.NoError(t, err)
 
 	err = c.assignDefaultValues(r)
-	assert.NoError(t, err)
+	if assert.NoError(t, err) {
+		assert.Equal(t, int64(123), c.values["int_val"])
+		assert.Equal(t, float64(123), c.values["float_val"])
+		assert.Equal(t, true, c.values["bool_val"])
+		assert.Equal(t, "hello", c.values["string_val"])
+	}
+}
+
+func TestTemplateConfigAssignValuesFromTemplatedDefaultValues(t *testing.T) {
+	testDir := "./testdata/config-assign-from-templated-default-value"
+
+	ctx := context.Background()
+	c, err := newConfig(ctx, filepath.Join(testDir, "schema.json"))
+	require.NoError(t, err)
+
+	r, err := newRenderer(ctx, nil, nil, filepath.Join(testDir, "template/template"), filepath.Join(testDir, "template/library"), t.TempDir())
+	require.NoError(t, err)
 
-	assert.Len(t, c.values, 2)
-	assert.Equal(t, "my_file", c.values["string_val"])
-	assert.Equal(t, int64(123), c.values["int_val"])
+	// Note: only the string value is templated.
+	// The JSON schema package doesn't allow using a string default for integer types.
+	err = c.assignDefaultValues(r)
+	if assert.NoError(t, err) {
+		assert.Equal(t, int64(123), c.values["int_val"])
+		assert.Equal(t, float64(123), c.values["float_val"])
+		assert.Equal(t, true, c.values["bool_val"])
+		assert.Equal(t, "world", c.values["string_val"])
+	}
 }
 
 func TestTemplateConfigValidateValuesDefined(t *testing.T) {
-	c := testConfig(t)
+	ctx := context.Background()
+	c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json")
+	require.NoError(t, err)
+
 	c.values = map[string]any{
 		"int_val":   1,
 		"float_val": 1.0,
 		"bool_val":  false,
 	}
 
-	err := c.validate()
+	err = c.validate()
 	assert.EqualError(t, err, "validation for template input parameters failed. no value provided for required property string_val")
 }
 
 func TestTemplateConfigValidateTypeForValidConfig(t *testing.T) {
-	c := testConfig(t)
+	ctx := context.Background()
+	c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json")
+	require.NoError(t, err)
+
 	c.values = map[string]any{
 		"int_val":    1,
 		"float_val":  1.1,
 		"bool_val":   true,
 		"string_val": "abcd",
 	}
 
-	err := c.validate()
+	err = c.validate()
 	assert.NoError(t, err)
 }
 
 func TestTemplateConfigValidateTypeForUnknownField(t *testing.T) {
-	c := testConfig(t)
+	ctx := context.Background()
+	c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json")
+	require.NoError(t, err)
+
 	c.values = map[string]any{
 		"unknown_prop": 1,
 		"int_val":      1,
@@ -115,20 +158,23 @@ func TestTemplateConfigValidateTypeForUnknownField(t *testing.T) {
 		"string_val":   "abcd",
 	}
 
-	err := c.validate()
+	err = c.validate()
 	assert.EqualError(t, err, "validation for template input parameters failed. property unknown_prop is not defined in the schema")
 }
 
 func TestTemplateConfigValidateTypeForInvalidType(t *testing.T) {
-	c := testConfig(t)
+	ctx := context.Background()
+	c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json")
+	require.NoError(t, err)
+
 	c.values = map[string]any{
 		"int_val":    "this-should-be-an-int",
 		"float_val":  1.1,
 		"bool_val":   true,
 		"string_val": "abcd",
 	}
 
-	err := c.validate()
+	err = c.validate()
 	assert.EqualError(t, err, "validation for template input parameters failed. incorrect type for property int_val: expected type integer, but value is \"this-should-be-an-int\"")
 }
 
@@ -224,19 +270,6 @@ func TestTemplateEnumValidation(t *testing.T) {
 	assert.NoError(t, c.validate())
 }
 
-func TestAssignDefaultValuesWithTemplatedDefaults(t *testing.T) {
-	c := testConfig(t)
-	ctx := context.Background()
-	ctx = root.SetWorkspaceClient(ctx, nil)
-	helpers := loadHelpers(ctx)
-	r, err := newRenderer(ctx, nil, helpers, "./testdata/templated-defaults/template", "./testdata/templated-defaults/library", t.TempDir())
-	require.NoError(t, err)
-
-	err = c.assignDefaultValues(r)
-	assert.NoError(t, err)
-	assert.Equal(t, "my_file", c.values["string_val"])
-}
-
 func TestTemplateSchemaErrorsWithEmptyDescription(t *testing.T) {
 	_, err := newConfig(context.Background(), "./testdata/config-test-schema/invalid-test-schema.json")
 	assert.EqualError(t, err, "template property property-without-description is missing a description")

diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go
@@ -16,6 +16,7 @@ import (
 	bundleConfig "github.com/databricks/cli/bundle/config"
 	"github.com/databricks/cli/bundle/phases"
 	"github.com/databricks/cli/cmd/root"
+	"github.com/databricks/cli/internal/testutil"
 	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/cli/libs/tags"
 	"github.com/databricks/databricks-sdk-go"
@@ -655,15 +656,27 @@ func TestRendererFileTreeRendering(t *testing.T) {
 func TestRendererSubTemplateInPath(t *testing.T) {
 	ctx := context.Background()
 	ctx = root.SetWorkspaceClient(ctx, nil)
-	tmpDir := t.TempDir()
 
-	helpers := loadHelpers(ctx)
-	r, err := newRenderer(ctx, nil, helpers, "./testdata/template-in-path/template", "./testdata/template-in-path/library", tmpDir)
+	// Copy the template directory to a temporary directory where we can safely include a templated file path.
+	// These paths include characters that are forbidden in Go modules, so we can't use the testdata directory.
+	// Also see https://github.com/databricks/cli/pull/1671.
+	templateDir := t.TempDir()
+	testutil.CopyDirectory(t, "./testdata/template-in-path", templateDir)
+
+	// Use a backtick-quoted string; double quotes are a reserved character for Windows paths:
+	// https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file.
+	testutil.Touch(t, filepath.Join(templateDir, "template/{{template `dir_name`}}/{{template `file_name`}}"))
+
+	tmpDir := t.TempDir()
+	r, err := newRenderer(ctx, nil, nil, filepath.Join(templateDir, "template"), filepath.Join(templateDir, "library"), tmpDir)
 	require.NoError(t, err)
 
 	err = r.walk()
 	require.NoError(t, err)
 
-	assert.Equal(t, filepath.Join(tmpDir, "my_directory", "my_file"), r.files[0].DstPath().absPath())
-	assert.Equal(t, "my_directory/my_file", r.files[0].DstPath().relPath)
+	if assert.Len(t, r.files, 2) {
+		f := r.files[1]
+		assert.Equal(t, filepath.Join(tmpDir, "my_directory", "my_file"), f.DstPath().absPath())
+		assert.Equal(t, "my_directory/my_file", f.DstPath().relPath)
+	}
 }
diff --git a/libs/template/testdata/config-assign-from-default-value/schema.json b/libs/template/testdata/config-assign-from-default-value/schema.json
@@ -0,0 +1,24 @@
+{
+    "properties": {
+        "int_val": {
+            "type": "integer",
+            "description": "This is an integer value",
+            "default": 123
+        },
+        "float_val": {
+            "type": "number",
+            "description": "This is a float value",
+            "default": 123
+        },
+        "bool_val": {
+            "type": "boolean",
+            "description": "This is a boolean value",
+            "default": true
+        },
+        "string_val": {
+            "type": "string",
+            "description": "This is a string value",
+            "default": "hello"
+        }
+    }
+}
diff --git a/libs/template/testdata/config-assign-from-file-invalid-int/schema.json b/libs/template/testdata/config-assign-from-file-invalid-int/schema.json
@@ -0,0 +1,20 @@
+{
+    "properties": {
+        "int_val": {
+            "type": "integer",
+            "description": "This is an integer value"
+        },
+        "float_val": {
+            "type": "number",
+            "description": "This is a float value"
+        },
+        "bool_val": {
+            "type": "boolean",
+            "description": "This is a boolean value"
+        },
+        "string_val": {
+            "type": "string",
+            "description": "This is a string value"
+        }
+    }
+}