From 72dde793d88801aa4706d31d5b01028e92eddf60 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Mon, 18 Nov 2024 22:55:24 +0530 Subject: [PATCH 01/24] Fix workspace extensions filer accidentally reading notebooks (#1891) ## Changes The workspace extensions filer should not read or stat a notebook called `foo` if the user calls `.Stat(ctx, "foo")`. Instead, the filer should return a file not found error. This is because the contract for the workspace extensions filer is to only work for notebooks when the file path / name includes the extension (example: `foo.ipynb` or `foo.sql` instead of just `foo`) ## Tests Integration tests. --- internal/filer_test.go | 57 +++++++++++++++++++ .../workspace_files_extensions_client.go | 43 +++++++++++++- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/internal/filer_test.go b/internal/filer_test.go index 20207d34..a2760d91 100644 --- a/internal/filer_test.go +++ b/internal/filer_test.go @@ -723,6 +723,63 @@ func TestAccWorkspaceFilesExtensionsDirectoriesAreNotNotebooks(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) } +func TestAccWorkspaceFilesExtensionsNotebooksAreNotReadAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) + require.NoError(t, err) + + // Reading foo should fail. Even though the WSFS name for the notebook is foo + // reading the notebook should only work with the .ipynb extension. + _, err = wf.Read(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + _, err = wf.Read(ctx, "foo.ipynb") + assert.NoError(t, err) +} + +func TestAccWorkspaceFilesExtensionsNotebooksAreNotStatAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) + require.NoError(t, err) + + // Stating foo should fail. Even though the WSFS name for the notebook is foo + // stating the notebook should only work with the .ipynb extension. + _, err = wf.Stat(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + _, err = wf.Stat(ctx, "foo.ipynb") + assert.NoError(t, err) +} + +func TestAccWorkspaceFilesExtensionsNotebooksAreNotDeletedAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) + require.NoError(t, err) + + // Deleting foo should fail. Even though the WSFS name for the notebook is foo + // deleting the notebook should only work with the .ipynb extension. + err = wf.Delete(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + err = wf.Delete(ctx, "foo.ipynb") + assert.NoError(t, err) +} + func TestAccWorkspaceFilesExtensions_ExportFormatIsPreserved(t *testing.T) { t.Parallel() diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index 53b77dd5..2a605209 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -244,6 +244,17 @@ func (w *workspaceFilesExtensionsClient) Write(ctx context.Context, name string, // Try to read the file as a regular file. If the file is not found, try to read it as a notebook. func (w *workspaceFilesExtensionsClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { + // Ensure that the file / notebook exists. We do this check here to avoid reading + // the content of a notebook called `foo` when the user actually wanted + // to read the content of a file called `foo`. + // + // To read the content of a notebook called `foo` in the workspace the user + // should use the name with the extension included like `foo.ipynb` or `foo.sql`. + _, err := w.Stat(ctx, name) + if err != nil { + return nil, err + } + r, err := w.wsfs.Read(ctx, name) // If the file is not found, it might be a notebook. @@ -276,7 +287,18 @@ func (w *workspaceFilesExtensionsClient) Delete(ctx context.Context, name string return ReadOnlyError{"delete"} } - err := w.wsfs.Delete(ctx, name, mode...) + // Ensure that the file / notebook exists. We do this check here to avoid + // deleting the a notebook called `foo` when the user actually wanted to + // delete a file called `foo`. + // + // To delete a notebook called `foo` in the workspace the user should use the + // name with the extension included like `foo.ipynb` or `foo.sql`. + _, err := w.Stat(ctx, name) + if err != nil { + return err + } + + err = w.wsfs.Delete(ctx, name, mode...) // If the file is not found, it might be a notebook. if errors.As(err, &FileDoesNotExistError{}) { @@ -315,7 +337,24 @@ func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string) return wsfsFileInfo{ObjectInfo: stat.ObjectInfo}, nil } - return info, err + if err != nil { + return nil, err + } + + // If an object is found and it is a notebook, return a FileDoesNotExistError. + // If a notebook is found by the workspace files client, without having stripped + // the extension, this implies that no file with the same name exists. + // + // This check is done to avoid returning the stat for a notebook called `foo` + // when the user actually wanted to stat a file called `foo`. + // + // To stat the metadata of a notebook called `foo` in the workspace the user + // should use the name with the extension included like `foo.ipynb` or `foo.sql`. + if info.Sys().(workspace.ObjectInfo).ObjectType == workspace.ObjectTypeNotebook { + return nil, FileDoesNotExistError{name} + } + + return info, nil } // Note: The import API returns opaque internal errors for namespace clashes From 4fea0219fddee863c20af68da1d5965412d35a2e Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 20 Nov 2024 10:28:35 +0100 Subject: [PATCH 02/24] Use `fs.FS` interface to read template (#1910) ## Changes While working on the v2 of #1744, I found that: * Template initialization first copies built-in templates to a temporary directory before initializing them * Reading a template's contents goes through a `filer.Filer` but is hardcoded to a local one This change updates the interface for reading templates to be `fs.FS`. This is compatible with the `embed.FS` type for the built-in templates, so they no longer have to be copied to a temporary directory before being used. The alternative is to use a `filer.Filer` throughout, but this would have required even more plumbing, and we don't need to _read_ templates, including notebooks, from the workspace filesystem (yet?). As part of making `template.Materialize` take an `fs.FS` argument, the logic to match a given argument to a particular built-in template in the `init` command has moved to sit next to its implementation. ## Tests Existing tests pass. --- cmd/bundle/init.go | 35 +++++++++++++- internal/bundle/helpers.go | 2 +- libs/jsonschema/schema.go | 9 +++- libs/jsonschema/schema_test.go | 7 +++ libs/template/builtin.go | 47 +++++++++++++++++++ libs/template/builtin_test.go | 28 +++++++++++ libs/template/config.go | 6 +-- libs/template/config_test.go | 29 ++++++------ libs/template/file.go | 21 +++++++-- libs/template/file_test.go | 12 ++--- libs/template/helpers_test.go | 16 +++---- libs/template/materialize.go | 77 +++---------------------------- libs/template/materialize_test.go | 6 +-- libs/template/renderer.go | 58 +++++++++++++---------- libs/template/renderer_test.go | 58 ++++++++--------------- 15 files changed, 232 insertions(+), 179 deletions(-) create mode 100644 libs/template/builtin.go create mode 100644 libs/template/builtin_test.go diff --git a/cmd/bundle/init.go b/cmd/bundle/init.go index 7f2c0efc..d31a702a 100644 --- a/cmd/bundle/init.go +++ b/cmd/bundle/init.go @@ -3,6 +3,7 @@ package bundle import ( "errors" "fmt" + "io/fs" "os" "path/filepath" "slices" @@ -109,6 +110,24 @@ func getUrlForNativeTemplate(name string) string { return "" } +func getFsForNativeTemplate(name string) (fs.FS, error) { + builtin, err := template.Builtin() + if err != nil { + return nil, err + } + + // If this is a built-in template, the return value will be non-nil. + var templateFS fs.FS + for _, entry := range builtin { + if entry.Name == name { + templateFS = entry.FS + break + } + } + + return templateFS, nil +} + func isRepoUrl(url string) bool { result := false for _, prefix := range gitUrlPrefixes { @@ -198,9 +217,20 @@ See https://docs.databricks.com/en/dev-tools/bundles/templates.html for more inf if templateDir != "" { return errors.New("--template-dir can only be used with a Git repository URL") } + + templateFS, err := getFsForNativeTemplate(templatePath) + if err != nil { + return err + } + + // If this is not a built-in template, then it must be a local file system path. + if templateFS == nil { + templateFS = os.DirFS(templatePath) + } + // skip downloading the repo because input arg is not a URL. We assume // it's a path on the local file system in that case - return template.Materialize(ctx, configFile, templatePath, outputDir) + return template.Materialize(ctx, configFile, templateFS, outputDir) } // Create a temporary directory with the name of the repository. The '*' @@ -224,7 +254,8 @@ See https://docs.databricks.com/en/dev-tools/bundles/templates.html for more inf // Clean up downloaded repository once the template is materialized. defer os.RemoveAll(repoDir) - return template.Materialize(ctx, configFile, filepath.Join(repoDir, templateDir), outputDir) + templateFS := os.DirFS(filepath.Join(repoDir, templateDir)) + return template.Materialize(ctx, configFile, templateFS, outputDir) } return cmd } diff --git a/internal/bundle/helpers.go b/internal/bundle/helpers.go index 8f1a866f..9740061e 100644 --- a/internal/bundle/helpers.go +++ b/internal/bundle/helpers.go @@ -42,7 +42,7 @@ func initTestTemplateWithBundleRoot(t *testing.T, ctx context.Context, templateN cmd := cmdio.NewIO(flags.OutputJSON, strings.NewReader(""), os.Stdout, os.Stderr, "", "bundles") ctx = cmdio.InContext(ctx, cmd) - err = template.Materialize(ctx, configFilePath, templateRoot, bundleRoot) + err = template.Materialize(ctx, configFilePath, os.DirFS(templateRoot), bundleRoot) return bundleRoot, err } diff --git a/libs/jsonschema/schema.go b/libs/jsonschema/schema.go index 7690ec2f..b9c3fb08 100644 --- a/libs/jsonschema/schema.go +++ b/libs/jsonschema/schema.go @@ -3,7 +3,9 @@ package jsonschema import ( "encoding/json" "fmt" + "io/fs" "os" + "path/filepath" "regexp" "slices" @@ -255,7 +257,12 @@ func (schema *Schema) validate() error { } func Load(path string) (*Schema, error) { - b, err := os.ReadFile(path) + dir, file := filepath.Split(path) + return LoadFS(os.DirFS(dir), file) +} + +func LoadFS(fsys fs.FS, path string) (*Schema, error) { + b, err := fs.ReadFile(fsys, path) if err != nil { return nil, err } diff --git a/libs/jsonschema/schema_test.go b/libs/jsonschema/schema_test.go index cf1f1276..d66868bb 100644 --- a/libs/jsonschema/schema_test.go +++ b/libs/jsonschema/schema_test.go @@ -1,6 +1,7 @@ package jsonschema import ( + "os" "testing" "github.com/stretchr/testify/assert" @@ -305,3 +306,9 @@ func TestValidateSchemaSkippedPropertiesHaveDefaults(t *testing.T) { err = s.validate() assert.NoError(t, err) } + +func TestSchema_LoadFS(t *testing.T) { + fsys := os.DirFS("./testdata/schema-load-int") + _, err := LoadFS(fsys, "schema-valid.json") + assert.NoError(t, err) +} diff --git a/libs/template/builtin.go b/libs/template/builtin.go new file mode 100644 index 00000000..dcb3a885 --- /dev/null +++ b/libs/template/builtin.go @@ -0,0 +1,47 @@ +package template + +import ( + "embed" + "io/fs" +) + +//go:embed all:templates +var builtinTemplates embed.FS + +// BuiltinTemplate represents a template that is built into the CLI. +type BuiltinTemplate struct { + Name string + FS fs.FS +} + +// Builtin returns the list of all built-in templates. +func Builtin() ([]BuiltinTemplate, error) { + templates, err := fs.Sub(builtinTemplates, "templates") + if err != nil { + return nil, err + } + + entries, err := fs.ReadDir(templates, ".") + if err != nil { + return nil, err + } + + var out []BuiltinTemplate + for _, entry := range entries { + if !entry.IsDir() { + continue + } + + templateFS, err := fs.Sub(templates, entry.Name()) + if err != nil { + return nil, err + } + + out = append(out, BuiltinTemplate{ + Name: entry.Name(), + FS: templateFS, + }) + } + + return out, nil +} diff --git a/libs/template/builtin_test.go b/libs/template/builtin_test.go new file mode 100644 index 00000000..504e0acc --- /dev/null +++ b/libs/template/builtin_test.go @@ -0,0 +1,28 @@ +package template + +import ( + "io/fs" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuiltin(t *testing.T) { + out, err := Builtin() + require.NoError(t, err) + assert.Len(t, out, 3) + + // Confirm names. + assert.Equal(t, "dbt-sql", out[0].Name) + assert.Equal(t, "default-python", out[1].Name) + assert.Equal(t, "default-sql", out[2].Name) + + // Confirm that the filesystems work. + _, err = fs.Stat(out[0].FS, `template/{{.project_name}}/dbt_project.yml.tmpl`) + assert.NoError(t, err) + _, err = fs.Stat(out[1].FS, `template/{{.project_name}}/tests/main_test.py.tmpl`) + assert.NoError(t, err) + _, err = fs.Stat(out[2].FS, `template/{{.project_name}}/src/orders_daily.sql.tmpl`) + assert.NoError(t, err) +} diff --git a/libs/template/config.go b/libs/template/config.go index 5470aefe..8e7695b9 100644 --- a/libs/template/config.go +++ b/libs/template/config.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io/fs" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/jsonschema" @@ -28,9 +29,8 @@ type config struct { schema *jsonschema.Schema } -func newConfig(ctx context.Context, schemaPath string) (*config, error) { - // Read config schema - schema, err := jsonschema.Load(schemaPath) +func newConfig(ctx context.Context, templateFS fs.FS, schemaPath string) (*config, error) { + schema, err := jsonschema.LoadFS(templateFS, schemaPath) if err != nil { return nil, err } diff --git a/libs/template/config_test.go b/libs/template/config_test.go index ab9dbeb5..49d3423e 100644 --- a/libs/template/config_test.go +++ b/libs/template/config_test.go @@ -3,6 +3,8 @@ package template import ( "context" "fmt" + "os" + "path" "path/filepath" "testing" "text/template" @@ -16,7 +18,7 @@ func TestTemplateConfigAssignValuesFromFile(t *testing.T) { testDir := "./testdata/config-assign-from-file" ctx := context.Background() - c, err := newConfig(ctx, filepath.Join(testDir, "schema.json")) + c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) err = c.assignValuesFromFile(filepath.Join(testDir, "config.json")) @@ -32,7 +34,7 @@ func TestTemplateConfigAssignValuesFromFileDoesNotOverwriteExistingConfigs(t *te testDir := "./testdata/config-assign-from-file" ctx := context.Background() - c, err := newConfig(ctx, filepath.Join(testDir, "schema.json")) + c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) c.values = map[string]any{ @@ -52,7 +54,7 @@ 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")) + c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) err = c.assignValuesFromFile(filepath.Join(testDir, "config.json")) @@ -63,7 +65,7 @@ func TestTemplateConfigAssignValuesFromFileFiltersPropertiesNotInTheSchema(t *te testDir := "./testdata/config-assign-from-file-unknown-property" ctx := context.Background() - c, err := newConfig(ctx, filepath.Join(testDir, "schema.json")) + c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) err = c.assignValuesFromFile(filepath.Join(testDir, "config.json")) @@ -78,10 +80,10 @@ func TestTemplateConfigAssignValuesFromDefaultValues(t *testing.T) { testDir := "./testdata/config-assign-from-default-value" ctx := context.Background() - c, err := newConfig(ctx, filepath.Join(testDir, "schema.json")) + c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) - r, err := newRenderer(ctx, nil, nil, "./testdata/empty/template", "./testdata/empty/library", t.TempDir()) + r, err := newRenderer(ctx, nil, nil, os.DirFS("."), "./testdata/empty/template", "./testdata/empty/library", t.TempDir()) require.NoError(t, err) err = c.assignDefaultValues(r) @@ -97,10 +99,10 @@ 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")) + c, err := newConfig(ctx, os.DirFS(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()) + r, err := newRenderer(ctx, nil, nil, os.DirFS("."), path.Join(testDir, "template/template"), path.Join(testDir, "template/library"), t.TempDir()) require.NoError(t, err) // Note: only the string value is templated. @@ -116,7 +118,7 @@ func TestTemplateConfigAssignValuesFromTemplatedDefaultValues(t *testing.T) { func TestTemplateConfigValidateValuesDefined(t *testing.T) { ctx := context.Background() - c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json") + c, err := newConfig(ctx, os.DirFS("testdata/config-test-schema"), "test-schema.json") require.NoError(t, err) c.values = map[string]any{ @@ -131,7 +133,7 @@ func TestTemplateConfigValidateValuesDefined(t *testing.T) { func TestTemplateConfigValidateTypeForValidConfig(t *testing.T) { ctx := context.Background() - c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json") + c, err := newConfig(ctx, os.DirFS("testdata/config-test-schema"), "test-schema.json") require.NoError(t, err) c.values = map[string]any{ @@ -147,7 +149,7 @@ func TestTemplateConfigValidateTypeForValidConfig(t *testing.T) { func TestTemplateConfigValidateTypeForUnknownField(t *testing.T) { ctx := context.Background() - c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json") + c, err := newConfig(ctx, os.DirFS("testdata/config-test-schema"), "test-schema.json") require.NoError(t, err) c.values = map[string]any{ @@ -164,7 +166,7 @@ func TestTemplateConfigValidateTypeForUnknownField(t *testing.T) { func TestTemplateConfigValidateTypeForInvalidType(t *testing.T) { ctx := context.Background() - c, err := newConfig(ctx, "testdata/config-test-schema/test-schema.json") + c, err := newConfig(ctx, os.DirFS("testdata/config-test-schema"), "test-schema.json") require.NoError(t, err) c.values = map[string]any{ @@ -271,7 +273,8 @@ func TestTemplateEnumValidation(t *testing.T) { } func TestTemplateSchemaErrorsWithEmptyDescription(t *testing.T) { - _, err := newConfig(context.Background(), "./testdata/config-test-schema/invalid-test-schema.json") + ctx := context.Background() + _, err := newConfig(ctx, os.DirFS("./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/file.go b/libs/template/file.go index aafb1acf..5492ebeb 100644 --- a/libs/template/file.go +++ b/libs/template/file.go @@ -6,8 +6,7 @@ import ( "io/fs" "os" "path/filepath" - - "github.com/databricks/cli/libs/filer" + "slices" ) // Interface representing a file to be materialized from a template into a project @@ -19,6 +18,10 @@ type file interface { // Write file to disk at the destination path. PersistToDisk() error + + // contents returns the file contents as a byte slice. + // This is used for testing purposes. + contents() ([]byte, error) } type destinationPath struct { @@ -46,8 +49,8 @@ type copyFile struct { dstPath *destinationPath - // Filer rooted at template root. Used to read srcPath. - srcFiler filer.Filer + // [fs.FS] rooted at template root. Used to read srcPath. + srcFS fs.FS // Relative path from template root for file to be copied. srcPath string @@ -63,7 +66,7 @@ func (f *copyFile) PersistToDisk() error { if err != nil { return err } - srcFile, err := f.srcFiler.Read(f.ctx, f.srcPath) + srcFile, err := f.srcFS.Open(f.srcPath) if err != nil { return err } @@ -77,6 +80,10 @@ func (f *copyFile) PersistToDisk() error { return err } +func (f *copyFile) contents() ([]byte, error) { + return fs.ReadFile(f.srcFS, f.srcPath) +} + type inMemoryFile struct { dstPath *destinationPath @@ -99,3 +106,7 @@ func (f *inMemoryFile) PersistToDisk() error { } return os.WriteFile(path, f.content, f.perm) } + +func (f *inMemoryFile) contents() ([]byte, error) { + return slices.Clone(f.content), nil +} diff --git a/libs/template/file_test.go b/libs/template/file_test.go index 85938895..e1bd5456 100644 --- a/libs/template/file_test.go +++ b/libs/template/file_test.go @@ -8,7 +8,6 @@ import ( "runtime" "testing" - "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -33,10 +32,7 @@ func testInMemoryFile(t *testing.T, perm fs.FileMode) { func testCopyFile(t *testing.T, perm fs.FileMode) { tmpDir := t.TempDir() - - templateFiler, err := filer.NewLocalClient(tmpDir) - require.NoError(t, err) - err = os.WriteFile(filepath.Join(tmpDir, "source"), []byte("qwerty"), perm) + err := os.WriteFile(filepath.Join(tmpDir, "source"), []byte("qwerty"), perm) require.NoError(t, err) f := ©File{ @@ -45,9 +41,9 @@ func testCopyFile(t *testing.T, perm fs.FileMode) { root: tmpDir, relPath: "a/b/c", }, - perm: perm, - srcPath: "source", - srcFiler: templateFiler, + perm: perm, + srcPath: "source", + srcFS: os.DirFS(tmpDir), } err = f.PersistToDisk() assert.NoError(t, err) diff --git a/libs/template/helpers_test.go b/libs/template/helpers_test.go index 8cc7b928..8a779ecc 100644 --- a/libs/template/helpers_test.go +++ b/libs/template/helpers_test.go @@ -22,7 +22,7 @@ func TestTemplatePrintStringWithoutProcessing(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/print-without-processing/template", "./testdata/print-without-processing/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/print-without-processing/template", "./testdata/print-without-processing/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -39,7 +39,7 @@ func TestTemplateRegexpCompileFunction(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/regexp-compile/template", "./testdata/regexp-compile/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/regexp-compile/template", "./testdata/regexp-compile/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -57,7 +57,7 @@ func TestTemplateRandIntFunction(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/random-int/template", "./testdata/random-int/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/random-int/template", "./testdata/random-int/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -75,7 +75,7 @@ func TestTemplateUuidFunction(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/uuid/template", "./testdata/uuid/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/uuid/template", "./testdata/uuid/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -92,7 +92,7 @@ func TestTemplateUrlFunction(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/urlparse-function/template", "./testdata/urlparse-function/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/urlparse-function/template", "./testdata/urlparse-function/library", tmpDir) require.NoError(t, err) @@ -109,7 +109,7 @@ func TestTemplateMapPairFunction(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/map-pair/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/map-pair/template", "./testdata/map-pair/library", tmpDir) require.NoError(t, err) @@ -132,7 +132,7 @@ func TestWorkspaceHost(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) require.NoError(t, err) @@ -157,7 +157,7 @@ func TestWorkspaceHostNotConfigured(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) assert.NoError(t, err) diff --git a/libs/template/materialize.go b/libs/template/materialize.go index d824bf38..0163eb7d 100644 --- a/libs/template/materialize.go +++ b/libs/template/materialize.go @@ -2,13 +2,9 @@ package template import ( "context" - "embed" "errors" "fmt" "io/fs" - "os" - "path" - "path/filepath" "github.com/databricks/cli/libs/cmdio" ) @@ -17,39 +13,20 @@ const libraryDirName = "library" const templateDirName = "template" const schemaFileName = "databricks_template_schema.json" -//go:embed all:templates -var builtinTemplates embed.FS - // This function materializes the input templates as a project, using user defined // configurations. // Parameters: // // ctx: context containing a cmdio object. This is used to prompt the user // configFilePath: file path containing user defined config values -// templateRoot: root of the template definition +// templateFS: root of the template definition // outputDir: root of directory where to initialize the template -func Materialize(ctx context.Context, configFilePath, templateRoot, outputDir string) error { - // Use a temporary directory in case any builtin templates like default-python are used - tempDir, err := os.MkdirTemp("", "templates") - defer os.RemoveAll(tempDir) - if err != nil { - return err - } - templateRoot, err = prepareBuiltinTemplates(templateRoot, tempDir) - if err != nil { - return err +func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, outputDir string) error { + if _, err := fs.Stat(templateFS, schemaFileName); errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("not a bundle template: expected to find a template schema file at %s", schemaFileName) } - templatePath := filepath.Join(templateRoot, templateDirName) - libraryPath := filepath.Join(templateRoot, libraryDirName) - schemaPath := filepath.Join(templateRoot, schemaFileName) - helpers := loadHelpers(ctx) - - if _, err := os.Stat(schemaPath); errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("not a bundle template: expected to find a template schema file at %s", schemaPath) - } - - config, err := newConfig(ctx, schemaPath) + config, err := newConfig(ctx, templateFS, schemaFileName) if err != nil { return err } @@ -62,7 +39,8 @@ func Materialize(ctx context.Context, configFilePath, templateRoot, outputDir st } } - r, err := newRenderer(ctx, config.values, helpers, templatePath, libraryPath, outputDir) + helpers := loadHelpers(ctx) + r, err := newRenderer(ctx, config.values, helpers, templateFS, templateDirName, libraryDirName, outputDir) if err != nil { return err } @@ -111,44 +89,3 @@ func Materialize(ctx context.Context, configFilePath, templateRoot, outputDir st } return nil } - -// If the given templateRoot matches -func prepareBuiltinTemplates(templateRoot string, tempDir string) (string, error) { - // Check that `templateRoot` is a clean basename, i.e. `some_path` and not `./some_path` or "." - // Return early if that's not the case. - if templateRoot == "." || path.Base(templateRoot) != templateRoot { - return templateRoot, nil - } - - _, err := fs.Stat(builtinTemplates, path.Join("templates", templateRoot)) - if err != nil { - // The given path doesn't appear to be using out built-in templates - return templateRoot, nil - } - - // We have a built-in template with the same name as templateRoot! - // Now we need to make a fully copy of the builtin templates to a real file system - // since template.Parse() doesn't support embed.FS. - err = fs.WalkDir(builtinTemplates, "templates", func(path string, entry fs.DirEntry, err error) error { - if err != nil { - return err - } - - targetPath := filepath.Join(tempDir, path) - if entry.IsDir() { - return os.Mkdir(targetPath, 0755) - } else { - content, err := fs.ReadFile(builtinTemplates, path) - if err != nil { - return err - } - return os.WriteFile(targetPath, content, 0644) - } - }) - - if err != nil { - return "", err - } - - return filepath.Join(tempDir, "templates", templateRoot), nil -} diff --git a/libs/template/materialize_test.go b/libs/template/materialize_test.go index b4be3fe9..dc510a30 100644 --- a/libs/template/materialize_test.go +++ b/libs/template/materialize_test.go @@ -3,7 +3,7 @@ package template import ( "context" "fmt" - "path/filepath" + "os" "testing" "github.com/databricks/cli/cmd/root" @@ -19,6 +19,6 @@ func TestMaterializeForNonTemplateDirectory(t *testing.T) { ctx := root.SetWorkspaceClient(context.Background(), w) // Try to materialize a non-template directory. - err = Materialize(ctx, "", tmpDir, "") - assert.EqualError(t, err, fmt.Sprintf("not a bundle template: expected to find a template schema file at %s", filepath.Join(tmpDir, schemaFileName))) + err = Materialize(ctx, "", os.DirFS(tmpDir), "") + assert.EqualError(t, err, fmt.Sprintf("not a bundle template: expected to find a template schema file at %s", schemaFileName)) } diff --git a/libs/template/renderer.go b/libs/template/renderer.go index 827f3013..bc865039 100644 --- a/libs/template/renderer.go +++ b/libs/template/renderer.go @@ -8,14 +8,12 @@ import ( "io/fs" "os" "path" - "path/filepath" "regexp" "slices" "sort" "strings" "text/template" - "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/logger" ) @@ -52,32 +50,42 @@ type renderer struct { // do not match any glob patterns from this list skipPatterns []string - // Filer rooted at template root. The file tree from this root is walked to - // generate the project - templateFiler filer.Filer + // [fs.FS] that holds the template's file tree. + srcFS fs.FS // Root directory for the project instantiated from the template instanceRoot string } -func newRenderer(ctx context.Context, config map[string]any, helpers template.FuncMap, templateRoot, libraryRoot, instanceRoot string) (*renderer, error) { +func newRenderer( + ctx context.Context, + config map[string]any, + helpers template.FuncMap, + templateFS fs.FS, + templateDir string, + libraryDir string, + instanceRoot string, +) (*renderer, error) { // Initialize new template, with helper functions loaded tmpl := template.New("").Funcs(helpers) - // Load user defined associated templates from the library root - libraryGlob := filepath.Join(libraryRoot, "*") - matches, err := filepath.Glob(libraryGlob) + // Find user-defined templates in the library directory + matches, err := fs.Glob(templateFS, path.Join(libraryDir, "*")) if err != nil { return nil, err } + + // Parse user-defined templates. + // Note: we do not call [ParseFS] with the glob directly because + // it returns an error if no files match the pattern. if len(matches) != 0 { - tmpl, err = tmpl.ParseFiles(matches...) + tmpl, err = tmpl.ParseFS(templateFS, matches...) if err != nil { return nil, err } } - templateFiler, err := filer.NewLocalClient(templateRoot) + srcFS, err := fs.Sub(templateFS, path.Clean(templateDir)) if err != nil { return nil, err } @@ -85,13 +93,13 @@ func newRenderer(ctx context.Context, config map[string]any, helpers template.Fu ctx = log.NewContext(ctx, log.GetLogger(ctx).With("action", "initialize-template")) return &renderer{ - ctx: ctx, - config: config, - baseTemplate: tmpl, - files: make([]file, 0), - skipPatterns: make([]string, 0), - templateFiler: templateFiler, - instanceRoot: instanceRoot, + ctx: ctx, + config: config, + baseTemplate: tmpl, + files: make([]file, 0), + skipPatterns: make([]string, 0), + srcFS: srcFS, + instanceRoot: instanceRoot, }, nil } @@ -141,7 +149,7 @@ func (r *renderer) executeTemplate(templateDefinition string) (string, error) { func (r *renderer) computeFile(relPathTemplate string) (file, error) { // read file permissions - info, err := r.templateFiler.Stat(r.ctx, relPathTemplate) + info, err := fs.Stat(r.srcFS, relPathTemplate) if err != nil { return nil, err } @@ -161,10 +169,10 @@ func (r *renderer) computeFile(relPathTemplate string) (file, error) { root: r.instanceRoot, relPath: relPath, }, - perm: perm, - ctx: r.ctx, - srcPath: relPathTemplate, - srcFiler: r.templateFiler, + perm: perm, + ctx: r.ctx, + srcFS: r.srcFS, + srcPath: relPathTemplate, }, nil } else { // Trim the .tmpl suffix from file name, if specified in the template @@ -173,7 +181,7 @@ func (r *renderer) computeFile(relPathTemplate string) (file, error) { } // read template file's content - templateReader, err := r.templateFiler.Read(r.ctx, relPathTemplate) + templateReader, err := r.srcFS.Open(relPathTemplate) if err != nil { return nil, err } @@ -263,7 +271,7 @@ func (r *renderer) walk() error { // // 2. For directories: They are appended to a slice, which acts as a queue // allowing BFS traversal of the template file tree - entries, err := r.templateFiler.ReadDir(r.ctx, currentDirectory) + entries, err := fs.ReadDir(r.srcFS, currentDirectory) if err != nil { return err } diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 92133c5f..9b8861e7 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -3,9 +3,9 @@ package template import ( "context" "fmt" - "io" "io/fs" "os" + "path" "path/filepath" "runtime" "strings" @@ -41,9 +41,8 @@ func assertFilePermissions(t *testing.T, path string, perm fs.FileMode) { func assertBuiltinTemplateValid(t *testing.T, template string, settings map[string]any, target string, isServicePrincipal bool, build bool, tempDir string) { ctx := context.Background() - templatePath, err := prepareBuiltinTemplates(template, tempDir) + templateFS, err := fs.Sub(builtinTemplates, path.Join("templates", template)) require.NoError(t, err) - libraryPath := filepath.Join(templatePath, "library") w := &databricks.WorkspaceClient{ Config: &workspaceConfig.Config{Host: "https://myhost.com"}, @@ -58,7 +57,7 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - renderer, err := newRenderer(ctx, settings, helpers, templatePath, libraryPath, tempDir) + renderer, err := newRenderer(ctx, settings, helpers, templateFS, templateDirName, libraryDirName, tempDir) require.NoError(t, err) // Evaluate template @@ -67,7 +66,7 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri err = renderer.persistToDisk() require.NoError(t, err) - b, err := bundle.Load(ctx, filepath.Join(tempDir, "template", "my_project")) + b, err := bundle.Load(ctx, filepath.Join(tempDir, "my_project")) require.NoError(t, err) diags := bundle.Apply(ctx, b, phases.LoadNamedTarget(target)) require.NoError(t, diags.Error()) @@ -96,18 +95,6 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri } } -func TestPrepareBuiltInTemplatesWithRelativePaths(t *testing.T) { - // CWD should not be resolved as a built in template - dir, err := prepareBuiltinTemplates(".", t.TempDir()) - assert.NoError(t, err) - assert.Equal(t, ".", dir) - - // relative path should not be resolved as a built in template - dir, err = prepareBuiltinTemplates("./default-python", t.TempDir()) - assert.NoError(t, err) - assert.Equal(t, "./default-python", dir) -} - func TestBuiltinPythonTemplateValid(t *testing.T) { // Test option combinations options := []string{"yes", "no"} @@ -194,7 +181,7 @@ func TestRendererWithAssociatedTemplateInLibrary(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/email/template", "./testdata/email/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/email/template", "./testdata/email/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -381,7 +368,7 @@ func TestRendererWalk(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/walk/template", "./testdata/walk/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/walk/template", "./testdata/walk/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -392,18 +379,9 @@ func TestRendererWalk(t *testing.T) { if f.DstPath().relPath != path { continue } - switch v := f.(type) { - case *inMemoryFile: - return strings.Trim(string(v.content), "\r\n") - case *copyFile: - r, err := r.templateFiler.Read(context.Background(), v.srcPath) - require.NoError(t, err) - b, err := io.ReadAll(r) - require.NoError(t, err) - return strings.Trim(string(b), "\r\n") - default: - require.FailNow(t, "execution should not reach here") - } + b, err := f.contents() + require.NoError(t, err) + return strings.Trim(string(b), "\r\n") } require.FailNow(t, "file is absent: "+path) return "" @@ -422,7 +400,7 @@ func TestRendererFailFunction(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/fail/template", "./testdata/fail/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/fail/template", "./testdata/fail/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -435,7 +413,7 @@ func TestRendererSkipsDirsEagerly(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/skip-dir-eagerly/template", "./testdata/skip-dir-eagerly/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-dir-eagerly/template", "./testdata/skip-dir-eagerly/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -452,7 +430,7 @@ func TestRendererSkipAllFilesInCurrentDirectory(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/skip-all-files-in-cwd/template", "./testdata/skip-all-files-in-cwd/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-all-files-in-cwd/template", "./testdata/skip-all-files-in-cwd/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -475,7 +453,7 @@ func TestRendererSkipPatternsAreRelativeToFileDirectory(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/skip-is-relative/template", "./testdata/skip-is-relative/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-is-relative/template", "./testdata/skip-is-relative/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -493,7 +471,7 @@ func TestRendererSkip(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/skip/template", "./testdata/skip/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip/template", "./testdata/skip/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -525,7 +503,7 @@ func TestRendererReadsPermissionsBits(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/executable-bit-read/template", "./testdata/executable-bit-read/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/executable-bit-read/template", "./testdata/executable-bit-read/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -615,7 +593,7 @@ func TestRendererNonTemplatesAreCreatedAsCopyFiles(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, "./testdata/copy-file-walk/template", "./testdata/copy-file-walk/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/copy-file-walk/template", "./testdata/copy-file-walk/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -635,7 +613,7 @@ func TestRendererFileTreeRendering(t *testing.T) { r, err := newRenderer(ctx, map[string]any{ "dir_name": "my_directory", "file_name": "my_file", - }, helpers, "./testdata/file-tree-rendering/template", "./testdata/file-tree-rendering/library", tmpDir) + }, helpers, os.DirFS("."), "./testdata/file-tree-rendering/template", "./testdata/file-tree-rendering/library", tmpDir) require.NoError(t, err) err = r.walk() @@ -668,7 +646,7 @@ func TestRendererSubTemplateInPath(t *testing.T) { 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) + r, err := newRenderer(ctx, nil, nil, os.DirFS(templateDir), "template", "library", tmpDir) require.NoError(t, err) err = r.walk() From 75b09ff230105eac4ff6a24881289729a8fea64e Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 20 Nov 2024 11:11:31 +0100 Subject: [PATCH 03/24] Use `filer.Filer` to write template instantiation (#1911) ## Changes Prior to this change, the output directory was part of the `renderer` type and passed down to every `file` it produced. Every file knew its absolute destination path. This is incompatible with the use of a filer, where all operations are automatically anchored to some base path. To make this compatible, this change updates: * the `file` type to only know its own path relative to the instantiation root, * the `renderer` type to no longer require or pass along the output directory, * the `persistToDisk` function to take a context and filer argument, * the `filer.WriteMode` to represent permission bits ## Tests * Existing tests pass. * Manually confirmed template initialization works as expected. --- libs/filer/filer.go | 13 +++- libs/filer/filer_test.go | 12 ++++ libs/filer/local_client.go | 13 +++- libs/template/config_test.go | 4 +- libs/template/file.go | 84 ++++++++----------------- libs/template/file_test.go | 62 +++++++------------ libs/template/helpers_test.go | 24 +++---- libs/template/materialize.go | 10 ++- libs/template/renderer.go | 32 +++------- libs/template/renderer_test.go | 110 +++++++++++++++------------------ 10 files changed, 161 insertions(+), 203 deletions(-) create mode 100644 libs/filer/filer_test.go diff --git a/libs/filer/filer.go b/libs/filer/filer.go index fcfbcea0..b5be4c3c 100644 --- a/libs/filer/filer.go +++ b/libs/filer/filer.go @@ -7,13 +7,24 @@ import ( "io/fs" ) +// WriteMode captures intent when writing a file. +// +// The first 9 bits are reserved for the [fs.FileMode] permission bits. +// These are used only by the local filer implementation and have +// no effect for the other implementations. type WriteMode int +// writeModePerm is a mask to extract permission bits from a WriteMode. +const writeModePerm = WriteMode(fs.ModePerm) + const ( - OverwriteIfExists WriteMode = 1 << iota + // Note: these constants are defined as powers of 2 to support combining them using a bit-wise OR. + // They starts from the 10th bit (permission mask + 1) to avoid conflicts with the permission bits. + OverwriteIfExists WriteMode = (writeModePerm + 1) << iota CreateParentDirectories ) +// DeleteMode captures intent when deleting a file. type DeleteMode int const ( diff --git a/libs/filer/filer_test.go b/libs/filer/filer_test.go new file mode 100644 index 00000000..bacea730 --- /dev/null +++ b/libs/filer/filer_test.go @@ -0,0 +1,12 @@ +package filer + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWriteMode(t *testing.T) { + assert.Equal(t, 512, int(OverwriteIfExists)) + assert.Equal(t, 1024, int(CreateParentDirectories)) +} diff --git a/libs/filer/local_client.go b/libs/filer/local_client.go index 48e8a05e..8b25345f 100644 --- a/libs/filer/local_client.go +++ b/libs/filer/local_client.go @@ -28,6 +28,15 @@ func (w *LocalClient) Write(ctx context.Context, name string, reader io.Reader, return err } + // Retrieve permission mask from the [WriteMode], if present. + perm := fs.FileMode(0644) + for _, m := range mode { + bits := m & writeModePerm + if bits != 0 { + perm = fs.FileMode(bits) + } + } + flags := os.O_WRONLY | os.O_CREATE if slices.Contains(mode, OverwriteIfExists) { flags |= os.O_TRUNC @@ -35,7 +44,7 @@ func (w *LocalClient) Write(ctx context.Context, name string, reader io.Reader, flags |= os.O_EXCL } - f, err := os.OpenFile(absPath, flags, 0644) + f, err := os.OpenFile(absPath, flags, perm) if errors.Is(err, fs.ErrNotExist) && slices.Contains(mode, CreateParentDirectories) { // Create parent directories if they don't exist. err = os.MkdirAll(filepath.Dir(absPath), 0755) @@ -43,7 +52,7 @@ func (w *LocalClient) Write(ctx context.Context, name string, reader io.Reader, return err } // Try again. - f, err = os.OpenFile(absPath, flags, 0644) + f, err = os.OpenFile(absPath, flags, perm) } if err != nil { diff --git a/libs/template/config_test.go b/libs/template/config_test.go index 49d3423e..a855019b 100644 --- a/libs/template/config_test.go +++ b/libs/template/config_test.go @@ -83,7 +83,7 @@ func TestTemplateConfigAssignValuesFromDefaultValues(t *testing.T) { c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) - r, err := newRenderer(ctx, nil, nil, os.DirFS("."), "./testdata/empty/template", "./testdata/empty/library", t.TempDir()) + r, err := newRenderer(ctx, nil, nil, os.DirFS("."), "./testdata/empty/template", "./testdata/empty/library") require.NoError(t, err) err = c.assignDefaultValues(r) @@ -102,7 +102,7 @@ func TestTemplateConfigAssignValuesFromTemplatedDefaultValues(t *testing.T) { c, err := newConfig(ctx, os.DirFS(testDir), "schema.json") require.NoError(t, err) - r, err := newRenderer(ctx, nil, nil, os.DirFS("."), path.Join(testDir, "template/template"), path.Join(testDir, "template/library"), t.TempDir()) + r, err := newRenderer(ctx, nil, nil, os.DirFS("."), path.Join(testDir, "template/template"), path.Join(testDir, "template/library")) require.NoError(t, err) // Note: only the string value is templated. diff --git a/libs/template/file.go b/libs/template/file.go index 5492ebeb..36d079b3 100644 --- a/libs/template/file.go +++ b/libs/template/file.go @@ -1,53 +1,36 @@ package template import ( + "bytes" "context" - "io" "io/fs" - "os" - "path/filepath" "slices" + + "github.com/databricks/cli/libs/filer" ) // Interface representing a file to be materialized from a template into a project // instance type file interface { - // Destination path for file. This is where the file will be created when - // PersistToDisk is called. - DstPath() *destinationPath + // Path of the file relative to the root of the instantiated template. + // This is where the file is written to when persisting the template to disk. + // Must be slash-separated. + RelPath() string // Write file to disk at the destination path. - PersistToDisk() error + Write(ctx context.Context, out filer.Filer) error // contents returns the file contents as a byte slice. // This is used for testing purposes. contents() ([]byte, error) } -type destinationPath struct { - // Root path for the project instance. This path uses the system's default - // file separator. For example /foo/bar on Unix and C:\foo\bar on windows - root string - - // Unix like file path relative to the "root" of the instantiated project. Is used to - // evaluate whether the file should be skipped by comparing it to a list of - // skip glob patterns. - relPath string -} - -// Absolute path of the file, in the os native format. For example /foo/bar on -// Unix and C:\foo\bar on windows -func (f *destinationPath) absPath() string { - return filepath.Join(f.root, filepath.FromSlash(f.relPath)) -} - type copyFile struct { - ctx context.Context - // Permissions bits for the destination file perm fs.FileMode - dstPath *destinationPath + // Destination path for the file. + relPath string // [fs.FS] rooted at template root. Used to read srcPath. srcFS fs.FS @@ -56,28 +39,17 @@ type copyFile struct { srcPath string } -func (f *copyFile) DstPath() *destinationPath { - return f.dstPath +func (f *copyFile) RelPath() string { + return f.relPath } -func (f *copyFile) PersistToDisk() error { - path := f.DstPath().absPath() - err := os.MkdirAll(filepath.Dir(path), 0755) +func (f *copyFile) Write(ctx context.Context, out filer.Filer) error { + src, err := f.srcFS.Open(f.srcPath) if err != nil { return err } - srcFile, err := f.srcFS.Open(f.srcPath) - if err != nil { - return err - } - defer srcFile.Close() - dstFile, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, f.perm) - if err != nil { - return err - } - defer dstFile.Close() - _, err = io.Copy(dstFile, srcFile) - return err + defer src.Close() + return out.Write(ctx, f.relPath, src, filer.CreateParentDirectories, filer.WriteMode(f.perm)) } func (f *copyFile) contents() ([]byte, error) { @@ -85,26 +57,22 @@ func (f *copyFile) contents() ([]byte, error) { } type inMemoryFile struct { - dstPath *destinationPath - - content []byte - // Permissions bits for the destination file perm fs.FileMode + + // Destination path for the file. + relPath string + + // Contents of the file. + content []byte } -func (f *inMemoryFile) DstPath() *destinationPath { - return f.dstPath +func (f *inMemoryFile) RelPath() string { + return f.relPath } -func (f *inMemoryFile) PersistToDisk() error { - path := f.DstPath().absPath() - - err := os.MkdirAll(filepath.Dir(path), 0755) - if err != nil { - return err - } - return os.WriteFile(path, f.content, f.perm) +func (f *inMemoryFile) Write(ctx context.Context, out filer.Filer) error { + return out.Write(ctx, f.relPath, bytes.NewReader(f.content), filer.CreateParentDirectories, filer.WriteMode(f.perm)) } func (f *inMemoryFile) contents() ([]byte, error) { diff --git a/libs/template/file_test.go b/libs/template/file_test.go index e1bd5456..bd5f6d63 100644 --- a/libs/template/file_test.go +++ b/libs/template/file_test.go @@ -8,77 +8,56 @@ import ( "runtime" "testing" + "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func testInMemoryFile(t *testing.T, perm fs.FileMode) { +func testInMemoryFile(t *testing.T, ctx context.Context, perm fs.FileMode) { tmpDir := t.TempDir() f := &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a/b/c", - }, perm: perm, + relPath: "a/b/c", content: []byte("123"), } - err := f.PersistToDisk() + + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = f.Write(ctx, out) assert.NoError(t, err) assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "123") assertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) } -func testCopyFile(t *testing.T, perm fs.FileMode) { +func testCopyFile(t *testing.T, ctx context.Context, perm fs.FileMode) { tmpDir := t.TempDir() err := os.WriteFile(filepath.Join(tmpDir, "source"), []byte("qwerty"), perm) require.NoError(t, err) f := ©File{ - ctx: context.Background(), - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a/b/c", - }, perm: perm, - srcPath: "source", + relPath: "a/b/c", srcFS: os.DirFS(tmpDir), + srcPath: "source", } - err = f.PersistToDisk() + + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = f.Write(ctx, out) assert.NoError(t, err) assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "qwerty") assertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) } -func TestTemplateFileDestinationPath(t *testing.T) { - if runtime.GOOS == "windows" { - t.SkipNow() - } - f := &destinationPath{ - root: `a/b/c`, - relPath: "d/e", - } - assert.Equal(t, `a/b/c/d/e`, f.absPath()) -} - -func TestTemplateFileDestinationPathForWindows(t *testing.T) { - if runtime.GOOS != "windows" { - t.SkipNow() - } - f := &destinationPath{ - root: `c:\a\b\c`, - relPath: "d/e", - } - assert.Equal(t, `c:\a\b\c\d\e`, f.absPath()) -} - func TestTemplateInMemoryFilePersistToDisk(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } - testInMemoryFile(t, 0755) + ctx := context.Background() + testInMemoryFile(t, ctx, 0755) } func TestTemplateInMemoryFilePersistToDiskForWindows(t *testing.T) { @@ -87,14 +66,16 @@ func TestTemplateInMemoryFilePersistToDiskForWindows(t *testing.T) { } // we have separate tests for windows because of differences in valid // fs.FileMode values we can use for different operating systems. - testInMemoryFile(t, 0666) + ctx := context.Background() + testInMemoryFile(t, ctx, 0666) } func TestTemplateCopyFilePersistToDisk(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } - testCopyFile(t, 0644) + ctx := context.Background() + testCopyFile(t, ctx, 0644) } func TestTemplateCopyFilePersistToDiskForWindows(t *testing.T) { @@ -103,5 +84,6 @@ func TestTemplateCopyFilePersistToDiskForWindows(t *testing.T) { } // we have separate tests for windows because of differences in valid // fs.FileMode values we can use for different operating systems. - testCopyFile(t, 0666) + ctx := context.Background() + testCopyFile(t, ctx, 0666) } diff --git a/libs/template/helpers_test.go b/libs/template/helpers_test.go index 8a779ecc..9f5804c0 100644 --- a/libs/template/helpers_test.go +++ b/libs/template/helpers_test.go @@ -18,11 +18,10 @@ import ( func TestTemplatePrintStringWithoutProcessing(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/print-without-processing/template", "./testdata/print-without-processing/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/print-without-processing/template", "./testdata/print-without-processing/library") require.NoError(t, err) err = r.walk() @@ -35,11 +34,10 @@ func TestTemplatePrintStringWithoutProcessing(t *testing.T) { func TestTemplateRegexpCompileFunction(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/regexp-compile/template", "./testdata/regexp-compile/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/regexp-compile/template", "./testdata/regexp-compile/library") require.NoError(t, err) err = r.walk() @@ -53,11 +51,10 @@ func TestTemplateRegexpCompileFunction(t *testing.T) { func TestTemplateRandIntFunction(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/random-int/template", "./testdata/random-int/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/random-int/template", "./testdata/random-int/library") require.NoError(t, err) err = r.walk() @@ -71,11 +68,10 @@ func TestTemplateRandIntFunction(t *testing.T) { func TestTemplateUuidFunction(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/uuid/template", "./testdata/uuid/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/uuid/template", "./testdata/uuid/library") require.NoError(t, err) err = r.walk() @@ -88,11 +84,10 @@ func TestTemplateUuidFunction(t *testing.T) { func TestTemplateUrlFunction(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/urlparse-function/template", "./testdata/urlparse-function/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/urlparse-function/template", "./testdata/urlparse-function/library") require.NoError(t, err) @@ -105,11 +100,10 @@ func TestTemplateUrlFunction(t *testing.T) { func TestTemplateMapPairFunction(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/map-pair/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/map-pair/template", "./testdata/map-pair/library") require.NoError(t, err) @@ -122,7 +116,6 @@ func TestTemplateMapPairFunction(t *testing.T) { func TestWorkspaceHost(t *testing.T) { ctx := context.Background() - tmpDir := t.TempDir() w := &databricks.WorkspaceClient{ Config: &workspaceConfig.Config{ @@ -132,7 +125,7 @@ func TestWorkspaceHost(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library") require.NoError(t, err) @@ -149,7 +142,6 @@ func TestWorkspaceHostNotConfigured(t *testing.T) { ctx := context.Background() cmd := cmdio.NewIO(flags.OutputJSON, strings.NewReader(""), os.Stdout, os.Stderr, "", "template") ctx = cmdio.InContext(ctx, cmd) - tmpDir := t.TempDir() w := &databricks.WorkspaceClient{ Config: &workspaceConfig.Config{}, @@ -157,7 +149,7 @@ func TestWorkspaceHostNotConfigured(t *testing.T) { ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/workspace-host/template", "./testdata/map-pair/library") assert.NoError(t, err) diff --git a/libs/template/materialize.go b/libs/template/materialize.go index 0163eb7d..8338e119 100644 --- a/libs/template/materialize.go +++ b/libs/template/materialize.go @@ -7,6 +7,7 @@ import ( "io/fs" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/filer" ) const libraryDirName = "library" @@ -40,7 +41,7 @@ func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, o } helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, config.values, helpers, templateFS, templateDirName, libraryDirName, outputDir) + r, err := newRenderer(ctx, config.values, helpers, templateFS, templateDirName, libraryDirName) if err != nil { return err } @@ -72,7 +73,12 @@ func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, o return err } - err = r.persistToDisk() + out, err := filer.NewLocalClient(outputDir) + if err != nil { + return err + } + + err = r.persistToDisk(ctx, out) if err != nil { return err } diff --git a/libs/template/renderer.go b/libs/template/renderer.go index bc865039..0f30a67d 100644 --- a/libs/template/renderer.go +++ b/libs/template/renderer.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "io/fs" - "os" "path" "regexp" "slices" @@ -14,6 +13,7 @@ import ( "strings" "text/template" + "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/logger" ) @@ -52,9 +52,6 @@ type renderer struct { // [fs.FS] that holds the template's file tree. srcFS fs.FS - - // Root directory for the project instantiated from the template - instanceRoot string } func newRenderer( @@ -64,7 +61,6 @@ func newRenderer( templateFS fs.FS, templateDir string, libraryDir string, - instanceRoot string, ) (*renderer, error) { // Initialize new template, with helper functions loaded tmpl := template.New("").Funcs(helpers) @@ -99,7 +95,6 @@ func newRenderer( files: make([]file, 0), skipPatterns: make([]string, 0), srcFS: srcFS, - instanceRoot: instanceRoot, }, nil } @@ -165,12 +160,8 @@ func (r *renderer) computeFile(relPathTemplate string) (file, error) { // over as is, without treating it as a template if !strings.HasSuffix(relPathTemplate, templateExtension) { return ©File{ - dstPath: &destinationPath{ - root: r.instanceRoot, - relPath: relPath, - }, perm: perm, - ctx: r.ctx, + relPath: relPath, srcFS: r.srcFS, srcPath: relPathTemplate, }, nil @@ -202,11 +193,8 @@ func (r *renderer) computeFile(relPathTemplate string) (file, error) { } return &inMemoryFile{ - dstPath: &destinationPath{ - root: r.instanceRoot, - relPath: relPath, - }, perm: perm, + relPath: relPath, content: []byte(content), }, nil } @@ -291,7 +279,7 @@ func (r *renderer) walk() error { if err != nil { return err } - logger.Infof(r.ctx, "added file to list of possible project files: %s", f.DstPath().relPath) + logger.Infof(r.ctx, "added file to list of possible project files: %s", f.RelPath()) r.files = append(r.files, f) } @@ -299,17 +287,17 @@ func (r *renderer) walk() error { return nil } -func (r *renderer) persistToDisk() error { +func (r *renderer) persistToDisk(ctx context.Context, out filer.Filer) error { // Accumulate files which we will persist, skipping files whose path matches // any of the skip patterns filesToPersist := make([]file, 0) for _, file := range r.files { - match, err := isSkipped(file.DstPath().relPath, r.skipPatterns) + match, err := isSkipped(file.RelPath(), r.skipPatterns) if err != nil { return err } if match { - log.Infof(r.ctx, "skipping file: %s", file.DstPath()) + log.Infof(r.ctx, "skipping file: %s", file.RelPath()) continue } filesToPersist = append(filesToPersist, file) @@ -317,8 +305,8 @@ func (r *renderer) persistToDisk() error { // Assert no conflicting files exist for _, file := range filesToPersist { - path := file.DstPath().absPath() - _, err := os.Stat(path) + path := file.RelPath() + _, err := out.Stat(ctx, path) if err == nil { return fmt.Errorf("failed to initialize template, one or more files already exist: %s", path) } @@ -329,7 +317,7 @@ func (r *renderer) persistToDisk() error { // Persist files to disk for _, file := range filesToPersist { - err := file.PersistToDisk() + err := file.Write(ctx, out) if err != nil { return err } diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 9b8861e7..a4b9166d 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -18,6 +18,7 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/tags" "github.com/databricks/databricks-sdk-go" workspaceConfig "github.com/databricks/databricks-sdk-go/config" @@ -57,13 +58,15 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri ctx = root.SetWorkspaceClient(ctx, w) helpers := loadHelpers(ctx) - renderer, err := newRenderer(ctx, settings, helpers, templateFS, templateDirName, libraryDirName, tempDir) + renderer, err := newRenderer(ctx, settings, helpers, templateFS, templateDirName, libraryDirName) require.NoError(t, err) // Evaluate template err = renderer.walk() require.NoError(t, err) - err = renderer.persistToDisk() + out, err := filer.NewLocalClient(tempDir) + require.NoError(t, err) + err = renderer.persistToDisk(ctx, out) require.NoError(t, err) b, err := bundle.Load(ctx, filepath.Join(tempDir, "my_project")) @@ -181,13 +184,14 @@ func TestRendererWithAssociatedTemplateInLibrary(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/email/template", "./testdata/email/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/email/template", "./testdata/email/library") require.NoError(t, err) err = r.walk() require.NoError(t, err) - - err = r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) require.NoError(t, err) b, err := os.ReadFile(filepath.Join(tmpDir, "my_email")) @@ -312,45 +316,34 @@ func TestRendererPersistToDisk(t *testing.T) { r := &renderer{ ctx: ctx, - instanceRoot: tmpDir, skipPatterns: []string{"a/b/c", "mn*"}, files: []file{ &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a/b/c", - }, perm: 0444, + relPath: "a/b/c", content: nil, }, &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "mno", - }, perm: 0444, + relPath: "mno", content: nil, }, &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a/b/d", - }, perm: 0444, + relPath: "a/b/d", content: []byte("123"), }, &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "mmnn", - }, perm: 0444, + relPath: "mmnn", content: []byte("456"), }, }, } - err := r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) require.NoError(t, err) assert.NoFileExists(t, filepath.Join(tmpDir, "a", "b", "c")) @@ -365,10 +358,9 @@ func TestRendererPersistToDisk(t *testing.T) { func TestRendererWalk(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) - tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/walk/template", "./testdata/walk/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/walk/template", "./testdata/walk/library") require.NoError(t, err) err = r.walk() @@ -376,7 +368,7 @@ func TestRendererWalk(t *testing.T) { getContent := func(r *renderer, path string) string { for _, f := range r.files { - if f.DstPath().relPath != path { + if f.RelPath() != path { continue } b, err := f.contents() @@ -397,10 +389,9 @@ func TestRendererWalk(t *testing.T) { func TestRendererFailFunction(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) - tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/fail/template", "./testdata/fail/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/fail/template", "./testdata/fail/library") require.NoError(t, err) err = r.walk() @@ -410,10 +401,9 @@ func TestRendererFailFunction(t *testing.T) { func TestRendererSkipsDirsEagerly(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) - tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-dir-eagerly/template", "./testdata/skip-dir-eagerly/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-dir-eagerly/template", "./testdata/skip-dir-eagerly/library") require.NoError(t, err) err = r.walk() @@ -430,7 +420,7 @@ func TestRendererSkipAllFilesInCurrentDirectory(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-all-files-in-cwd/template", "./testdata/skip-all-files-in-cwd/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-all-files-in-cwd/template", "./testdata/skip-all-files-in-cwd/library") require.NoError(t, err) err = r.walk() @@ -438,7 +428,9 @@ func TestRendererSkipAllFilesInCurrentDirectory(t *testing.T) { // All 3 files are executed and have in memory representations require.Len(t, r.files, 3) - err = r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) require.NoError(t, err) entries, err := os.ReadDir(tmpDir) @@ -450,10 +442,9 @@ func TestRendererSkipAllFilesInCurrentDirectory(t *testing.T) { func TestRendererSkipPatternsAreRelativeToFileDirectory(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) - tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-is-relative/template", "./testdata/skip-is-relative/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip-is-relative/template", "./testdata/skip-is-relative/library") require.NoError(t, err) err = r.walk() @@ -471,7 +462,7 @@ func TestRendererSkip(t *testing.T) { tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip/template", "./testdata/skip/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/skip/template", "./testdata/skip/library") require.NoError(t, err) err = r.walk() @@ -480,7 +471,9 @@ func TestRendererSkip(t *testing.T) { // This is because "dir2/*" matches the files in dir2, but not dir2 itself assert.Len(t, r.files, 6) - err = r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) require.NoError(t, err) assert.FileExists(t, filepath.Join(tmpDir, "file1")) @@ -498,12 +491,11 @@ func TestRendererReadsPermissionsBits(t *testing.T) { if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { t.SkipNow() } - tmpDir := t.TempDir() ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/executable-bit-read/template", "./testdata/executable-bit-read/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/executable-bit-read/template", "./testdata/executable-bit-read/library") require.NoError(t, err) err = r.walk() @@ -511,7 +503,7 @@ func TestRendererReadsPermissionsBits(t *testing.T) { getPermissions := func(r *renderer, path string) fs.FileMode { for _, f := range r.files { - if f.DstPath().relPath != path { + if f.RelPath() != path { continue } switch v := f.(type) { @@ -534,6 +526,7 @@ func TestRendererReadsPermissionsBits(t *testing.T) { func TestRendererErrorOnConflictingFile(t *testing.T) { tmpDir := t.TempDir() + ctx := context.Background() f, err := os.Create(filepath.Join(tmpDir, "a")) require.NoError(t, err) @@ -544,17 +537,16 @@ func TestRendererErrorOnConflictingFile(t *testing.T) { skipPatterns: []string{}, files: []file{ &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a", - }, perm: 0444, + relPath: "a", content: []byte("123"), }, }, } - err = r.persistToDisk() - assert.EqualError(t, err, fmt.Sprintf("failed to initialize template, one or more files already exist: %s", filepath.Join(tmpDir, "a"))) + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) + assert.EqualError(t, err, fmt.Sprintf("failed to initialize template, one or more files already exist: %s", "a")) } func TestRendererNoErrorOnConflictingFileIfSkipped(t *testing.T) { @@ -571,16 +563,15 @@ func TestRendererNoErrorOnConflictingFileIfSkipped(t *testing.T) { skipPatterns: []string{"a"}, files: []file{ &inMemoryFile{ - dstPath: &destinationPath{ - root: tmpDir, - relPath: "a", - }, perm: 0444, + relPath: "a", content: []byte("123"), }, }, } - err = r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) // No error is returned even though a conflicting file exists. This is because // the generated file is being skipped assert.NoError(t, err) @@ -590,10 +581,9 @@ func TestRendererNoErrorOnConflictingFileIfSkipped(t *testing.T) { func TestRendererNonTemplatesAreCreatedAsCopyFiles(t *testing.T) { ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, nil) - tmpDir := t.TempDir() helpers := loadHelpers(ctx) - r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/copy-file-walk/template", "./testdata/copy-file-walk/library", tmpDir) + r, err := newRenderer(ctx, nil, helpers, os.DirFS("."), "./testdata/copy-file-walk/template", "./testdata/copy-file-walk/library") require.NoError(t, err) err = r.walk() @@ -601,7 +591,7 @@ func TestRendererNonTemplatesAreCreatedAsCopyFiles(t *testing.T) { assert.Len(t, r.files, 1) assert.Equal(t, r.files[0].(*copyFile).srcPath, "not-a-template") - assert.Equal(t, r.files[0].DstPath().absPath(), filepath.Join(tmpDir, "not-a-template")) + assert.Equal(t, r.files[0].RelPath(), "not-a-template") } func TestRendererFileTreeRendering(t *testing.T) { @@ -613,7 +603,7 @@ func TestRendererFileTreeRendering(t *testing.T) { r, err := newRenderer(ctx, map[string]any{ "dir_name": "my_directory", "file_name": "my_file", - }, helpers, os.DirFS("."), "./testdata/file-tree-rendering/template", "./testdata/file-tree-rendering/library", tmpDir) + }, helpers, os.DirFS("."), "./testdata/file-tree-rendering/template", "./testdata/file-tree-rendering/library") require.NoError(t, err) err = r.walk() @@ -621,9 +611,11 @@ func TestRendererFileTreeRendering(t *testing.T) { // Assert in memory representation is created. assert.Len(t, r.files, 1) - assert.Equal(t, r.files[0].DstPath().absPath(), filepath.Join(tmpDir, "my_directory", "my_file")) + assert.Equal(t, r.files[0].RelPath(), "my_directory/my_file") - err = r.persistToDisk() + out, err := filer.NewLocalClient(tmpDir) + require.NoError(t, err) + err = r.persistToDisk(ctx, out) require.NoError(t, err) // Assert files and directories are correctly materialized. @@ -645,8 +637,7 @@ func TestRendererSubTemplateInPath(t *testing.T) { // 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, os.DirFS(templateDir), "template", "library", tmpDir) + r, err := newRenderer(ctx, nil, nil, os.DirFS(templateDir), "template", "library") require.NoError(t, err) err = r.walk() @@ -654,7 +645,6 @@ func TestRendererSubTemplateInPath(t *testing.T) { 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) + assert.Equal(t, "my_directory/my_file", f.RelPath()) } } From 886e14910cd921e08bc66b90c46192b85d4756bd Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 20 Nov 2024 12:42:23 +0100 Subject: [PATCH 04/24] Fix template initialization when running on Databricks (#1912) ## Changes When running the CLI on Databricks Runtime (DBR), use the extension-aware filer to write an instantiated template if the instance path is located in the workspace filesystem. Notebooks cannot be written through the workspace filesystem's FUSE mount. As a result, this is the only method for initializing templates that contain notebooks when running the CLI on DBR and writing to the workspace filesystem. Depends on #1910 and #1911. Supersedes #1744. ## Tests * Manually confirmed I can initialize a template with notebooks when running the CLI from the web terminal. --- cmd/bundle/init.go | 32 +++++++++++++++++++++++++++++-- internal/bundle/helpers.go | 5 ++++- libs/template/materialize.go | 11 +++-------- libs/template/materialize_test.go | 2 +- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/cmd/bundle/init.go b/cmd/bundle/init.go index d31a702a..687c141e 100644 --- a/cmd/bundle/init.go +++ b/cmd/bundle/init.go @@ -1,6 +1,7 @@ package bundle import ( + "context" "errors" "fmt" "io/fs" @@ -11,6 +12,8 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/dbr" + "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/template" "github.com/spf13/cobra" @@ -147,6 +150,26 @@ func repoName(url string) string { return parts[len(parts)-1] } +func constructOutputFiler(ctx context.Context, outputDir string) (filer.Filer, error) { + outputDir, err := filepath.Abs(outputDir) + if err != nil { + return nil, err + } + + // If the CLI is running on DBR and we're writing to the workspace file system, + // use the extension-aware workspace filesystem filer to instantiate the template. + // + // It is not possible to write notebooks through the workspace filesystem's FUSE mount. + // Therefore this is the only way we can initialize templates that contain notebooks + // when running the CLI on DBR and initializing a template to the workspace. + // + if strings.HasPrefix(outputDir, "/Workspace/") && dbr.RunsOnRuntime(ctx) { + return filer.NewWorkspaceFilesExtensionsClient(root.WorkspaceClient(ctx), outputDir) + } + + return filer.NewLocalClient(outputDir) +} + func newInitCommand() *cobra.Command { cmd := &cobra.Command{ Use: "init [TEMPLATE_PATH]", @@ -201,6 +224,11 @@ See https://docs.databricks.com/en/dev-tools/bundles/templates.html for more inf templatePath = getNativeTemplateByDescription(description) } + outputFiler, err := constructOutputFiler(ctx, outputDir) + if err != nil { + return err + } + if templatePath == customTemplate { cmdio.LogString(ctx, "Please specify a path or Git repository to use a custom template.") cmdio.LogString(ctx, "See https://docs.databricks.com/en/dev-tools/bundles/templates.html to learn more about custom templates.") @@ -230,7 +258,7 @@ See https://docs.databricks.com/en/dev-tools/bundles/templates.html for more inf // skip downloading the repo because input arg is not a URL. We assume // it's a path on the local file system in that case - return template.Materialize(ctx, configFile, templateFS, outputDir) + return template.Materialize(ctx, configFile, templateFS, outputFiler) } // Create a temporary directory with the name of the repository. The '*' @@ -255,7 +283,7 @@ See https://docs.databricks.com/en/dev-tools/bundles/templates.html for more inf // Clean up downloaded repository once the template is materialized. defer os.RemoveAll(repoDir) templateFS := os.DirFS(filepath.Join(repoDir, templateDir)) - return template.Materialize(ctx, configFile, templateFS, outputDir) + return template.Materialize(ctx, configFile, templateFS, outputFiler) } return cmd } diff --git a/internal/bundle/helpers.go b/internal/bundle/helpers.go index 9740061e..dd9c841c 100644 --- a/internal/bundle/helpers.go +++ b/internal/bundle/helpers.go @@ -16,6 +16,7 @@ import ( "github.com/databricks/cli/internal" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/flags" "github.com/databricks/cli/libs/template" "github.com/databricks/cli/libs/vfs" @@ -42,7 +43,9 @@ func initTestTemplateWithBundleRoot(t *testing.T, ctx context.Context, templateN cmd := cmdio.NewIO(flags.OutputJSON, strings.NewReader(""), os.Stdout, os.Stderr, "", "bundles") ctx = cmdio.InContext(ctx, cmd) - err = template.Materialize(ctx, configFilePath, os.DirFS(templateRoot), bundleRoot) + out, err := filer.NewLocalClient(bundleRoot) + require.NoError(t, err) + err = template.Materialize(ctx, configFilePath, os.DirFS(templateRoot), out) return bundleRoot, err } diff --git a/libs/template/materialize.go b/libs/template/materialize.go index 8338e119..ee30444a 100644 --- a/libs/template/materialize.go +++ b/libs/template/materialize.go @@ -21,8 +21,8 @@ const schemaFileName = "databricks_template_schema.json" // ctx: context containing a cmdio object. This is used to prompt the user // configFilePath: file path containing user defined config values // templateFS: root of the template definition -// outputDir: root of directory where to initialize the template -func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, outputDir string) error { +// outputFiler: filer to use for writing the initialized template +func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, outputFiler filer.Filer) error { if _, err := fs.Stat(templateFS, schemaFileName); errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("not a bundle template: expected to find a template schema file at %s", schemaFileName) } @@ -73,12 +73,7 @@ func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, o return err } - out, err := filer.NewLocalClient(outputDir) - if err != nil { - return err - } - - err = r.persistToDisk(ctx, out) + err = r.persistToDisk(ctx, outputFiler) if err != nil { return err } diff --git a/libs/template/materialize_test.go b/libs/template/materialize_test.go index dc510a30..f7cd916e 100644 --- a/libs/template/materialize_test.go +++ b/libs/template/materialize_test.go @@ -19,6 +19,6 @@ func TestMaterializeForNonTemplateDirectory(t *testing.T) { ctx := root.SetWorkspaceClient(context.Background(), w) // Try to materialize a non-template directory. - err = Materialize(ctx, "", os.DirFS(tmpDir), "") + err = Materialize(ctx, "", os.DirFS(tmpDir), nil) assert.EqualError(t, err, fmt.Sprintf("not a bundle template: expected to find a template schema file at %s", schemaFileName)) } From 756e55fabceaf91669a8df682562712a3162da53 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Wed, 20 Nov 2024 13:22:27 +0100 Subject: [PATCH 05/24] Source-linked deployments for bundles in the workspace (#1884) ## Changes This change adds a preset for source-linked deployments. It is enabled by default for targets in `development` mode **if** the Databricks CLI is running from the `/Workspace` directory on DBR. It does not have an effect when running the CLI anywhere else. Key highlights: 1. Files in this mode won't be uploaded to workspace 2. Created resources will use references to source files instead of their workspace copies ## Tests 1. Apply preset unit test covering conditional logic 2. High-level process target mode unit test for testing integration between mutators --------- Co-authored-by: Pieter Noordhuis --- bundle/config/mutator/apply_presets.go | 10 ++ bundle/config/mutator/apply_presets_test.go | 85 +++++++++ bundle/config/mutator/process_target_mode.go | 9 + .../mutator/process_target_mode_test.go | 33 ++++ bundle/config/mutator/translate_paths.go | 10 +- bundle/config/mutator/translate_paths_test.go | 161 ++++++++++++++++++ bundle/config/presets.go | 5 + bundle/deploy/files/upload.go | 6 + bundle/trampoline/python_dbr_warning.go | 4 + 9 files changed, 321 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 59b8547b..9cec704e 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/textutil" @@ -221,6 +222,15 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos dashboard.DisplayName = prefix + dashboard.DisplayName } + if config.IsExplicitlyEnabled((b.Config.Presets.SourceLinkedDeployment)) { + isDatabricksWorkspace := dbr.RunsOnRuntime(ctx) && strings.HasPrefix(b.SyncRootPath, "/Workspace/") + if !isDatabricksWorkspace { + disabled := false + b.Config.Presets.SourceLinkedDeployment = &disabled + diags = diags.Extend(diag.Warningf("source-linked deployment is available only in the Databricks Workspace")) + } + } + return diags } diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 24295da4..f11a45d6 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -2,12 +2,14 @@ package mutator_test import ( "context" + "runtime" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/dbr" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -364,3 +366,86 @@ func TestApplyPresetsResourceNotDefined(t *testing.T) { }) } } + +func TestApplyPresetsSourceLinkedDeployment(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("this test is not applicable on Windows because source-linked mode works only in the Databricks Workspace") + } + + testContext := context.Background() + enabled := true + disabled := false + workspacePath := "/Workspace/user.name@company.com" + + tests := []struct { + bundlePath string + ctx context.Context + name string + initialValue *bool + expectedValue *bool + expectedWarning string + }{ + { + name: "preset enabled, bundle in Workspace, databricks runtime", + bundlePath: workspacePath, + ctx: dbr.MockRuntime(testContext, true), + initialValue: &enabled, + expectedValue: &enabled, + }, + { + name: "preset enabled, bundle not in Workspace, databricks runtime", + bundlePath: "/Users/user.name@company.com", + ctx: dbr.MockRuntime(testContext, true), + initialValue: &enabled, + expectedValue: &disabled, + expectedWarning: "source-linked deployment is available only in the Databricks Workspace", + }, + { + name: "preset enabled, bundle in Workspace, not databricks runtime", + bundlePath: workspacePath, + ctx: dbr.MockRuntime(testContext, false), + initialValue: &enabled, + expectedValue: &disabled, + expectedWarning: "source-linked deployment is available only in the Databricks Workspace", + }, + { + name: "preset disabled, bundle in Workspace, databricks runtime", + bundlePath: workspacePath, + ctx: dbr.MockRuntime(testContext, true), + initialValue: &disabled, + expectedValue: &disabled, + }, + { + name: "preset nil, bundle in Workspace, databricks runtime", + bundlePath: workspacePath, + ctx: dbr.MockRuntime(testContext, true), + initialValue: nil, + expectedValue: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &bundle.Bundle{ + SyncRootPath: tt.bundlePath, + Config: config.Root{ + Presets: config.Presets{ + SourceLinkedDeployment: tt.initialValue, + }, + }, + } + + diags := bundle.Apply(tt.ctx, b, mutator.ApplyPresets()) + if diags.HasError() { + t.Fatalf("unexpected error: %v", diags) + } + + if tt.expectedWarning != "" { + require.Equal(t, tt.expectedWarning, diags[0].Summary) + } + + require.Equal(t, tt.expectedValue, b.Config.Presets.SourceLinkedDeployment) + }) + } + +} diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 44b53681..df0136fa 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/iamutil" @@ -57,6 +58,14 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { t.TriggerPauseStatus = config.Paused } + if !config.IsExplicitlyDisabled(t.SourceLinkedDeployment) { + isInWorkspace := strings.HasPrefix(b.SyncRootPath, "/Workspace/") + if isInWorkspace && dbr.RunsOnRuntime(ctx) { + enabled := true + t.SourceLinkedDeployment = &enabled + } + } + if !config.IsExplicitlyDisabled(t.PipelinesDevelopment) { enabled := true t.PipelinesDevelopment = &enabled diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 4135d5fd..c5ea9ade 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -3,14 +3,17 @@ package mutator import ( "context" "reflect" + "runtime" "strings" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/tags" + "github.com/databricks/cli/libs/vfs" sdkconfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/compute" @@ -140,6 +143,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { }, }, }, + SyncRoot: vfs.MustNew("/Users/lennart.kats@databricks.com"), // Use AWS implementation for testing. Tagging: tags.ForCloud(&sdkconfig.Config{ Host: "https://company.cloud.databricks.com", @@ -522,3 +526,32 @@ func TestPipelinesDevelopmentDisabled(t *testing.T) { assert.False(t, b.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) } + +func TestSourceLinkedDeploymentEnabled(t *testing.T) { + b, diags := processSourceLinkedBundle(t, true) + require.NoError(t, diags.Error()) + assert.True(t, *b.Config.Presets.SourceLinkedDeployment) +} + +func TestSourceLinkedDeploymentDisabled(t *testing.T) { + b, diags := processSourceLinkedBundle(t, false) + require.NoError(t, diags.Error()) + assert.False(t, *b.Config.Presets.SourceLinkedDeployment) +} + +func processSourceLinkedBundle(t *testing.T, presetEnabled bool) (*bundle.Bundle, diag.Diagnostics) { + if runtime.GOOS == "windows" { + t.Skip("this test is not applicable on Windows because source-linked mode works only in the Databricks Workspace") + } + + b := mockBundle(config.Development) + + workspacePath := "/Workspace/lennart@company.com/" + b.SyncRootPath = workspacePath + b.Config.Presets.SourceLinkedDeployment = &presetEnabled + + ctx := dbr.MockRuntime(context.Background(), true) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) + diags := bundle.Apply(ctx, b, m) + return b, diags +} diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 321fa5b3..1e2484c7 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/notebook" @@ -103,8 +104,13 @@ func (t *translateContext) rewritePath( return fmt.Errorf("path %s is not contained in sync root path", localPath) } - // Prefix remote path with its remote root path. - remotePath := path.Join(t.b.Config.Workspace.FilePath, filepath.ToSlash(localRelPath)) + var workspacePath string + if config.IsExplicitlyEnabled(t.b.Config.Presets.SourceLinkedDeployment) { + workspacePath = t.b.SyncRootPath + } else { + workspacePath = t.b.Config.Workspace.FilePath + } + remotePath := path.Join(workspacePath, filepath.ToSlash(localRelPath)) // Convert local path into workspace path via specified function. interp, err := fn(*p, localPath, localRelPath, remotePath) diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 9d655b27..a2032f81 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" @@ -787,3 +788,163 @@ func TestTranslatePathWithComplexVariables(t *testing.T) { b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, ) } + +func TestTranslatePathsWithSourceLinkedDeployment(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("this test is not applicable on Windows because source-linked mode works only in the Databricks Workspace") + } + + dir := t.TempDir() + touchNotebookFile(t, filepath.Join(dir, "my_job_notebook.py")) + touchNotebookFile(t, filepath.Join(dir, "my_pipeline_notebook.py")) + touchEmptyFile(t, filepath.Join(dir, "my_python_file.py")) + touchEmptyFile(t, filepath.Join(dir, "dist", "task.jar")) + touchEmptyFile(t, filepath.Join(dir, "requirements.txt")) + + enabled := true + b := &bundle.Bundle{ + SyncRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Workspace: config.Workspace{ + FilePath: "/bundle", + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + NotebookTask: &jobs.NotebookTask{ + NotebookPath: "my_job_notebook.py", + }, + Libraries: []compute.Library{ + {Whl: "./dist/task.whl"}, + }, + }, + { + NotebookTask: &jobs.NotebookTask{ + NotebookPath: "/Users/jane.doe@databricks.com/absolute_remote.py", + }, + }, + { + NotebookTask: &jobs.NotebookTask{ + NotebookPath: "my_job_notebook.py", + }, + Libraries: []compute.Library{ + {Requirements: "requirements.txt"}, + }, + }, + { + SparkPythonTask: &jobs.SparkPythonTask{ + PythonFile: "my_python_file.py", + }, + }, + { + SparkJarTask: &jobs.SparkJarTask{ + MainClassName: "HelloWorld", + }, + Libraries: []compute.Library{ + {Jar: "./dist/task.jar"}, + }, + }, + { + SparkJarTask: &jobs.SparkJarTask{ + MainClassName: "HelloWorldRemote", + }, + Libraries: []compute.Library{ + {Jar: "dbfs:/bundle/dist/task_remote.jar"}, + }, + }, + }, + }, + }, + }, + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + PipelineSpec: &pipelines.PipelineSpec{ + Libraries: []pipelines.PipelineLibrary{ + { + Notebook: &pipelines.NotebookLibrary{ + Path: "my_pipeline_notebook.py", + }, + }, + { + Notebook: &pipelines.NotebookLibrary{ + Path: "/Users/jane.doe@databricks.com/absolute_remote.py", + }, + }, + { + File: &pipelines.FileLibrary{ + Path: "my_python_file.py", + }, + }, + }, + }, + }, + }, + }, + Presets: config.Presets{ + SourceLinkedDeployment: &enabled, + }, + }, + } + + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) + diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) + require.NoError(t, diags.Error()) + + // updated to source path + assert.Equal( + t, + filepath.Join(dir, "my_job_notebook"), + b.Config.Resources.Jobs["job"].Tasks[0].NotebookTask.NotebookPath, + ) + assert.Equal( + t, + filepath.Join(dir, "requirements.txt"), + b.Config.Resources.Jobs["job"].Tasks[2].Libraries[0].Requirements, + ) + assert.Equal( + t, + filepath.Join(dir, "my_python_file.py"), + b.Config.Resources.Jobs["job"].Tasks[3].SparkPythonTask.PythonFile, + ) + assert.Equal( + t, + filepath.Join(dir, "my_pipeline_notebook"), + b.Config.Resources.Pipelines["pipeline"].Libraries[0].Notebook.Path, + ) + assert.Equal( + t, + filepath.Join(dir, "my_python_file.py"), + b.Config.Resources.Pipelines["pipeline"].Libraries[2].File.Path, + ) + + // left as is + assert.Equal( + t, + filepath.Join("dist", "task.whl"), + b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, + ) + assert.Equal( + t, + "/Users/jane.doe@databricks.com/absolute_remote.py", + b.Config.Resources.Jobs["job"].Tasks[1].NotebookTask.NotebookPath, + ) + assert.Equal( + t, + filepath.Join("dist", "task.jar"), + b.Config.Resources.Jobs["job"].Tasks[4].Libraries[0].Jar, + ) + assert.Equal( + t, + "dbfs:/bundle/dist/task_remote.jar", + b.Config.Resources.Jobs["job"].Tasks[5].Libraries[0].Jar, + ) + assert.Equal( + t, + "/Users/jane.doe@databricks.com/absolute_remote.py", + b.Config.Resources.Pipelines["pipeline"].Libraries[1].Notebook.Path, + ) +} diff --git a/bundle/config/presets.go b/bundle/config/presets.go index 61009a25..30f56c0f 100644 --- a/bundle/config/presets.go +++ b/bundle/config/presets.go @@ -17,6 +17,11 @@ type Presets struct { // JobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs. JobsMaxConcurrentRuns int `json:"jobs_max_concurrent_runs,omitempty"` + // SourceLinkedDeployment indicates whether source-linked deployment is enabled. Works only in Databricks Workspace + // When set to true, resources created during deployment will point to source files in the workspace instead of their workspace copies. + // File synchronization to ${workspace.file_path} is skipped. + SourceLinkedDeployment *bool `json:"source_linked_deployment,omitempty"` + // Tags to add to all resources. Tags map[string]string `json:"tags,omitempty"` } diff --git a/bundle/deploy/files/upload.go b/bundle/deploy/files/upload.go index bab4e176..452850dc 100644 --- a/bundle/deploy/files/upload.go +++ b/bundle/deploy/files/upload.go @@ -7,6 +7,7 @@ import ( "io/fs" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/permissions" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" @@ -23,6 +24,11 @@ func (m *upload) Name() string { } func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + if config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) { + cmdio.LogString(ctx, "Source-linked deployment is enabled. Deployed resources reference the source files in your working tree instead of separate copies.") + return nil + } + cmdio.LogString(ctx, fmt.Sprintf("Uploading bundle files to %s...", b.Config.Workspace.FilePath)) opts, err := GetSyncOptions(ctx, bundle.ReadOnly(b)) if err != nil { diff --git a/bundle/trampoline/python_dbr_warning.go b/bundle/trampoline/python_dbr_warning.go index f62e9eab..cf3e9aeb 100644 --- a/bundle/trampoline/python_dbr_warning.go +++ b/bundle/trampoline/python_dbr_warning.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/log" @@ -22,6 +23,9 @@ func WrapperWarning() bundle.Mutator { func (m *wrapperWarning) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { if isPythonWheelWrapperOn(b) { + if config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) { + return diag.Warningf("Python wheel notebook wrapper is not available when using source-linked deployment mode. You can disable this mode by setting 'presets.source_linked_deployment: false'") + } return nil } From fab3e8f168c4fa8811991f407ff64232249f84d1 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 20 Nov 2024 13:20:39 +0100 Subject: [PATCH 06/24] Added integration test to deploy bundle to /Shared root path (#1914) ## Changes Added integration test to deploy bundle to /Shared root path ## Tests ``` --- PASS: TestAccDeployBasicToSharedWorkspace (24.58s) PASS coverage: 31.2% of statements in ./... ok github.com/databricks/cli/internal/bundle 25.572s coverage: 31.2% of statements in ./... ``` --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> --- .../basic/databricks_template_schema.json | 5 +++ .../basic/template/databricks.yml.tmpl | 4 ++ internal/bundle/deploy_to_shared_test.go | 38 +++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 internal/bundle/deploy_to_shared_test.go diff --git a/internal/bundle/bundles/basic/databricks_template_schema.json b/internal/bundle/bundles/basic/databricks_template_schema.json index c1c5cf12..41a723b0 100644 --- a/internal/bundle/bundles/basic/databricks_template_schema.json +++ b/internal/bundle/bundles/basic/databricks_template_schema.json @@ -11,6 +11,11 @@ "node_type_id": { "type": "string", "description": "Node type id for job cluster" + }, + "root_path": { + "type": "string", + "description": "Root path to deploy bundle to", + "default": "" } } } diff --git a/internal/bundle/bundles/basic/template/databricks.yml.tmpl b/internal/bundle/bundles/basic/template/databricks.yml.tmpl index a88cbd30..0eca4231 100644 --- a/internal/bundle/bundles/basic/template/databricks.yml.tmpl +++ b/internal/bundle/bundles/basic/template/databricks.yml.tmpl @@ -2,7 +2,11 @@ bundle: name: basic workspace: + {{ if .root_path }} + root_path: "{{.root_path}}/.bundle/{{.unique_id}}" + {{ else }} root_path: "~/.bundle/{{.unique_id}}" + {{ end }} resources: jobs: diff --git a/internal/bundle/deploy_to_shared_test.go b/internal/bundle/deploy_to_shared_test.go new file mode 100644 index 00000000..568c1fb5 --- /dev/null +++ b/internal/bundle/deploy_to_shared_test.go @@ -0,0 +1,38 @@ +package bundle + +import ( + "fmt" + "testing" + + "github.com/databricks/cli/internal" + "github.com/databricks/cli/internal/acc" + "github.com/databricks/cli/libs/env" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestAccDeployBasicToSharedWorkspacePath(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + + nodeTypeId := internal.GetNodeTypeId(env.Get(ctx, "CLOUD_ENV")) + uniqueId := uuid.New().String() + + currentUser, err := wt.W.CurrentUser.Me(ctx) + require.NoError(t, err) + + bundleRoot, err := initTestTemplate(t, ctx, "basic", map[string]any{ + "unique_id": uniqueId, + "node_type_id": nodeTypeId, + "spark_version": defaultSparkVersion, + "root_path": fmt.Sprintf("/Shared/%s", currentUser.UserName), + }) + require.NoError(t, err) + + t.Cleanup(func() { + err = destroyBundle(wt.T, ctx, bundleRoot) + require.NoError(wt.T, err) + }) + + err = deployBundle(wt.T, ctx, bundleRoot) + require.NoError(wt.T, err) +} From 592e1111b77ca9d4e2ecdfe63e40a8dbeef544d8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 20 Nov 2024 13:53:25 +0100 Subject: [PATCH 07/24] Update filenames used by bundle generate to use `..yml` (#1901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Changes Update filenames used by bundle generate to use '.resource-type.yml' Similar to [Add sub-extension to resource files in built-in templates by shreyas-goenka · Pull Request #1777 · databricks/cli](https://github.com/databricks/cli/pull/1777) --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> --- cmd/bundle/generate/generate_test.go | 122 +++++++++++++++++++++++++- cmd/bundle/generate/job.go | 14 ++- cmd/bundle/generate/pipeline.go | 14 ++- internal/bundle/bind_resource_test.go | 2 +- 4 files changed, 147 insertions(+), 5 deletions(-) diff --git a/cmd/bundle/generate/generate_test.go b/cmd/bundle/generate/generate_test.go index 943f721c..bc1549e6 100644 --- a/cmd/bundle/generate/generate_test.go +++ b/cmd/bundle/generate/generate_test.go @@ -3,8 +3,10 @@ package generate import ( "bytes" "context" + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "testing" @@ -90,7 +92,7 @@ func TestGeneratePipelineCommand(t *testing.T) { err := cmd.RunE(cmd, []string{}) require.NoError(t, err) - data, err := os.ReadFile(filepath.Join(configDir, "test_pipeline.yml")) + data, err := os.ReadFile(filepath.Join(configDir, "test_pipeline.pipeline.yml")) require.NoError(t, err) require.Equal(t, fmt.Sprintf(`resources: pipelines: @@ -186,7 +188,123 @@ func TestGenerateJobCommand(t *testing.T) { err := cmd.RunE(cmd, []string{}) require.NoError(t, err) - data, err := os.ReadFile(filepath.Join(configDir, "test_job.yml")) + data, err := os.ReadFile(filepath.Join(configDir, "test_job.job.yml")) + require.NoError(t, err) + + require.Equal(t, fmt.Sprintf(`resources: + jobs: + test_job: + name: test-job + job_clusters: + - new_cluster: + custom_tags: + "Tag1": "24X7-1234" + - new_cluster: + spark_conf: + "spark.databricks.delta.preview.enabled": "true" + tasks: + - task_key: notebook_task + notebook_task: + notebook_path: %s + parameters: + - name: empty + default: "" +`, filepath.Join("..", "src", "notebook.py")), string(data)) + + data, err = os.ReadFile(filepath.Join(srcDir, "notebook.py")) + require.NoError(t, err) + require.Equal(t, "# Databricks notebook source\nNotebook content", string(data)) +} + +func touchEmptyFile(t *testing.T, path string) { + err := os.MkdirAll(filepath.Dir(path), 0700) + require.NoError(t, err) + f, err := os.Create(path) + require.NoError(t, err) + f.Close() +} + +func TestGenerateJobCommandOldFileRename(t *testing.T) { + cmd := NewGenerateJobCommand() + + root := t.TempDir() + b := &bundle.Bundle{ + BundleRootPath: root, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + + jobsApi := m.GetMockJobsAPI() + jobsApi.EXPECT().Get(mock.Anything, jobs.GetJobRequest{JobId: 1234}).Return(&jobs.Job{ + Settings: &jobs.JobSettings{ + Name: "test-job", + JobClusters: []jobs.JobCluster{ + {NewCluster: compute.ClusterSpec{ + CustomTags: map[string]string{ + "Tag1": "24X7-1234", + }, + }}, + {NewCluster: compute.ClusterSpec{ + SparkConf: map[string]string{ + "spark.databricks.delta.preview.enabled": "true", + }, + }}, + }, + Tasks: []jobs.Task{ + { + TaskKey: "notebook_task", + NotebookTask: &jobs.NotebookTask{ + NotebookPath: "/test/notebook", + }, + }, + }, + Parameters: []jobs.JobParameterDefinition{ + { + Name: "empty", + Default: "", + }, + }, + }, + }, nil) + + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/test/notebook").Return(&workspace.ObjectInfo{ + ObjectType: workspace.ObjectTypeNotebook, + Language: workspace.LanguagePython, + Path: "/test/notebook", + }, nil) + + notebookContent := io.NopCloser(bytes.NewBufferString("# Databricks notebook source\nNotebook content")) + workspaceApi.EXPECT().Download(mock.Anything, "/test/notebook", mock.Anything).Return(notebookContent, nil) + + cmd.SetContext(bundle.Context(context.Background(), b)) + cmd.Flag("existing-job-id").Value.Set("1234") + + configDir := filepath.Join(root, "resources") + cmd.Flag("config-dir").Value.Set(configDir) + + srcDir := filepath.Join(root, "src") + cmd.Flag("source-dir").Value.Set(srcDir) + + var key string + cmd.Flags().StringVar(&key, "key", "test_job", "") + + // Create an old generated file first + oldFilename := filepath.Join(configDir, "test_job.yml") + touchEmptyFile(t, oldFilename) + + // Having an existing files require --force flag to regenerate them + cmd.Flag("force").Value.Set("true") + + err := cmd.RunE(cmd, []string{}) + require.NoError(t, err) + + // Make sure file do not exists after the run + _, err = os.Stat(oldFilename) + require.True(t, errors.Is(err, fs.ErrNotExist)) + + data, err := os.ReadFile(filepath.Join(configDir, "test_job.job.yml")) require.NoError(t, err) require.Equal(t, fmt.Sprintf(`resources: diff --git a/cmd/bundle/generate/job.go b/cmd/bundle/generate/job.go index 99bc6166..9ac41e3c 100644 --- a/cmd/bundle/generate/job.go +++ b/cmd/bundle/generate/job.go @@ -1,7 +1,9 @@ package generate import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" @@ -83,7 +85,17 @@ func NewGenerateJobCommand() *cobra.Command { return err } - filename := filepath.Join(configDir, fmt.Sprintf("%s.yml", jobKey)) + oldFilename := filepath.Join(configDir, fmt.Sprintf("%s.yml", jobKey)) + filename := filepath.Join(configDir, fmt.Sprintf("%s.job.yml", jobKey)) + + // User might continuously run generate command to update their bundle jobs with any changes made in Databricks UI. + // Due to changing in the generated file names, we need to first rename existing resource file to the new name. + // Otherwise users can end up with duplicated resources. + err = os.Rename(oldFilename, filename) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to rename file %s. DABs uses the resource type as a sub-extension for generated content, please rename it to %s, err: %w", oldFilename, filename, err) + } + saver := yamlsaver.NewSaverWithStyle(map[string]yaml.Style{ // Including all JobSettings and nested fields which are map[string]string type "spark_conf": yaml.DoubleQuotedStyle, diff --git a/cmd/bundle/generate/pipeline.go b/cmd/bundle/generate/pipeline.go index bd973fe0..910baa45 100644 --- a/cmd/bundle/generate/pipeline.go +++ b/cmd/bundle/generate/pipeline.go @@ -1,7 +1,9 @@ package generate import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" @@ -83,7 +85,17 @@ func NewGeneratePipelineCommand() *cobra.Command { return err } - filename := filepath.Join(configDir, fmt.Sprintf("%s.yml", pipelineKey)) + oldFilename := filepath.Join(configDir, fmt.Sprintf("%s.yml", pipelineKey)) + filename := filepath.Join(configDir, fmt.Sprintf("%s.pipeline.yml", pipelineKey)) + + // User might continuously run generate command to update their bundle jobs with any changes made in Databricks UI. + // Due to changing in the generated file names, we need to first rename existing resource file to the new name. + // Otherwise users can end up with duplicated resources. + err = os.Rename(oldFilename, filename) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to rename file %s. DABs uses the resource type as a sub-extension for generated content, please rename it to %s, err: %w", oldFilename, filename, err) + } + saver := yamlsaver.NewSaverWithStyle( // Including all PipelineSpec and nested fields which are map[string]string type map[string]yaml.Style{ diff --git a/internal/bundle/bind_resource_test.go b/internal/bundle/bind_resource_test.go index 2449c31f..8cc5da53 100644 --- a/internal/bundle/bind_resource_test.go +++ b/internal/bundle/bind_resource_test.go @@ -166,7 +166,7 @@ func TestAccGenerateAndBind(t *testing.T) { _, err = os.Stat(filepath.Join(bundleRoot, "src", "test.py")) require.NoError(t, err) - matches, err := filepath.Glob(filepath.Join(bundleRoot, "resources", "test_job_key.yml")) + matches, err := filepath.Glob(filepath.Join(bundleRoot, "resources", "test_job_key.job.yml")) require.NoError(t, err) require.Len(t, matches, 1) From ade95d96493fe36a5acec754ad26b2890c5fc902 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 20 Nov 2024 15:48:18 +0100 Subject: [PATCH 08/24] [Release] Release v0.235.0 (#1918) **Note:** the `bundle generate` command now uses the `..yml` sub-extension for the configuration files it writes. Existing configuration files that do not use this sub-extension are renamed to include it. Bundles: * Make `TableName` field part of quality monitor schema ([#1903](https://github.com/databricks/cli/pull/1903)). * Do not prepend paths starting with ~ or variable reference ([#1905](https://github.com/databricks/cli/pull/1905)). * Fix workspace extensions filer accidentally reading notebooks ([#1891](https://github.com/databricks/cli/pull/1891)). * Fix template initialization when running on Databricks ([#1912](https://github.com/databricks/cli/pull/1912)). * Source-linked deployments for bundles in the workspace ([#1884](https://github.com/databricks/cli/pull/1884)). * Added integration test to deploy bundle to /Shared root path ([#1914](https://github.com/databricks/cli/pull/1914)). * Update filenames used by bundle generate to use `..yml` ([#1901](https://github.com/databricks/cli/pull/1901)). Internal: * Extract functionality to detect if the CLI is running on DBR ([#1889](https://github.com/databricks/cli/pull/1889)). * Consolidate test helpers for `io/fs` ([#1906](https://github.com/databricks/cli/pull/1906)). * Use `fs.FS` interface to read template ([#1910](https://github.com/databricks/cli/pull/1910)). * Use `filer.Filer` to write template instantiation ([#1911](https://github.com/databricks/cli/pull/1911)). --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5b6496b..f2645b21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Version changelog +## [Release] Release v0.235.0 + +**Note:** the `bundle generate` command now uses the `..yml` +sub-extension for the configuration files it writes. Existing configuration +files that do not use this sub-extension are renamed to include it. + +Bundles: + * Make `TableName` field part of quality monitor schema ([#1903](https://github.com/databricks/cli/pull/1903)). + * Do not prepend paths starting with ~ or variable reference ([#1905](https://github.com/databricks/cli/pull/1905)). + * Fix workspace extensions filer accidentally reading notebooks ([#1891](https://github.com/databricks/cli/pull/1891)). + * Fix template initialization when running on Databricks ([#1912](https://github.com/databricks/cli/pull/1912)). + * Source-linked deployments for bundles in the workspace ([#1884](https://github.com/databricks/cli/pull/1884)). + * Added integration test to deploy bundle to /Shared root path ([#1914](https://github.com/databricks/cli/pull/1914)). + * Update filenames used by bundle generate to use `..yml` ([#1901](https://github.com/databricks/cli/pull/1901)). + +Internal: + * Extract functionality to detect if the CLI is running on DBR ([#1889](https://github.com/databricks/cli/pull/1889)). + * Consolidate test helpers for `io/fs` ([#1906](https://github.com/databricks/cli/pull/1906)). + * Use `fs.FS` interface to read template ([#1910](https://github.com/databricks/cli/pull/1910)). + * Use `filer.Filer` to write template instantiation ([#1911](https://github.com/databricks/cli/pull/1911)). + + + ## [Release] Release v0.234.0 Bundles: From 984c38e03ec0b3915ac4d05ff08272ada74f9f6b Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 20 Nov 2024 22:00:10 +0530 Subject: [PATCH 09/24] Add unique ID to `root_path` for bundle integration test fixtures (#1917) ## Changes Integration tests using these fixtures could have been flaky when run in parallel using the same user's identity. They would also possibly have piggybacked state from previous runs. This PR adds a UUID to the root_path to force independent bundle deployments for every test run. I have checked that all bundles in `internal/bundle/bundles` have `root_path` namespaced to a UUID. ## Tests Self testing. --- internal/bundle/bundles/empty_bundle/databricks.yml | 2 -- .../bundles/recreate_pipeline/template/databricks.yml.tmpl | 5 ++++- .../bundle/bundles/uc_schema/template/databricks.yml.tmpl | 5 ++++- 3 files changed, 8 insertions(+), 4 deletions(-) delete mode 100644 internal/bundle/bundles/empty_bundle/databricks.yml diff --git a/internal/bundle/bundles/empty_bundle/databricks.yml b/internal/bundle/bundles/empty_bundle/databricks.yml deleted file mode 100644 index efc62782..00000000 --- a/internal/bundle/bundles/empty_bundle/databricks.yml +++ /dev/null @@ -1,2 +0,0 @@ -bundle: - name: abc diff --git a/internal/bundle/bundles/recreate_pipeline/template/databricks.yml.tmpl b/internal/bundle/bundles/recreate_pipeline/template/databricks.yml.tmpl index 10350f13..4ebeb265 100644 --- a/internal/bundle/bundles/recreate_pipeline/template/databricks.yml.tmpl +++ b/internal/bundle/bundles/recreate_pipeline/template/databricks.yml.tmpl @@ -1,5 +1,8 @@ bundle: - name: "bundle-playground" + name: recreate-pipeline + +workspace: + root_path: "~/.bundle/{{.unique_id}}" variables: catalog: diff --git a/internal/bundle/bundles/uc_schema/template/databricks.yml.tmpl b/internal/bundle/bundles/uc_schema/template/databricks.yml.tmpl index 961af25e..15076ac8 100644 --- a/internal/bundle/bundles/uc_schema/template/databricks.yml.tmpl +++ b/internal/bundle/bundles/uc_schema/template/databricks.yml.tmpl @@ -1,5 +1,8 @@ bundle: - name: "bundle-playground" + name: uc-schema + +workspace: + root_path: "~/.bundle/{{.unique_id}}" resources: pipelines: From 14fe03dcb95c65cd4d6bb3e18d17108888d9b6c0 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 21 Nov 2024 11:28:50 +0100 Subject: [PATCH 10/24] Breakout variable lookup into separate files and tests (#1921) ## Changes While looking into adding variable lookups for notification destinations ([API][API]), I found the codegen approach for different classes of variable lookups a bit complex. The template had a custom field override (for service principals), the package had an override for the cluster lookup, and it didn't produce tests. The notification destinations API uses a default page size of 20 for listing. I want to use a larger page size to limit the number of API calls, so that would imply another customization on the template or a manual override. This code being rather mechanical, I used copilot to produce all instances of the resolvers and their tests (after writing one of them manually). [api]: https://docs.databricks.com/api/workspace/notificationdestinations ## Tests * Unit tests pass * Manual confirmation that lookups of warehouses still work --- .codegen.json | 3 +- .codegen/lookup.go.tmpl | 134 ------- .gitattributes | 1 - bundle/config/variable/lookup.go | 347 +++--------------- bundle/config/variable/lookup_test.go | 60 +++ bundle/config/variable/resolve_alert.go | 24 ++ bundle/config/variable/resolve_alert_test.go | 49 +++ ...lookup_overrides.go => resolve_cluster.go} | 12 +- .../config/variable/resolve_cluster_policy.go | 24 ++ .../variable/resolve_cluster_policy_test.go | 49 +++ .../config/variable/resolve_cluster_test.go | 50 +++ bundle/config/variable/resolve_dashboard.go | 24 ++ .../config/variable/resolve_dashboard_test.go | 49 +++ .../config/variable/resolve_instance_pool.go | 24 ++ .../variable/resolve_instance_pool_test.go | 49 +++ bundle/config/variable/resolve_job.go | 24 ++ bundle/config/variable/resolve_job_test.go | 49 +++ bundle/config/variable/resolve_metastore.go | 24 ++ .../config/variable/resolve_metastore_test.go | 49 +++ bundle/config/variable/resolve_pipeline.go | 24 ++ .../config/variable/resolve_pipeline_test.go | 49 +++ bundle/config/variable/resolve_query.go | 24 ++ bundle/config/variable/resolve_query_test.go | 49 +++ .../variable/resolve_service_principal.go | 24 ++ .../resolve_service_principal_test.go | 49 +++ bundle/config/variable/resolve_warehouse.go | 24 ++ .../config/variable/resolve_warehouse_test.go | 49 +++ 27 files changed, 898 insertions(+), 439 deletions(-) delete mode 100644 .codegen/lookup.go.tmpl create mode 100644 bundle/config/variable/lookup_test.go create mode 100644 bundle/config/variable/resolve_alert.go create mode 100644 bundle/config/variable/resolve_alert_test.go rename bundle/config/variable/{lookup_overrides.go => resolve_cluster.go} (81%) create mode 100644 bundle/config/variable/resolve_cluster_policy.go create mode 100644 bundle/config/variable/resolve_cluster_policy_test.go create mode 100644 bundle/config/variable/resolve_cluster_test.go create mode 100644 bundle/config/variable/resolve_dashboard.go create mode 100644 bundle/config/variable/resolve_dashboard_test.go create mode 100644 bundle/config/variable/resolve_instance_pool.go create mode 100644 bundle/config/variable/resolve_instance_pool_test.go create mode 100644 bundle/config/variable/resolve_job.go create mode 100644 bundle/config/variable/resolve_job_test.go create mode 100644 bundle/config/variable/resolve_metastore.go create mode 100644 bundle/config/variable/resolve_metastore_test.go create mode 100644 bundle/config/variable/resolve_pipeline.go create mode 100644 bundle/config/variable/resolve_pipeline_test.go create mode 100644 bundle/config/variable/resolve_query.go create mode 100644 bundle/config/variable/resolve_query_test.go create mode 100644 bundle/config/variable/resolve_service_principal.go create mode 100644 bundle/config/variable/resolve_service_principal_test.go create mode 100644 bundle/config/variable/resolve_warehouse.go create mode 100644 bundle/config/variable/resolve_warehouse_test.go diff --git a/.codegen.json b/.codegen.json index 4524ab55..73ab8c2a 100644 --- a/.codegen.json +++ b/.codegen.json @@ -5,8 +5,7 @@ }, "batch": { ".codegen/cmds-workspace.go.tmpl": "cmd/workspace/cmd.go", - ".codegen/cmds-account.go.tmpl": "cmd/account/cmd.go", - ".codegen/lookup.go.tmpl": "bundle/config/variable/lookup.go" + ".codegen/cmds-account.go.tmpl": "cmd/account/cmd.go" }, "toolchain": { "required": ["go"], diff --git a/.codegen/lookup.go.tmpl b/.codegen/lookup.go.tmpl deleted file mode 100644 index 124b629d..00000000 --- a/.codegen/lookup.go.tmpl +++ /dev/null @@ -1,134 +0,0 @@ -// Code generated from OpenAPI specs by Databricks SDK Generator. DO NOT EDIT. - -package variable - -{{ $allowlist := - list - "alerts" - "clusters" - "cluster-policies" - "clusters" - "dashboards" - "instance-pools" - "jobs" - "metastores" - "pipelines" - "service-principals" - "queries" - "warehouses" -}} - -{{ $customField := - dict - "service-principals" "ApplicationId" -}} - -import ( - "context" - "fmt" - - "github.com/databricks/databricks-sdk-go" -) - -type Lookup struct { - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - {{.Singular.PascalName}} string `json:"{{.Singular.SnakeName}},omitempty"` - - {{end}} - {{- end}} -} - -func LookupFromMap(m map[string]any) *Lookup { - l := &Lookup{} - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - if v, ok := m["{{.Singular.SnakeName}}"]; ok { - l.{{.Singular.PascalName}} = v.(string) - } - {{end -}} - {{- end}} - return l -} - -func (l *Lookup) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { - if err := l.validate(); err != nil { - return "", err - } - - r := allResolvers() - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - if l.{{.Singular.PascalName}} != "" { - return r.{{.Singular.PascalName}}(ctx, w, l.{{.Singular.PascalName}}) - } - {{end -}} - {{- end}} - - return "", fmt.Errorf("no valid lookup fields provided") -} - -func (l *Lookup) String() string { - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - if l.{{.Singular.PascalName}} != "" { - return fmt.Sprintf("{{.Singular.KebabName}}: %s", l.{{.Singular.PascalName}}) - } - {{end -}} - {{- end}} - return "" -} - -func (l *Lookup) validate() error { - // Validate that only one field is set - count := 0 - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - if l.{{.Singular.PascalName}} != "" { - count++ - } - {{end -}} - {{- end}} - - if count != 1 { - return fmt.Errorf("exactly one lookup field must be provided") - } - - if strings.Contains(l.String(), "${var") { - return fmt.Errorf("lookup fields cannot contain variable references") - } - - return nil -} - - -type resolverFunc func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) -type resolvers struct { - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - {{.Singular.PascalName}} resolverFunc - {{end -}} - {{- end}} -} - -func allResolvers() *resolvers { - r := &resolvers{} - {{range .Services -}} - {{- if in $allowlist .KebabName -}} - r.{{.Singular.PascalName}} = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["{{.Singular.PascalName}}"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.{{.PascalName}}.GetBy{{range .NamedIdMap.NamePath}}{{.PascalName}}{{end}}(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.{{ getOrDefault $customField .KebabName ((index .NamedIdMap.IdPath 0).PascalName) }}), nil - } - {{end -}} - {{- end}} - - return r -} diff --git a/.gitattributes b/.gitattributes index ecb5669e..2755c02d 100755 --- a/.gitattributes +++ b/.gitattributes @@ -1,4 +1,3 @@ -bundle/config/variable/lookup.go linguist-generated=true cmd/account/access-control/access-control.go linguist-generated=true cmd/account/billable-usage/billable-usage.go linguist-generated=true cmd/account/budgets/budgets.go linguist-generated=true diff --git a/bundle/config/variable/lookup.go b/bundle/config/variable/lookup.go index e40b0ef7..f8cb6719 100755 --- a/bundle/config/variable/lookup.go +++ b/bundle/config/variable/lookup.go @@ -1,11 +1,8 @@ -// Code generated from OpenAPI specs by Databricks SDK Generator. DO NOT EDIT. - package variable import ( "context" "fmt" - "strings" "github.com/databricks/databricks-sdk-go" ) @@ -34,323 +31,75 @@ type Lookup struct { Warehouse string `json:"warehouse,omitempty"` } -func LookupFromMap(m map[string]any) *Lookup { - l := &Lookup{} - if v, ok := m["alert"]; ok { - l.Alert = v.(string) +type resolver interface { + // Resolve resolves the underlying entity's ID. + Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) + + // String returns a human-readable representation of the resolver. + String() string +} + +func (l *Lookup) constructResolver() (resolver, error) { + var resolvers []resolver + + if l.Alert != "" { + resolvers = append(resolvers, resolveAlert{name: l.Alert}) } - if v, ok := m["cluster_policy"]; ok { - l.ClusterPolicy = v.(string) + if l.ClusterPolicy != "" { + resolvers = append(resolvers, resolveClusterPolicy{name: l.ClusterPolicy}) } - if v, ok := m["cluster"]; ok { - l.Cluster = v.(string) + if l.Cluster != "" { + resolvers = append(resolvers, resolveCluster{name: l.Cluster}) } - if v, ok := m["dashboard"]; ok { - l.Dashboard = v.(string) + if l.Dashboard != "" { + resolvers = append(resolvers, resolveDashboard{name: l.Dashboard}) } - if v, ok := m["instance_pool"]; ok { - l.InstancePool = v.(string) + if l.InstancePool != "" { + resolvers = append(resolvers, resolveInstancePool{name: l.InstancePool}) } - if v, ok := m["job"]; ok { - l.Job = v.(string) + if l.Job != "" { + resolvers = append(resolvers, resolveJob{name: l.Job}) } - if v, ok := m["metastore"]; ok { - l.Metastore = v.(string) + if l.Metastore != "" { + resolvers = append(resolvers, resolveMetastore{name: l.Metastore}) } - if v, ok := m["pipeline"]; ok { - l.Pipeline = v.(string) + if l.Pipeline != "" { + resolvers = append(resolvers, resolvePipeline{name: l.Pipeline}) } - if v, ok := m["query"]; ok { - l.Query = v.(string) + if l.Query != "" { + resolvers = append(resolvers, resolveQuery{name: l.Query}) } - if v, ok := m["service_principal"]; ok { - l.ServicePrincipal = v.(string) + if l.ServicePrincipal != "" { + resolvers = append(resolvers, resolveServicePrincipal{name: l.ServicePrincipal}) } - if v, ok := m["warehouse"]; ok { - l.Warehouse = v.(string) + if l.Warehouse != "" { + resolvers = append(resolvers, resolveWarehouse{name: l.Warehouse}) } - return l + switch len(resolvers) { + case 0: + return nil, fmt.Errorf("no valid lookup fields provided") + case 1: + return resolvers[0], nil + default: + return nil, fmt.Errorf("exactly one lookup field must be provided") + } } func (l *Lookup) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { - if err := l.validate(); err != nil { + r, err := l.constructResolver() + if err != nil { return "", err } - r := allResolvers() - if l.Alert != "" { - return r.Alert(ctx, w, l.Alert) - } - if l.ClusterPolicy != "" { - return r.ClusterPolicy(ctx, w, l.ClusterPolicy) - } - if l.Cluster != "" { - return r.Cluster(ctx, w, l.Cluster) - } - if l.Dashboard != "" { - return r.Dashboard(ctx, w, l.Dashboard) - } - if l.InstancePool != "" { - return r.InstancePool(ctx, w, l.InstancePool) - } - if l.Job != "" { - return r.Job(ctx, w, l.Job) - } - if l.Metastore != "" { - return r.Metastore(ctx, w, l.Metastore) - } - if l.Pipeline != "" { - return r.Pipeline(ctx, w, l.Pipeline) - } - if l.Query != "" { - return r.Query(ctx, w, l.Query) - } - if l.ServicePrincipal != "" { - return r.ServicePrincipal(ctx, w, l.ServicePrincipal) - } - if l.Warehouse != "" { - return r.Warehouse(ctx, w, l.Warehouse) - } - - return "", fmt.Errorf("no valid lookup fields provided") + return r.Resolve(ctx, w) } func (l *Lookup) String() string { - if l.Alert != "" { - return fmt.Sprintf("alert: %s", l.Alert) - } - if l.ClusterPolicy != "" { - return fmt.Sprintf("cluster-policy: %s", l.ClusterPolicy) - } - if l.Cluster != "" { - return fmt.Sprintf("cluster: %s", l.Cluster) - } - if l.Dashboard != "" { - return fmt.Sprintf("dashboard: %s", l.Dashboard) - } - if l.InstancePool != "" { - return fmt.Sprintf("instance-pool: %s", l.InstancePool) - } - if l.Job != "" { - return fmt.Sprintf("job: %s", l.Job) - } - if l.Metastore != "" { - return fmt.Sprintf("metastore: %s", l.Metastore) - } - if l.Pipeline != "" { - return fmt.Sprintf("pipeline: %s", l.Pipeline) - } - if l.Query != "" { - return fmt.Sprintf("query: %s", l.Query) - } - if l.ServicePrincipal != "" { - return fmt.Sprintf("service-principal: %s", l.ServicePrincipal) - } - if l.Warehouse != "" { - return fmt.Sprintf("warehouse: %s", l.Warehouse) + r, _ := l.constructResolver() + if r == nil { + return "" } - return "" -} - -func (l *Lookup) validate() error { - // Validate that only one field is set - count := 0 - if l.Alert != "" { - count++ - } - if l.ClusterPolicy != "" { - count++ - } - if l.Cluster != "" { - count++ - } - if l.Dashboard != "" { - count++ - } - if l.InstancePool != "" { - count++ - } - if l.Job != "" { - count++ - } - if l.Metastore != "" { - count++ - } - if l.Pipeline != "" { - count++ - } - if l.Query != "" { - count++ - } - if l.ServicePrincipal != "" { - count++ - } - if l.Warehouse != "" { - count++ - } - - if count != 1 { - return fmt.Errorf("exactly one lookup field must be provided") - } - - if strings.Contains(l.String(), "${var") { - return fmt.Errorf("lookup fields cannot contain variable references") - } - - return nil -} - -type resolverFunc func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) -type resolvers struct { - Alert resolverFunc - ClusterPolicy resolverFunc - Cluster resolverFunc - Dashboard resolverFunc - InstancePool resolverFunc - Job resolverFunc - Metastore resolverFunc - Pipeline resolverFunc - Query resolverFunc - ServicePrincipal resolverFunc - Warehouse resolverFunc -} - -func allResolvers() *resolvers { - r := &resolvers{} - r.Alert = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Alert"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Alerts.GetByDisplayName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.Id), nil - } - r.ClusterPolicy = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["ClusterPolicy"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.ClusterPolicies.GetByName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.PolicyId), nil - } - r.Cluster = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Cluster"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Clusters.GetByClusterName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.ClusterId), nil - } - r.Dashboard = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Dashboard"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Dashboards.GetByName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.Id), nil - } - r.InstancePool = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["InstancePool"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.InstancePools.GetByInstancePoolName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.InstancePoolId), nil - } - r.Job = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Job"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Jobs.GetBySettingsName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.JobId), nil - } - r.Metastore = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Metastore"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Metastores.GetByName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.MetastoreId), nil - } - r.Pipeline = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Pipeline"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Pipelines.GetByName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.PipelineId), nil - } - r.Query = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Query"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Queries.GetByDisplayName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.Id), nil - } - r.ServicePrincipal = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["ServicePrincipal"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.ServicePrincipals.GetByDisplayName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.ApplicationId), nil - } - r.Warehouse = func(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { - fn, ok := lookupOverrides["Warehouse"] - if ok { - return fn(ctx, w, name) - } - entity, err := w.Warehouses.GetByName(ctx, name) - if err != nil { - return "", err - } - - return fmt.Sprint(entity.Id), nil - } - - return r + return r.String() } diff --git a/bundle/config/variable/lookup_test.go b/bundle/config/variable/lookup_test.go new file mode 100644 index 00000000..a8474875 --- /dev/null +++ b/bundle/config/variable/lookup_test.go @@ -0,0 +1,60 @@ +package variable + +import ( + "context" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLookup_Coverage(t *testing.T) { + var lookup Lookup + val := reflect.ValueOf(lookup) + typ := val.Type() + + for i := 0; i < val.NumField(); i++ { + field := val.Field(i) + if field.Kind() != reflect.String { + t.Fatalf("Field %s is not a string", typ.Field(i).Name) + } + + fieldType := typ.Field(i) + t.Run(fieldType.Name, func(t *testing.T) { + // Use a fresh instance of the struct in each test + var lookup Lookup + + // Set the field to a non-empty string + reflect.ValueOf(&lookup).Elem().Field(i).SetString("value") + + // Test the [String] function + assert.NotEmpty(t, lookup.String()) + }) + } +} + +func TestLookup_Empty(t *testing.T) { + var lookup Lookup + + // Resolve returns an error when no fields are provided + _, err := lookup.Resolve(context.Background(), nil) + assert.ErrorContains(t, err, "no valid lookup fields provided") + + // No string representation for an invalid lookup + assert.Empty(t, lookup.String()) + +} + +func TestLookup_Multiple(t *testing.T) { + lookup := Lookup{ + Alert: "alert", + Query: "query", + } + + // Resolve returns an error when multiple fields are provided + _, err := lookup.Resolve(context.Background(), nil) + assert.ErrorContains(t, err, "exactly one lookup field must be provided") + + // No string representation for an invalid lookup + assert.Empty(t, lookup.String()) +} diff --git a/bundle/config/variable/resolve_alert.go b/bundle/config/variable/resolve_alert.go new file mode 100644 index 00000000..be83e81f --- /dev/null +++ b/bundle/config/variable/resolve_alert.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveAlert struct { + name string +} + +func (l resolveAlert) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Alerts.GetByDisplayName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.Id), nil +} + +func (l resolveAlert) String() string { + return fmt.Sprintf("alert: %s", l.name) +} diff --git a/bundle/config/variable/resolve_alert_test.go b/bundle/config/variable/resolve_alert_test.go new file mode 100644 index 00000000..32f8d641 --- /dev/null +++ b/bundle/config/variable/resolve_alert_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveAlert_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockAlertsAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "alert"). + Return(&sql.ListAlertsResponseAlert{ + Id: "1234", + }, nil) + + ctx := context.Background() + l := resolveAlert{name: "alert"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "1234", result) +} + +func TestResolveAlert_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockAlertsAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "alert"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveAlert{name: "alert"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveAlert_String(t *testing.T) { + l := resolveAlert{name: "name"} + assert.Equal(t, "alert: name", l.String()) +} diff --git a/bundle/config/variable/lookup_overrides.go b/bundle/config/variable/resolve_cluster.go similarity index 81% rename from bundle/config/variable/lookup_overrides.go rename to bundle/config/variable/resolve_cluster.go index 1be373dc..2d68b7fb 100644 --- a/bundle/config/variable/lookup_overrides.go +++ b/bundle/config/variable/resolve_cluster.go @@ -8,13 +8,13 @@ import ( "github.com/databricks/databricks-sdk-go/service/compute" ) -var lookupOverrides = map[string]resolverFunc{ - "Cluster": resolveCluster, +type resolveCluster struct { + name string } // We added a custom resolver for the cluster to add filtering for the cluster source when we list all clusters. // Without the filtering listing could take a very long time (5-10 mins) which leads to lookup timeouts. -func resolveCluster(ctx context.Context, w *databricks.WorkspaceClient, name string) (string, error) { +func (l resolveCluster) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { result, err := w.Clusters.ListAll(ctx, compute.ListClustersRequest{ FilterBy: &compute.ListClustersFilterBy{ ClusterSources: []compute.ClusterSource{compute.ClusterSourceApi, compute.ClusterSourceUi}, @@ -30,6 +30,8 @@ func resolveCluster(ctx context.Context, w *databricks.WorkspaceClient, name str key := v.ClusterName tmp[key] = append(tmp[key], v) } + + name := l.name alternatives, ok := tmp[name] if !ok || len(alternatives) == 0 { return "", fmt.Errorf("cluster named '%s' does not exist", name) @@ -39,3 +41,7 @@ func resolveCluster(ctx context.Context, w *databricks.WorkspaceClient, name str } return alternatives[0].ClusterId, nil } + +func (l resolveCluster) String() string { + return fmt.Sprintf("cluster: %s", l.name) +} diff --git a/bundle/config/variable/resolve_cluster_policy.go b/bundle/config/variable/resolve_cluster_policy.go new file mode 100644 index 00000000..b19380a6 --- /dev/null +++ b/bundle/config/variable/resolve_cluster_policy.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveClusterPolicy struct { + name string +} + +func (l resolveClusterPolicy) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.ClusterPolicies.GetByName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.PolicyId), nil +} + +func (l resolveClusterPolicy) String() string { + return fmt.Sprintf("cluster-policy: %s", l.name) +} diff --git a/bundle/config/variable/resolve_cluster_policy_test.go b/bundle/config/variable/resolve_cluster_policy_test.go new file mode 100644 index 00000000..fb17fad1 --- /dev/null +++ b/bundle/config/variable/resolve_cluster_policy_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveClusterPolicy_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockClusterPoliciesAPI() + api.EXPECT(). + GetByName(mock.Anything, "policy"). + Return(&compute.Policy{ + PolicyId: "1234", + }, nil) + + ctx := context.Background() + l := resolveClusterPolicy{name: "policy"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "1234", result) +} + +func TestResolveClusterPolicy_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockClusterPoliciesAPI() + api.EXPECT(). + GetByName(mock.Anything, "policy"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveClusterPolicy{name: "policy"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveClusterPolicy_String(t *testing.T) { + l := resolveClusterPolicy{name: "name"} + assert.Equal(t, "cluster-policy: name", l.String()) +} diff --git a/bundle/config/variable/resolve_cluster_test.go b/bundle/config/variable/resolve_cluster_test.go new file mode 100644 index 00000000..2f3aa27c --- /dev/null +++ b/bundle/config/variable/resolve_cluster_test.go @@ -0,0 +1,50 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveCluster_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockClustersAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return([]compute.ClusterDetails{ + {ClusterId: "1234", ClusterName: "cluster1"}, + {ClusterId: "2345", ClusterName: "cluster2"}, + }, nil) + + ctx := context.Background() + l := resolveCluster{name: "cluster2"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "2345", result) +} + +func TestResolveCluster_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockClustersAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return([]compute.ClusterDetails{}, nil) + + ctx := context.Background() + l := resolveCluster{name: "cluster"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.Error(t, err) + assert.Contains(t, err.Error(), "cluster named 'cluster' does not exist") +} + +func TestResolveCluster_String(t *testing.T) { + l := resolveCluster{name: "name"} + assert.Equal(t, "cluster: name", l.String()) +} diff --git a/bundle/config/variable/resolve_dashboard.go b/bundle/config/variable/resolve_dashboard.go new file mode 100644 index 00000000..44fd4519 --- /dev/null +++ b/bundle/config/variable/resolve_dashboard.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveDashboard struct { + name string +} + +func (l resolveDashboard) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Dashboards.GetByName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.Id), nil +} + +func (l resolveDashboard) String() string { + return fmt.Sprintf("dashboard: %s", l.name) +} diff --git a/bundle/config/variable/resolve_dashboard_test.go b/bundle/config/variable/resolve_dashboard_test.go new file mode 100644 index 00000000..3afed479 --- /dev/null +++ b/bundle/config/variable/resolve_dashboard_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveDashboard_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockDashboardsAPI() + api.EXPECT(). + GetByName(mock.Anything, "dashboard"). + Return(&sql.Dashboard{ + Id: "1234", + }, nil) + + ctx := context.Background() + l := resolveDashboard{name: "dashboard"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "1234", result) +} + +func TestResolveDashboard_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockDashboardsAPI() + api.EXPECT(). + GetByName(mock.Anything, "dashboard"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveDashboard{name: "dashboard"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveDashboard_String(t *testing.T) { + l := resolveDashboard{name: "name"} + assert.Equal(t, "dashboard: name", l.String()) +} diff --git a/bundle/config/variable/resolve_instance_pool.go b/bundle/config/variable/resolve_instance_pool.go new file mode 100644 index 00000000..cbf0775c --- /dev/null +++ b/bundle/config/variable/resolve_instance_pool.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveInstancePool struct { + name string +} + +func (l resolveInstancePool) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.InstancePools.GetByInstancePoolName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.InstancePoolId), nil +} + +func (l resolveInstancePool) String() string { + return fmt.Sprintf("instance-pool: %s", l.name) +} diff --git a/bundle/config/variable/resolve_instance_pool_test.go b/bundle/config/variable/resolve_instance_pool_test.go new file mode 100644 index 00000000..cfd1ba01 --- /dev/null +++ b/bundle/config/variable/resolve_instance_pool_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveInstancePool_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockInstancePoolsAPI() + api.EXPECT(). + GetByInstancePoolName(mock.Anything, "instance_pool"). + Return(&compute.InstancePoolAndStats{ + InstancePoolId: "5678", + }, nil) + + ctx := context.Background() + l := resolveInstancePool{name: "instance_pool"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "5678", result) +} + +func TestResolveInstancePool_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockInstancePoolsAPI() + api.EXPECT(). + GetByInstancePoolName(mock.Anything, "instance_pool"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveInstancePool{name: "instance_pool"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveInstancePool_String(t *testing.T) { + l := resolveInstancePool{name: "name"} + assert.Equal(t, "instance-pool: name", l.String()) +} diff --git a/bundle/config/variable/resolve_job.go b/bundle/config/variable/resolve_job.go new file mode 100644 index 00000000..3def6488 --- /dev/null +++ b/bundle/config/variable/resolve_job.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveJob struct { + name string +} + +func (l resolveJob) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Jobs.GetBySettingsName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.JobId), nil +} + +func (l resolveJob) String() string { + return fmt.Sprintf("job: %s", l.name) +} diff --git a/bundle/config/variable/resolve_job_test.go b/bundle/config/variable/resolve_job_test.go new file mode 100644 index 00000000..523d0795 --- /dev/null +++ b/bundle/config/variable/resolve_job_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveJob_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockJobsAPI() + api.EXPECT(). + GetBySettingsName(mock.Anything, "job"). + Return(&jobs.BaseJob{ + JobId: 5678, + }, nil) + + ctx := context.Background() + l := resolveJob{name: "job"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "5678", result) +} + +func TestResolveJob_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockJobsAPI() + api.EXPECT(). + GetBySettingsName(mock.Anything, "job"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveJob{name: "job"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveJob_String(t *testing.T) { + l := resolveJob{name: "name"} + assert.Equal(t, "job: name", l.String()) +} diff --git a/bundle/config/variable/resolve_metastore.go b/bundle/config/variable/resolve_metastore.go new file mode 100644 index 00000000..958e4378 --- /dev/null +++ b/bundle/config/variable/resolve_metastore.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveMetastore struct { + name string +} + +func (l resolveMetastore) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Metastores.GetByName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.MetastoreId), nil +} + +func (l resolveMetastore) String() string { + return fmt.Sprintf("metastore: %s", l.name) +} diff --git a/bundle/config/variable/resolve_metastore_test.go b/bundle/config/variable/resolve_metastore_test.go new file mode 100644 index 00000000..55c4d92d --- /dev/null +++ b/bundle/config/variable/resolve_metastore_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveMetastore_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockMetastoresAPI() + api.EXPECT(). + GetByName(mock.Anything, "metastore"). + Return(&catalog.MetastoreInfo{ + MetastoreId: "abcd", + }, nil) + + ctx := context.Background() + l := resolveMetastore{name: "metastore"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "abcd", result) +} + +func TestResolveMetastore_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockMetastoresAPI() + api.EXPECT(). + GetByName(mock.Anything, "metastore"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveMetastore{name: "metastore"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveMetastore_String(t *testing.T) { + l := resolveMetastore{name: "name"} + assert.Equal(t, "metastore: name", l.String()) +} diff --git a/bundle/config/variable/resolve_pipeline.go b/bundle/config/variable/resolve_pipeline.go new file mode 100644 index 00000000..cabc620d --- /dev/null +++ b/bundle/config/variable/resolve_pipeline.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolvePipeline struct { + name string +} + +func (l resolvePipeline) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Pipelines.GetByName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.PipelineId), nil +} + +func (l resolvePipeline) String() string { + return fmt.Sprintf("pipeline: %s", l.name) +} diff --git a/bundle/config/variable/resolve_pipeline_test.go b/bundle/config/variable/resolve_pipeline_test.go new file mode 100644 index 00000000..620d7624 --- /dev/null +++ b/bundle/config/variable/resolve_pipeline_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/pipelines" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolvePipeline_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockPipelinesAPI() + api.EXPECT(). + GetByName(mock.Anything, "pipeline"). + Return(&pipelines.PipelineStateInfo{ + PipelineId: "abcd", + }, nil) + + ctx := context.Background() + l := resolvePipeline{name: "pipeline"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "abcd", result) +} + +func TestResolvePipeline_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockPipelinesAPI() + api.EXPECT(). + GetByName(mock.Anything, "pipeline"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolvePipeline{name: "pipeline"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolvePipeline_String(t *testing.T) { + l := resolvePipeline{name: "name"} + assert.Equal(t, "pipeline: name", l.String()) +} diff --git a/bundle/config/variable/resolve_query.go b/bundle/config/variable/resolve_query.go new file mode 100644 index 00000000..602ff8de --- /dev/null +++ b/bundle/config/variable/resolve_query.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveQuery struct { + name string +} + +func (l resolveQuery) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Queries.GetByDisplayName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.Id), nil +} + +func (l resolveQuery) String() string { + return fmt.Sprintf("query: %s", l.name) +} diff --git a/bundle/config/variable/resolve_query_test.go b/bundle/config/variable/resolve_query_test.go new file mode 100644 index 00000000..21516e45 --- /dev/null +++ b/bundle/config/variable/resolve_query_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveQuery_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockQueriesAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "query"). + Return(&sql.ListQueryObjectsResponseQuery{ + Id: "1234", + }, nil) + + ctx := context.Background() + l := resolveQuery{name: "query"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "1234", result) +} + +func TestResolveQuery_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockQueriesAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "query"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveQuery{name: "query"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveQuery_String(t *testing.T) { + l := resolveQuery{name: "name"} + assert.Equal(t, "query: name", l.String()) +} diff --git a/bundle/config/variable/resolve_service_principal.go b/bundle/config/variable/resolve_service_principal.go new file mode 100644 index 00000000..3bea4314 --- /dev/null +++ b/bundle/config/variable/resolve_service_principal.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveServicePrincipal struct { + name string +} + +func (l resolveServicePrincipal) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.ServicePrincipals.GetByDisplayName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.ApplicationId), nil +} + +func (l resolveServicePrincipal) String() string { + return fmt.Sprintf("service-principal: %s", l.name) +} diff --git a/bundle/config/variable/resolve_service_principal_test.go b/bundle/config/variable/resolve_service_principal_test.go new file mode 100644 index 00000000..c80f9e4a --- /dev/null +++ b/bundle/config/variable/resolve_service_principal_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveServicePrincipal_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockServicePrincipalsAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "service-principal"). + Return(&iam.ServicePrincipal{ + ApplicationId: "5678", + }, nil) + + ctx := context.Background() + l := resolveServicePrincipal{name: "service-principal"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "5678", result) +} + +func TestResolveServicePrincipal_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockServicePrincipalsAPI() + api.EXPECT(). + GetByDisplayName(mock.Anything, "service-principal"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveServicePrincipal{name: "service-principal"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveServicePrincipal_String(t *testing.T) { + l := resolveServicePrincipal{name: "name"} + assert.Equal(t, "service-principal: name", l.String()) +} diff --git a/bundle/config/variable/resolve_warehouse.go b/bundle/config/variable/resolve_warehouse.go new file mode 100644 index 00000000..fbd3663a --- /dev/null +++ b/bundle/config/variable/resolve_warehouse.go @@ -0,0 +1,24 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" +) + +type resolveWarehouse struct { + name string +} + +func (l resolveWarehouse) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + entity, err := w.Warehouses.GetByName(ctx, l.name) + if err != nil { + return "", err + } + return fmt.Sprint(entity.Id), nil +} + +func (l resolveWarehouse) String() string { + return fmt.Sprintf("warehouse: %s", l.name) +} diff --git a/bundle/config/variable/resolve_warehouse_test.go b/bundle/config/variable/resolve_warehouse_test.go new file mode 100644 index 00000000..68e3925b --- /dev/null +++ b/bundle/config/variable/resolve_warehouse_test.go @@ -0,0 +1,49 @@ +package variable + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveWarehouse_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockWarehousesAPI() + api.EXPECT(). + GetByName(mock.Anything, "warehouse"). + Return(&sql.EndpointInfo{ + Id: "abcd", + }, nil) + + ctx := context.Background() + l := resolveWarehouse{name: "warehouse"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "abcd", result) +} + +func TestResolveWarehouse_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockWarehousesAPI() + api.EXPECT(). + GetByName(mock.Anything, "warehouse"). + Return(nil, &apierr.APIError{StatusCode: 404}) + + ctx := context.Background() + l := resolveWarehouse{name: "warehouse"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.ErrorIs(t, err, apierr.ErrNotFound) +} + +func TestResolveWarehouse_String(t *testing.T) { + l := resolveWarehouse{name: "name"} + assert.Equal(t, "warehouse: name", l.String()) +} From c2e2abcc35bdbc822681ff577728bd3685ac0e9f Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:21:21 +0530 Subject: [PATCH 11/24] Extend "notebook not found" error to warn about missing extension (#1920) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Changes The full workspace path for a notebook does not contain the notebook's extension. If a user converts that file path to a relative path (like `/Workspace/bundle_root/bar/nb` -> `./bar/nb`), they can be confused as to why the new file path does not work. The changes in this PR nudge them to add the appropriate file extension (e.g., `./bar/nb.py` or `./bar/nb.ipynb`). One common way users can end up in this scenario is by using the view job as YAML functionality in the Databricks UI. ## Tests Unit test and manually. ``` (.venv) ➜ bundle-playground git:(master) ✗ cli bundle validate Error: notebook ./foo not found. Local notebook references are expected to contain one of the following file extensions: [.py, .r, .scala, .sql, .ipynb] ``` --- bundle/config/mutator/translate_paths.go | 28 +++++++++- bundle/config/mutator/translate_paths_test.go | 54 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 1e2484c7..5e016d8a 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -126,7 +126,33 @@ func (t *translateContext) rewritePath( func (t *translateContext) translateNotebookPath(literal, localFullPath, localRelPath, remotePath string) (string, error) { nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath)) if errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("notebook %s not found", literal) + if filepath.Ext(localFullPath) != notebook.ExtensionNone { + return "", fmt.Errorf("notebook %s not found", literal) + } + + extensions := []string{ + notebook.ExtensionPython, + notebook.ExtensionR, + notebook.ExtensionScala, + notebook.ExtensionSql, + notebook.ExtensionJupyter, + } + + // Check whether a file with a notebook extension already exists. This + // way we can provide a more targeted error message. + for _, ext := range extensions { + literalWithExt := literal + ext + localRelPathWithExt := filepath.ToSlash(localRelPath + ext) + if _, err := fs.Stat(t.b.SyncRoot, localRelPathWithExt); err == nil { + return "", fmt.Errorf(`notebook %s not found. Did you mean %s? +Local notebook references are expected to contain one of the following +file extensions: [%s]`, literal, literalWithExt, strings.Join(extensions, ", ")) + } + } + + // Return a generic error message if no matching possible file is found. + return "", fmt.Errorf(`notebook %s not found. Local notebook references are expected +to contain one of the following file extensions: [%s]`, literal, strings.Join(extensions, ", ")) } if err != nil { return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localFullPath, err) diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index a2032f81..bf6ba15d 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -2,6 +2,7 @@ package mutator_test import ( "context" + "fmt" "os" "path/filepath" "runtime" @@ -508,6 +509,59 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { assert.EqualError(t, diags.Error(), "notebook ./doesnt_exist.py not found") } +func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) { + for _, ext := range []string{ + ".py", + ".r", + ".scala", + ".sql", + ".ipynb", + "", + } { + t.Run("case_"+ext, func(t *testing.T) { + dir := t.TempDir() + + if ext != "" { + touchEmptyFile(t, filepath.Join(dir, "foo"+ext)) + } + + b := &bundle.Bundle{ + SyncRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + PipelineSpec: &pipelines.PipelineSpec{ + Libraries: []pipelines.PipelineLibrary{ + { + Notebook: &pipelines.NotebookLibrary{ + Path: "./foo", + }, + }, + }, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) + diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) + + if ext == "" { + assert.EqualError(t, diags.Error(), `notebook ./foo not found. Local notebook references are expected +to contain one of the following file extensions: [.py, .r, .scala, .sql, .ipynb]`) + } else { + assert.EqualError(t, diags.Error(), fmt.Sprintf(`notebook ./foo not found. Did you mean ./foo%s? +Local notebook references are expected to contain one of the following +file extensions: [.py, .r, .scala, .sql, .ipynb]`, ext)) + } + }) + } +} + func TestPipelineFileDoesNotExistError(t *testing.T) { dir := t.TempDir() From abc2f3c825e1e445ba561d62118025120d07928f Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:16:24 +0530 Subject: [PATCH 12/24] Fix `TestAccBundleInitOnMlopsStacks` (#1924) ## Changes The ML production team modified mlops-stack to use `mode: development` for their development target here: https://github.com/databricks/mlops-stacks/pull/174 This PR makes the integration test assertion agnostic of the prefix to make it pass again. ## Tests The test passes now --- internal/init_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/init_test.go b/internal/init_test.go index a6241d62..25bfc19d 100644 --- a/internal/init_test.go +++ b/internal/init_test.go @@ -97,7 +97,7 @@ func TestAccBundleInitOnMlopsStacks(t *testing.T) { require.NoError(t, err) job, err := w.Jobs.GetByJobId(context.Background(), batchJobId) assert.NoError(t, err) - assert.Equal(t, fmt.Sprintf("dev-%s-batch-inference-job", projectName), job.Settings.Name) + assert.Contains(t, job.Settings.Name, fmt.Sprintf("dev-%s-batch-inference-job", projectName)) } func TestAccBundleInitHelpers(t *testing.T) { From a3cea07c9e41229cbaea8af3fbb100a20f0d529f Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 21 Nov 2024 15:52:14 +0100 Subject: [PATCH 13/24] Support lookup by name of notification destinations (#1922) ## Changes Add support for notification destinations in variable lookups. More information: https://docs.databricks.com/en/admin/workspace-settings/notification-destinations.html Depends on #1921. ## Tests * New unit test * Manually confirmed that the lookup works --- bundle/config/variable/lookup.go | 5 ++ .../resolve_notification_destination.go | 46 +++++++++++ .../resolve_notification_destination_test.go | 82 +++++++++++++++++++ 3 files changed, 133 insertions(+) create mode 100644 bundle/config/variable/resolve_notification_destination.go create mode 100644 bundle/config/variable/resolve_notification_destination_test.go diff --git a/bundle/config/variable/lookup.go b/bundle/config/variable/lookup.go index f8cb6719..37e380f1 100755 --- a/bundle/config/variable/lookup.go +++ b/bundle/config/variable/lookup.go @@ -22,6 +22,8 @@ type Lookup struct { Metastore string `json:"metastore,omitempty"` + NotificationDestination string `json:"notification_destination,omitempty"` + Pipeline string `json:"pipeline,omitempty"` Query string `json:"query,omitempty"` @@ -63,6 +65,9 @@ func (l *Lookup) constructResolver() (resolver, error) { if l.Metastore != "" { resolvers = append(resolvers, resolveMetastore{name: l.Metastore}) } + if l.NotificationDestination != "" { + resolvers = append(resolvers, resolveNotificationDestination{name: l.NotificationDestination}) + } if l.Pipeline != "" { resolvers = append(resolvers, resolvePipeline{name: l.Pipeline}) } diff --git a/bundle/config/variable/resolve_notification_destination.go b/bundle/config/variable/resolve_notification_destination.go new file mode 100644 index 00000000..4c4cd892 --- /dev/null +++ b/bundle/config/variable/resolve_notification_destination.go @@ -0,0 +1,46 @@ +package variable + +import ( + "context" + "fmt" + + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/settings" +) + +type resolveNotificationDestination struct { + name string +} + +func (l resolveNotificationDestination) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) { + result, err := w.NotificationDestinations.ListAll(ctx, settings.ListNotificationDestinationsRequest{ + // The default page size for this API is 20. + // We use a higher value to make fewer API calls. + PageSize: 200, + }) + if err != nil { + return "", err + } + + // Collect all notification destinations with the given name. + var entities []settings.ListNotificationDestinationsResult + for _, entity := range result { + if entity.DisplayName == l.name { + entities = append(entities, entity) + } + } + + // Return the ID of the first matching notification destination. + switch len(entities) { + case 0: + return "", fmt.Errorf("notification destination named %q does not exist", l.name) + case 1: + return entities[0].Id, nil + default: + return "", fmt.Errorf("there are %d instances of clusters named %q", len(entities), l.name) + } +} + +func (l resolveNotificationDestination) String() string { + return fmt.Sprintf("notification-destination: %s", l.name) +} diff --git a/bundle/config/variable/resolve_notification_destination_test.go b/bundle/config/variable/resolve_notification_destination_test.go new file mode 100644 index 00000000..2b8201d1 --- /dev/null +++ b/bundle/config/variable/resolve_notification_destination_test.go @@ -0,0 +1,82 @@ +package variable + +import ( + "context" + "fmt" + "testing" + + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/settings" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestResolveNotificationDestination_ResolveSuccess(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockNotificationDestinationsAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return([]settings.ListNotificationDestinationsResult{ + {Id: "1234", DisplayName: "destination"}, + }, nil) + + ctx := context.Background() + l := resolveNotificationDestination{name: "destination"} + result, err := l.Resolve(ctx, m.WorkspaceClient) + require.NoError(t, err) + assert.Equal(t, "1234", result) +} + +func TestResolveNotificationDestination_ResolveError(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockNotificationDestinationsAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return(nil, fmt.Errorf("bad")) + + ctx := context.Background() + l := resolveNotificationDestination{name: "destination"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + assert.ErrorContains(t, err, "bad") +} + +func TestResolveNotificationDestination_ResolveNotFound(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockNotificationDestinationsAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return([]settings.ListNotificationDestinationsResult{}, nil) + + ctx := context.Background() + l := resolveNotificationDestination{name: "destination"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.Error(t, err) + assert.ErrorContains(t, err, `notification destination named "destination" does not exist`) +} + +func TestResolveNotificationDestination_ResolveMultiple(t *testing.T) { + m := mocks.NewMockWorkspaceClient(t) + + api := m.GetMockNotificationDestinationsAPI() + api.EXPECT(). + ListAll(mock.Anything, mock.Anything). + Return([]settings.ListNotificationDestinationsResult{ + {Id: "1234", DisplayName: "destination"}, + {Id: "5678", DisplayName: "destination"}, + }, nil) + + ctx := context.Background() + l := resolveNotificationDestination{name: "destination"} + _, err := l.Resolve(ctx, m.WorkspaceClient) + require.Error(t, err) + assert.ErrorContains(t, err, `there are 2 instances of clusters named "destination"`) +} + +func TestResolveNotificationDestination_String(t *testing.T) { + l := resolveNotificationDestination{name: "name"} + assert.Equal(t, "notification-destination: name", l.String()) +} From abfd1713e088503eb869981b4f2f1aa6c9b810a5 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 21 Nov 2024 16:03:13 +0100 Subject: [PATCH 14/24] Skip sync warning if no sync paths are defined (#1926) ## Changes Users can configure the bundle to not synchronize any files with: ```yaml sync: paths: [] ``` If it is explicitly configured as an empty list, the validate command must not warn about not having any files to synchronize. The warning exists to alert users who are unintentionally not synchronizing any files (they might have a `.gitignore` pattern that matches everything). Closes #1663. ## Tests * New unit test. --- bundle/config/validate/files_to_sync.go | 7 ++ bundle/config/validate/files_to_sync_test.go | 105 +++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 bundle/config/validate/files_to_sync_test.go diff --git a/bundle/config/validate/files_to_sync.go b/bundle/config/validate/files_to_sync.go index 7cdad772..a1427848 100644 --- a/bundle/config/validate/files_to_sync.go +++ b/bundle/config/validate/files_to_sync.go @@ -21,6 +21,12 @@ func (v *filesToSync) Name() string { } func (v *filesToSync) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + // The user may be intentional about not synchronizing any files. + // In this case, we should not show any warnings. + if len(rb.Config().Sync.Paths) == 0 { + return nil + } + sync, err := files.GetSync(ctx, rb) if err != nil { return diag.FromErr(err) @@ -31,6 +37,7 @@ func (v *filesToSync) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag. return diag.FromErr(err) } + // If there are files to sync, we don't need to show any warnings. if len(fl) != 0 { return nil } diff --git a/bundle/config/validate/files_to_sync_test.go b/bundle/config/validate/files_to_sync_test.go new file mode 100644 index 00000000..2a598fa7 --- /dev/null +++ b/bundle/config/validate/files_to_sync_test.go @@ -0,0 +1,105 @@ +package validate + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/vfs" + sdkconfig "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/databricks/databricks-sdk-go/service/workspace" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestFilesToSync_NoPaths(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Sync: config.Sync{ + Paths: []string{}, + }, + }, + } + + ctx := context.Background() + rb := bundle.ReadOnly(b) + diags := bundle.ApplyReadOnly(ctx, rb, FilesToSync()) + assert.Empty(t, diags) +} + +func setupBundleForFilesToSyncTest(t *testing.T) *bundle.Bundle { + dir := t.TempDir() + + testutil.Touch(t, dir, "file1") + testutil.Touch(t, dir, "file2") + + b := &bundle.Bundle{ + BundleRootPath: dir, + BundleRoot: vfs.MustNew(dir), + SyncRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Bundle: config.Bundle{ + Target: "default", + }, + Workspace: config.Workspace{ + FilePath: "/this/doesnt/matter", + CurrentUser: &config.User{ + User: &iam.User{}, + }, + }, + Sync: config.Sync{ + // Paths are relative to [SyncRootPath]. + Paths: []string{"."}, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{ + Host: "https://foo.com", + } + + // The initialization logic in [sync.New] performs a check on the destination path. + // Removing this check at initialization time is tbd... + m.GetMockWorkspaceAPI().EXPECT().GetStatusByPath(mock.Anything, "/this/doesnt/matter").Return(&workspace.ObjectInfo{ + ObjectType: workspace.ObjectTypeDirectory, + }, nil) + + b.SetWorkpaceClient(m.WorkspaceClient) + return b +} + +func TestFilesToSync_EverythingIgnored(t *testing.T) { + b := setupBundleForFilesToSyncTest(t) + + // Ignore all files. + testutil.WriteFile(t, "*\n.*\n", b.BundleRootPath, ".gitignore") + + ctx := context.Background() + rb := bundle.ReadOnly(b) + diags := bundle.ApplyReadOnly(ctx, rb, FilesToSync()) + require.Equal(t, 1, len(diags)) + assert.Equal(t, diag.Warning, diags[0].Severity) + assert.Equal(t, "There are no files to sync, please check your .gitignore", diags[0].Summary) +} + +func TestFilesToSync_EverythingExcluded(t *testing.T) { + b := setupBundleForFilesToSyncTest(t) + + // Exclude all files. + b.Config.Sync.Exclude = []string{"*"} + + ctx := context.Background() + rb := bundle.ReadOnly(b) + diags := bundle.ApplyReadOnly(ctx, rb, FilesToSync()) + require.Equal(t, 1, len(diags)) + assert.Equal(t, diag.Warning, diags[0].Severity) + assert.Equal(t, "There are no files to sync, please check your .gitignore and sync.exclude configuration", diags[0].Summary) +} From 490dd058aa6bceee65b1778a50e3ef42ad9d11dd Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Fri, 22 Nov 2024 15:44:33 +0100 Subject: [PATCH 15/24] Extended message for warning when source-linked mode is used outside of the workspace (#1929) ## Changes Added path and locations to the warning which displayed when source-linked mode is used outside of the workspace --- bundle/config/mutator/apply_presets.go | 14 +++++++++++++- bundle/config/mutator/apply_presets_test.go | 4 ++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 9cec704e..38170375 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -225,9 +225,21 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos if config.IsExplicitlyEnabled((b.Config.Presets.SourceLinkedDeployment)) { isDatabricksWorkspace := dbr.RunsOnRuntime(ctx) && strings.HasPrefix(b.SyncRootPath, "/Workspace/") if !isDatabricksWorkspace { + target := b.Config.Bundle.Target + path := dyn.NewPath(dyn.Key("targets"), dyn.Key(target), dyn.Key("presets"), dyn.Key("source_linked_deployment")) + diags = diags.Append( + diag.Diagnostic{ + Severity: diag.Warning, + Summary: "source-linked deployment is available only in the Databricks Workspace", + Paths: []dyn.Path{ + path, + }, + Locations: b.Config.GetLocations(path[2:].String()), + }, + ) + disabled := false b.Config.Presets.SourceLinkedDeployment = &disabled - diags = diags.Extend(diag.Warningf("source-linked deployment is available only in the Databricks Workspace")) } } diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index f11a45d6..497ef051 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -9,7 +9,9 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dbr" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -435,6 +437,7 @@ func TestApplyPresetsSourceLinkedDeployment(t *testing.T) { }, } + bundletest.SetLocation(b, "presets.source_linked_deployment", []dyn.Location{{File: "databricks.yml"}}) diags := bundle.Apply(tt.ctx, b, mutator.ApplyPresets()) if diags.HasError() { t.Fatalf("unexpected error: %v", diags) @@ -442,6 +445,7 @@ func TestApplyPresetsSourceLinkedDeployment(t *testing.T) { if tt.expectedWarning != "" { require.Equal(t, tt.expectedWarning, diags[0].Summary) + require.NotEmpty(t, diags[0].Locations) } require.Equal(t, tt.expectedValue, b.Config.Presets.SourceLinkedDeployment) From b323703c1b1213bda89ba5f63b3e69073623d2bd Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Fri, 22 Nov 2024 21:18:09 +0530 Subject: [PATCH 16/24] Add validation for single node clusters (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Changes This PR adds a warning validating that the configuration for a single node cluster is valid for interactive, job, job-task, and pipeline clusters. Note: We skip the validation if a cluster policy is configured because the policy is likely to configure `spark_conf` / `custom_tags` itself. Note: Terrform originally only had validation for interactive, job, and job-task clusters. This PR adding the validation for pipeline clusters as well is new. This PR follows the same logic as we used to have in Terraform. The validation was removed from Terraform because we had no way to demote the error to a warning: https://github.com/databricks/terraform-provider-databricks/pull/4222 ### Background Single-node clusters require `spark_conf` and `custom_tags` to be correctly set in the cluster definition for them to function optimally. The cluster will be created even if incorrectly configured, but its performance will not be great. For example, if both `spark_conf` and `custom_tags` are not set and `num_workers` is 0, then only the driver process will be launched on the cluster compute instance thus leading to sub-optimal utilization of available compute resources and no parallelization across worker processes when processing a spark query. ### Issue This PR addresses some issues reported in https://github.com/databricks/cli/issues/1546 ## Tests Unit tests and manually. Example output of the warning: ``` ➜ bundle-playground git:(master) ✗ cli bundle validate Warning: Single node cluster is not correctly configured at resources.pipelines.bar.clusters[0] in databricks.yml:29:11 num_workers should be 0 only for single-node clusters. To create a valid single node cluster please ensure that the following properties are correctly set in the cluster specification: spark_conf: spark.databricks.cluster.profile: singleNode spark.master: local[*] custom_tags: ResourceClass: SingleNode Name: foobar Target: default Workspace: User: shreyas.goenka@databricks.com Path: /Workspace/Users/shreyas.goenka@databricks.com/.bundle/foobar/default Found 1 warning ``` --- bundle/config/validate/single_node_cluster.go | 137 +++++ .../validate/single_node_cluster_test.go | 566 ++++++++++++++++++ bundle/config/validate/validate.go | 1 + 3 files changed, 704 insertions(+) create mode 100644 bundle/config/validate/single_node_cluster.go create mode 100644 bundle/config/validate/single_node_cluster_test.go diff --git a/bundle/config/validate/single_node_cluster.go b/bundle/config/validate/single_node_cluster.go new file mode 100644 index 00000000..7c159f61 --- /dev/null +++ b/bundle/config/validate/single_node_cluster.go @@ -0,0 +1,137 @@ +package validate + +import ( + "context" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/log" +) + +// Validates that any single node clusters defined in the bundle are correctly configured. +func SingleNodeCluster() bundle.ReadOnlyMutator { + return &singleNodeCluster{} +} + +type singleNodeCluster struct{} + +func (m *singleNodeCluster) Name() string { + return "validate:SingleNodeCluster" +} + +const singleNodeWarningDetail = `num_workers should be 0 only for single-node clusters. To create a +valid single node cluster please ensure that the following properties +are correctly set in the cluster specification: + + spark_conf: + spark.databricks.cluster.profile: singleNode + spark.master: local[*] + + custom_tags: + ResourceClass: SingleNode + ` + +const singleNodeWarningSummary = `Single node cluster is not correctly configured` + +func showSingleNodeClusterWarning(ctx context.Context, v dyn.Value) bool { + // Check if the user has explicitly set the num_workers to 0. Skip the warning + // if that's not the case. + numWorkers, ok := v.Get("num_workers").AsInt() + if !ok || numWorkers > 0 { + return false + } + + // Convenient type that contains the common fields from compute.ClusterSpec and + // pipelines.PipelineCluster that we are interested in. + type ClusterConf struct { + SparkConf map[string]string `json:"spark_conf"` + CustomTags map[string]string `json:"custom_tags"` + PolicyId string `json:"policy_id"` + } + + conf := &ClusterConf{} + err := convert.ToTyped(conf, v) + if err != nil { + return false + } + + // If the policy id is set, we don't want to show the warning. This is because + // the user might have configured `spark_conf` and `custom_tags` correctly + // in their cluster policy. + if conf.PolicyId != "" { + return false + } + + profile, ok := conf.SparkConf["spark.databricks.cluster.profile"] + if !ok { + log.Debugf(ctx, "spark_conf spark.databricks.cluster.profile not found in single-node cluster spec") + return true + } + if profile != "singleNode" { + log.Debugf(ctx, "spark_conf spark.databricks.cluster.profile is not singleNode in single-node cluster spec: %s", profile) + return true + } + + master, ok := conf.SparkConf["spark.master"] + if !ok { + log.Debugf(ctx, "spark_conf spark.master not found in single-node cluster spec") + return true + } + if !strings.HasPrefix(master, "local") { + log.Debugf(ctx, "spark_conf spark.master does not start with local in single-node cluster spec: %s", master) + return true + } + + resourceClass, ok := conf.CustomTags["ResourceClass"] + if !ok { + log.Debugf(ctx, "custom_tag ResourceClass not found in single-node cluster spec") + return true + } + if resourceClass != "SingleNode" { + log.Debugf(ctx, "custom_tag ResourceClass is not SingleNode in single-node cluster spec: %s", resourceClass) + return true + } + + return false +} + +func (m *singleNodeCluster) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + patterns := []dyn.Pattern{ + // Interactive clusters + dyn.NewPattern(dyn.Key("resources"), dyn.Key("clusters"), dyn.AnyKey()), + // Job clusters + dyn.NewPattern(dyn.Key("resources"), dyn.Key("jobs"), dyn.AnyKey(), dyn.Key("job_clusters"), dyn.AnyIndex(), dyn.Key("new_cluster")), + // Job task clusters + dyn.NewPattern(dyn.Key("resources"), dyn.Key("jobs"), dyn.AnyKey(), dyn.Key("tasks"), dyn.AnyIndex(), dyn.Key("new_cluster")), + // Job for each task clusters + dyn.NewPattern(dyn.Key("resources"), dyn.Key("jobs"), dyn.AnyKey(), dyn.Key("tasks"), dyn.AnyIndex(), dyn.Key("for_each_task"), dyn.Key("task"), dyn.Key("new_cluster")), + // Pipeline clusters + dyn.NewPattern(dyn.Key("resources"), dyn.Key("pipelines"), dyn.AnyKey(), dyn.Key("clusters"), dyn.AnyIndex()), + } + + for _, p := range patterns { + _, err := dyn.MapByPattern(rb.Config().Value(), p, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + warning := diag.Diagnostic{ + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: v.Locations(), + Paths: []dyn.Path{p}, + } + + if showSingleNodeClusterWarning(ctx, v) { + diags = append(diags, warning) + } + return v, nil + }) + if err != nil { + log.Debugf(ctx, "Error while applying single node cluster validation: %s", err) + } + } + return diags +} diff --git a/bundle/config/validate/single_node_cluster_test.go b/bundle/config/validate/single_node_cluster_test.go new file mode 100644 index 00000000..18771cc0 --- /dev/null +++ b/bundle/config/validate/single_node_cluster_test.go @@ -0,0 +1,566 @@ +package validate + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/pipelines" + "github.com/stretchr/testify/assert" +) + +func failCases() []struct { + name string + sparkConf map[string]string + customTags map[string]string +} { + return []struct { + name string + sparkConf map[string]string + customTags map[string]string + }{ + { + name: "no tags or conf", + }, + { + name: "no tags", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + "spark.master": "local[*]", + }, + }, + { + name: "no conf", + customTags: map[string]string{"ResourceClass": "SingleNode"}, + }, + { + name: "invalid spark cluster profile", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "invalid", + "spark.master": "local[*]", + }, + customTags: map[string]string{"ResourceClass": "SingleNode"}, + }, + { + name: "invalid spark.master", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + "spark.master": "invalid", + }, + customTags: map[string]string{"ResourceClass": "SingleNode"}, + }, + { + name: "invalid tags", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + "spark.master": "local[*]", + }, + customTags: map[string]string{"ResourceClass": "invalid"}, + }, + { + name: "missing ResourceClass tag", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + "spark.master": "local[*]", + }, + customTags: map[string]string{"what": "ever"}, + }, + { + name: "missing spark.master", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + }, + customTags: map[string]string{"ResourceClass": "SingleNode"}, + }, + { + name: "missing spark.databricks.cluster.profile", + sparkConf: map[string]string{ + "spark.master": "local[*]", + }, + customTags: map[string]string{"ResourceClass": "SingleNode"}, + }, + } +} + +func TestValidateSingleNodeClusterFailForInteractiveClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range failCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Clusters: map[string]*resources.Cluster{ + "foo": { + ClusterSpec: &compute.ClusterSpec{ + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.clusters.foo", []dyn.Location{{File: "a.yml", Line: 1, Column: 1}}) + + // We can't set num_workers to 0 explicitly in the typed configuration. + // Do it on the dyn.Value directly. + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.clusters.foo.num_workers", dyn.V(0)) + }) + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: []dyn.Location{{File: "a.yml", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.NewPath(dyn.Key("resources"), dyn.Key("clusters"), dyn.Key("foo"))}, + }, + }, diags) + }) + } +} + +func TestValidateSingleNodeClusterFailForJobClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range failCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + JobClusters: []jobs.JobCluster{ + { + NewCluster: compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + }, + }, + }, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.jobs.foo.job_clusters[0].new_cluster", []dyn.Location{{File: "b.yml", Line: 1, Column: 1}}) + + // We can't set num_workers to 0 explicitly in the typed configuration. + // Do it on the dyn.Value directly. + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.job_clusters[0].new_cluster.num_workers", dyn.V(0)) + }) + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: []dyn.Location{{File: "b.yml", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.jobs.foo.job_clusters[0].new_cluster")}, + }, + }, diags) + + }) + } +} + +func TestValidateSingleNodeClusterFailForJobTaskClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range failCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + NewCluster: &compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + }, + }, + }, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.jobs.foo.tasks[0].new_cluster", []dyn.Location{{File: "c.yml", Line: 1, Column: 1}}) + + // We can't set num_workers to 0 explicitly in the typed configuration. + // Do it on the dyn.Value directly. + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.tasks[0].new_cluster.num_workers", dyn.V(0)) + }) + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: []dyn.Location{{File: "c.yml", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.jobs.foo.tasks[0].new_cluster")}, + }, + }, diags) + }) + } +} + +func TestValidateSingleNodeClusterFailForPipelineClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range failCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "foo": { + PipelineSpec: &pipelines.PipelineSpec{ + Clusters: []pipelines.PipelineCluster{ + { + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + }, + }, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.pipelines.foo.clusters[0]", []dyn.Location{{File: "d.yml", Line: 1, Column: 1}}) + + // We can't set num_workers to 0 explicitly in the typed configuration. + // Do it on the dyn.Value directly. + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.pipelines.foo.clusters[0].num_workers", dyn.V(0)) + }) + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: []dyn.Location{{File: "d.yml", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.pipelines.foo.clusters[0]")}, + }, + }, diags) + }) + } +} + +func TestValidateSingleNodeClusterFailForJobForEachTaskCluster(t *testing.T) { + ctx := context.Background() + + for _, tc := range failCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + ForEachTask: &jobs.ForEachTask{ + Task: jobs.Task{ + NewCluster: &compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.jobs.foo.tasks[0].for_each_task.task.new_cluster", []dyn.Location{{File: "e.yml", Line: 1, Column: 1}}) + + // We can't set num_workers to 0 explicitly in the typed configuration. + // Do it on the dyn.Value directly. + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.tasks[0].for_each_task.task.new_cluster.num_workers", dyn.V(0)) + }) + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: singleNodeWarningSummary, + Detail: singleNodeWarningDetail, + Locations: []dyn.Location{{File: "e.yml", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.jobs.foo.tasks[0].for_each_task.task.new_cluster")}, + }, + }, diags) + }) + } +} + +func passCases() []struct { + name string + numWorkers *int + sparkConf map[string]string + customTags map[string]string + policyId string +} { + zero := 0 + one := 1 + + return []struct { + name string + numWorkers *int + sparkConf map[string]string + customTags map[string]string + policyId string + }{ + { + name: "single node cluster", + sparkConf: map[string]string{ + "spark.databricks.cluster.profile": "singleNode", + "spark.master": "local[*]", + }, + customTags: map[string]string{ + "ResourceClass": "SingleNode", + }, + numWorkers: &zero, + }, + { + name: "num workers is not zero", + numWorkers: &one, + }, + { + name: "num workers is not set", + }, + { + name: "policy id is not empty", + policyId: "policy-abc", + numWorkers: &zero, + }, + } +} + +func TestValidateSingleNodeClusterPassInteractiveClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range passCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Clusters: map[string]*resources.Cluster{ + "foo": { + ClusterSpec: &compute.ClusterSpec{ + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + PolicyId: tc.policyId, + }, + }, + }, + }, + }, + } + + if tc.numWorkers != nil { + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.clusters.foo.num_workers", dyn.V(*tc.numWorkers)) + }) + } + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Empty(t, diags) + }) + } +} + +func TestValidateSingleNodeClusterPassJobClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range passCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + JobClusters: []jobs.JobCluster{ + { + NewCluster: compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + PolicyId: tc.policyId, + }, + }, + }, + }, + }, + }, + }, + }, + } + + if tc.numWorkers != nil { + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.job_clusters[0].new_cluster.num_workers", dyn.V(*tc.numWorkers)) + }) + } + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Empty(t, diags) + }) + } +} + +func TestValidateSingleNodeClusterPassJobTaskClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range passCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + NewCluster: &compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + PolicyId: tc.policyId, + }, + }, + }, + }, + }, + }, + }, + }, + } + + if tc.numWorkers != nil { + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.tasks[0].new_cluster.num_workers", dyn.V(*tc.numWorkers)) + }) + } + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Empty(t, diags) + }) + } +} + +func TestValidateSingleNodeClusterPassPipelineClusters(t *testing.T) { + ctx := context.Background() + + for _, tc := range passCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "foo": { + PipelineSpec: &pipelines.PipelineSpec{ + Clusters: []pipelines.PipelineCluster{ + { + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + PolicyId: tc.policyId, + }, + }, + }, + }, + }, + }, + }, + } + + if tc.numWorkers != nil { + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.pipelines.foo.clusters[0].num_workers", dyn.V(*tc.numWorkers)) + }) + } + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Empty(t, diags) + }) + } +} + +func TestValidateSingleNodeClusterPassJobForEachTaskCluster(t *testing.T) { + ctx := context.Background() + + for _, tc := range passCases() { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + ForEachTask: &jobs.ForEachTask{ + Task: jobs.Task{ + NewCluster: &compute.ClusterSpec{ + ClusterName: "my_cluster", + SparkConf: tc.sparkConf, + CustomTags: tc.customTags, + PolicyId: tc.policyId, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + if tc.numWorkers != nil { + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.jobs.foo.tasks[0].for_each_task.task.new_cluster.num_workers", dyn.V(*tc.numWorkers)) + }) + } + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), SingleNodeCluster()) + assert.Empty(t, diags) + }) + } +} diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index 440477e6..eb4c3c3c 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -36,6 +36,7 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics ValidateSyncPatterns(), JobTaskClusterSpec(), ValidateFolderPermissions(), + SingleNodeCluster(), )) } From 4b069bb6e1ec3f349b86fe4e97995f2f2b64a328 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:46:20 +0000 Subject: [PATCH 17/24] Bump golang.org/x/term from 0.25.0 to 0.26.0 (#1907) Bumps [golang.org/x/term](https://github.com/golang/term) from 0.25.0 to 0.26.0.
Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=golang.org/x/term&package-manager=go_modules&previous-version=0.25.0&new-version=0.26.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 9ae5fde0..73b9984d 100644 --- a/go.mod +++ b/go.mod @@ -27,7 +27,7 @@ require ( golang.org/x/mod v0.22.0 golang.org/x/oauth2 v0.24.0 golang.org/x/sync v0.9.0 - golang.org/x/term v0.25.0 + golang.org/x/term v0.26.0 golang.org/x/text v0.20.0 gopkg.in/ini.v1 v1.67.0 // Apache 2.0 gopkg.in/yaml.v3 v3.0.1 @@ -64,7 +64,7 @@ require ( go.opentelemetry.io/otel/trace v1.24.0 // indirect golang.org/x/crypto v0.24.0 // indirect golang.org/x/net v0.26.0 // indirect - golang.org/x/sys v0.26.0 // indirect + golang.org/x/sys v0.27.0 // indirect golang.org/x/time v0.5.0 // indirect google.golang.org/api v0.182.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240521202816-d264139d666e // indirect diff --git a/go.sum b/go.sum index 2bfcfb2f..928827d9 100644 --- a/go.sum +++ b/go.sum @@ -212,10 +212,10 @@ golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo= -golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.25.0 h1:WtHI/ltw4NvSUig5KARz9h521QvRC8RmF/cuYqifU24= -golang.org/x/term v0.25.0/go.mod h1:RPyXicDX+6vLxogjjRxjgD2TKtmAO6NZBsBRfrOLu7M= +golang.org/x/sys v0.27.0 h1:wBqf8DvsY9Y/2P8gAfPDEYNuS30J4lPHJxXSb/nJZ+s= +golang.org/x/sys v0.27.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.26.0 h1:WEQa6V3Gja/BhNxg540hBip/kkaYtRg3cxg4oXSw4AU= +golang.org/x/term v0.26.0/go.mod h1:Si5m1o57C5nBNQo5z1iq+XDijt21BDBDp2bK0QI8e3E= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug= From 026c5555b2757d9575053ae02ecebb52a8d78a51 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:52:43 +0100 Subject: [PATCH 18/24] Bump github.com/Masterminds/semver/v3 from 3.3.0 to 3.3.1 (#1930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [github.com/Masterminds/semver/v3](https://github.com/Masterminds/semver) from 3.3.0 to 3.3.1.
Release notes

Sourced from github.com/Masterminds/semver/v3's releases.

v3.3.1

What's Changed

Full Changelog: https://github.com/Masterminds/semver/compare/v3.3.0...v3.3.1

Changelog

Sourced from github.com/Masterminds/semver/v3's changelog.

Changelog

Commits
  • 1558ca3 Merge pull request #253 from mattfarina/fix-bad-versions
  • 252dd61 Fix for allowing some version that were invalid
  • See full diff in compare view

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/Masterminds/semver/v3&package-manager=go_modules&previous-version=3.3.0&new-version=3.3.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 73b9984d..ab519450 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.23 toolchain go1.23.2 require ( - github.com/Masterminds/semver/v3 v3.3.0 // MIT + github.com/Masterminds/semver/v3 v3.3.1 // MIT github.com/briandowns/spinner v1.23.1 // Apache 2.0 github.com/databricks/databricks-sdk-go v0.51.0 // Apache 2.0 github.com/fatih/color v1.18.0 // MIT diff --git a/go.sum b/go.sum index 928827d9..6c318ac8 100644 --- a/go.sum +++ b/go.sum @@ -8,8 +8,8 @@ cloud.google.com/go/compute/metadata v0.3.0/go.mod h1:zFmK7XCadkQkj6TtorcaGlCW1h dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= -github.com/Masterminds/semver/v3 v3.3.0 h1:B8LGeaivUe71a5qox1ICM/JLl0NqZSW5CHyL+hmvYS0= -github.com/Masterminds/semver/v3 v3.3.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= +github.com/Masterminds/semver/v3 v3.3.1 h1:QtNSWtVZ3nBfk8mAOu/B6v7FMJ+NHTIgUPi7rj+4nv4= +github.com/Masterminds/semver/v3 v3.3.1/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow= github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= github.com/ProtonMail/go-crypto v1.1.0-alpha.2 h1:bkyFVUP+ROOARdgCiJzNQo2V2kiB97LyUpzH9P6Hrlg= From 85c0d2d3ee7e81070e4af8f8f79b59cacaeb6fb9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 26 Nov 2024 17:12:11 +0000 Subject: [PATCH 19/24] Bump github.com/stretchr/testify from 1.9.0 to 1.10.0 (#1932) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.9.0 to 1.10.0.
Release notes

Sourced from github.com/stretchr/testify's releases.

v1.10.0

What's Changed

Functional Changes

Fixes

Documantation, Build & CI

New Contributors

... (truncated)

Commits
  • 89cbdd9 Merge pull request #1626 from arjun-1/fix-functional-options-diff-indirect-calls
  • 07bac60 Merge pull request #1667 from sikehish/flaky
  • 716de8d Increase timeouts in Test_Mock_Called_blocks to reduce flakiness in CI
  • 118fb83 NotSame should fail if args are not pointers #1661 (#1664)
  • 7d99b2b attempt 2
  • 05f87c0 more similar
  • ea7129e better fmt
  • a1b9c9e Merge pull request #1663 from ybrustin/master
  • 8302de9 Merge branch 'master' into master
  • 89352f7 Merge pull request #1518 from hendrywiranto/adjust-readme-remove-v2
  • Additional commits viewable in compare view

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/stretchr/testify&package-manager=go_modules&previous-version=1.9.0&new-version=1.10.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index ab519450..35331313 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06 // MIT github.com/spf13/cobra v1.8.1 // Apache 2.0 github.com/spf13/pflag v1.0.5 // BSD-3-Clause - github.com/stretchr/testify v1.9.0 // MIT + github.com/stretchr/testify v1.10.0 // MIT golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 golang.org/x/mod v0.22.0 golang.org/x/oauth2 v0.24.0 diff --git a/go.sum b/go.sum index 6c318ac8..312a4d93 100644 --- a/go.sum +++ b/go.sum @@ -156,8 +156,8 @@ github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM= github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw= github.com/zclconf/go-cty v1.15.0 h1:tTCRWxsexYUmtt/wVxgDClUe+uQusuI443uL6e+5sXQ= From fae1b6742dfc173ac27847e99e40e7f7171eb8be Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 27 Nov 2024 12:51:08 +0100 Subject: [PATCH 20/24] Update target references to use `${bundle.target}` (#1935) ## Changes The built-in template contains a reference to `${bundle.environment}`. This property has been deprecated in favor of `${bundle.target}` a long time ago (#670), so we should no longer emit it. The environment field will continue to be usable until we cut a new major version in some far away future. ## Tests * Unit tests * The test `TestInterpolationWithTarget` still covers correct interpolation of `${bundle.environment}` --- bundle/tests/python_wheel/python_wheel/bundle.yml | 2 +- bundle/tests/python_wheel/python_wheel_dbfs_lib/bundle.yml | 2 +- bundle/tests/python_wheel/python_wheel_multiple/bundle.yml | 2 +- bundle/tests/python_wheel/python_wheel_no_artifact/bundle.yml | 2 +- .../python_wheel/python_wheel_no_artifact_no_setup/bundle.yml | 2 +- .../python_wheel/python_wheel_no_artifact_notebook/bundle.yml | 2 +- bundle/tests/python_wheel/python_wheel_no_build/bundle.yml | 2 +- .../resources/{{.project_name}}.pipeline.yml.tmpl | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/bundle/tests/python_wheel/python_wheel/bundle.yml b/bundle/tests/python_wheel/python_wheel/bundle.yml index c82ff83f..017fe1c4 100644 --- a/bundle/tests/python_wheel/python_wheel/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel/bundle.yml @@ -10,7 +10,7 @@ artifacts: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-132531-5opeqon1" diff --git a/bundle/tests/python_wheel/python_wheel_dbfs_lib/bundle.yml b/bundle/tests/python_wheel/python_wheel_dbfs_lib/bundle.yml index 07f4957b..fe2723aa 100644 --- a/bundle/tests/python_wheel/python_wheel_dbfs_lib/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_dbfs_lib/bundle.yml @@ -4,7 +4,7 @@ bundle: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-132531-5opeqon1" diff --git a/bundle/tests/python_wheel/python_wheel_multiple/bundle.yml b/bundle/tests/python_wheel/python_wheel_multiple/bundle.yml index 6964c58a..77011041 100644 --- a/bundle/tests/python_wheel/python_wheel_multiple/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_multiple/bundle.yml @@ -14,7 +14,7 @@ artifacts: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-132531-5opeqon1" diff --git a/bundle/tests/python_wheel/python_wheel_no_artifact/bundle.yml b/bundle/tests/python_wheel/python_wheel_no_artifact/bundle.yml index 88cb47be..d1d0a4db 100644 --- a/bundle/tests/python_wheel/python_wheel_no_artifact/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_no_artifact/bundle.yml @@ -4,7 +4,7 @@ bundle: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-aaaaa-bbbbbb" diff --git a/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml b/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml index d0308430..948bf155 100644 --- a/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml @@ -7,7 +7,7 @@ workspace: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-aaaaa-bbbbbb" diff --git a/bundle/tests/python_wheel/python_wheel_no_artifact_notebook/bundle.yml b/bundle/tests/python_wheel/python_wheel_no_artifact_notebook/bundle.yml index 93e4e691..77fd6ad9 100644 --- a/bundle/tests/python_wheel/python_wheel_no_artifact_notebook/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_no_artifact_notebook/bundle.yml @@ -4,7 +4,7 @@ bundle: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-aaaaa-bbbbbb" diff --git a/bundle/tests/python_wheel/python_wheel_no_build/bundle.yml b/bundle/tests/python_wheel/python_wheel_no_build/bundle.yml index 91b8b155..e10e3993 100644 --- a/bundle/tests/python_wheel/python_wheel_no_build/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_no_build/bundle.yml @@ -4,7 +4,7 @@ bundle: resources: jobs: test_job: - name: "[${bundle.environment}] My Wheel Job" + name: "[${bundle.target}] My Wheel Job" tasks: - task_key: TestTask existing_cluster_id: "0717-132531-5opeqon1" diff --git a/libs/template/templates/default-python/template/{{.project_name}}/resources/{{.project_name}}.pipeline.yml.tmpl b/libs/template/templates/default-python/template/{{.project_name}}/resources/{{.project_name}}.pipeline.yml.tmpl index 1c6b8607..50f11fe2 100644 --- a/libs/template/templates/default-python/template/{{.project_name}}/resources/{{.project_name}}.pipeline.yml.tmpl +++ b/libs/template/templates/default-python/template/{{.project_name}}/resources/{{.project_name}}.pipeline.yml.tmpl @@ -9,7 +9,7 @@ resources: {{- else}} catalog: {{default_catalog}} {{- end}} - target: {{.project_name}}_${bundle.environment} + target: {{.project_name}}_${bundle.target} libraries: - notebook: path: ../src/dlt_pipeline.ipynb From e57cbf1273a43fb6042e4a33634f1ffd10c08dea Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 27 Nov 2024 13:14:39 +0100 Subject: [PATCH 21/24] Remove unused field: Repository.real (#1936) --- libs/git/repository.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/libs/git/repository.go b/libs/git/repository.go index 0bbd5786..f0e9e1eb 100644 --- a/libs/git/repository.go +++ b/libs/git/repository.go @@ -19,10 +19,6 @@ var GitDirectoryName = ".git" // Repository represents a Git repository or a directory // that could later be initialized as Git repository. type Repository struct { - // real indicates if this is a real repository or a non-Git - // directory where we process .gitignore files. - real bool - // rootDir is the path to the root of the repository checkout. // This can be either the main repository checkout or a worktree checkout. // For more information about worktrees, see: https://git-scm.com/docs/git-worktree#_description. @@ -209,7 +205,6 @@ func (r *Repository) Ignore(relPath string) (bool, error) { } func NewRepository(path vfs.Path) (*Repository, error) { - real := true rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) if err != nil { if !errors.Is(err, fs.ErrNotExist) { @@ -217,7 +212,6 @@ func NewRepository(path vfs.Path) (*Repository, error) { } // Cannot find `.git` directory. // Treat the specified path as a potential repository root checkout. - real = false rootDir = path } @@ -229,7 +223,6 @@ func NewRepository(path vfs.Path) (*Repository, error) { } repo := &Repository{ - real: real, rootDir: rootDir, gitDir: gitDir, gitCommonDir: gitCommonDir, From 6fc2093a22d4e366061e593d14ce78c7b6c79447 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 28 Nov 2024 09:52:21 +0100 Subject: [PATCH 22/24] Remove unused method GitRepository (#1941) --- bundle/bundle.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 85625568..46710538 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -17,7 +17,6 @@ import ( "github.com/databricks/cli/bundle/env" "github.com/databricks/cli/bundle/metadata" "github.com/databricks/cli/libs/fileset" - "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/tags" @@ -223,15 +222,6 @@ func (b *Bundle) GetSyncIncludePatterns(ctx context.Context) ([]string, error) { return append(b.Config.Sync.Include, filepath.ToSlash(filepath.Join(internalDirRel, "*.*"))), nil } -func (b *Bundle) GitRepository() (*git.Repository, error) { - _, err := vfs.FindLeafInTree(b.BundleRoot, ".git") - if err != nil { - return nil, fmt.Errorf("unable to locate repository root: %w", err) - } - - return git.NewRepository(b.BundleRoot) -} - // AuthEnv returns a map with environment variables and their values // derived from the workspace client configuration that was resolved // in the context of this bundle. From 8053e9c4e48501e1b5476f2cbb329a14a6e0b897 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 28 Nov 2024 13:27:24 +0100 Subject: [PATCH 23/24] Fix segfault in bundle summary command (#1937) ## Changes This PR introduces use of new `isNil` method. It allows to ensure we filter out all improperly defined resources in `bundle summary` command. This includes deleted resources or resources with incorrect configuration such as only defining key of the resource and nothing else. Fixes #1919, #1913 ## Tests Added regression unit test case --- bundle/config/resources.go | 6 +++++ bundle/config/resources/clusters.go | 4 +++ bundle/config/resources/dashboard.go | 4 +++ bundle/config/resources/job.go | 4 +++ bundle/config/resources/mlflow_experiment.go | 4 +++ bundle/config/resources/mlflow_model.go | 4 +++ .../resources/model_serving_endpoint.go | 4 +++ bundle/config/resources/pipeline.go | 4 +++ bundle/config/resources/quality_monitor.go | 4 +++ bundle/config/resources/registered_model.go | 4 +++ bundle/config/resources/schema.go | 4 +++ bundle/render/render_text_output_test.go | 4 +++ bundle/resources/completion_test.go | 22 ++++++++++++---- bundle/resources/lookup_test.go | 25 ++++++++++++++----- 14 files changed, 86 insertions(+), 11 deletions(-) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 0affb6ef..2886e357 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -41,6 +41,9 @@ type ConfigResource interface { // InitializeURL initializes the URL field of the resource. InitializeURL(baseURL url.URL) + + // IsNil returns true if the resource is nil, for example, when it was removed from the bundle. + IsNil() bool } // ResourceGroup represents a group of resources of the same type. @@ -57,6 +60,9 @@ func collectResourceMap[T ConfigResource]( ) ResourceGroup { resources := make(map[string]ConfigResource) for key, resource := range input { + if resource.IsNil() { + continue + } resources[key] = resource } return ResourceGroup{ diff --git a/bundle/config/resources/clusters.go b/bundle/config/resources/clusters.go index eb0247c6..ba991e86 100644 --- a/bundle/config/resources/clusters.go +++ b/bundle/config/resources/clusters.go @@ -56,3 +56,7 @@ func (s *Cluster) GetName() string { func (s *Cluster) GetURL() string { return s.URL } + +func (s *Cluster) IsNil() bool { + return s.ClusterSpec == nil +} diff --git a/bundle/config/resources/dashboard.go b/bundle/config/resources/dashboard.go index 724b0339..5c58a2f2 100644 --- a/bundle/config/resources/dashboard.go +++ b/bundle/config/resources/dashboard.go @@ -79,3 +79,7 @@ func (r *Dashboard) GetName() string { func (r *Dashboard) GetURL() string { return r.URL } + +func (r *Dashboard) IsNil() bool { + return r.Dashboard == nil +} diff --git a/bundle/config/resources/job.go b/bundle/config/resources/job.go index 98db1ec5..0aa41b2e 100644 --- a/bundle/config/resources/job.go +++ b/bundle/config/resources/job.go @@ -63,3 +63,7 @@ func (j *Job) GetName() string { func (j *Job) GetURL() string { return j.URL } + +func (j *Job) IsNil() bool { + return j.JobSettings == nil +} diff --git a/bundle/config/resources/mlflow_experiment.go b/bundle/config/resources/mlflow_experiment.go index a5871468..5d179ec0 100644 --- a/bundle/config/resources/mlflow_experiment.go +++ b/bundle/config/resources/mlflow_experiment.go @@ -58,3 +58,7 @@ func (s *MlflowExperiment) GetName() string { func (s *MlflowExperiment) GetURL() string { return s.URL } + +func (s *MlflowExperiment) IsNil() bool { + return s.Experiment == nil +} diff --git a/bundle/config/resources/mlflow_model.go b/bundle/config/resources/mlflow_model.go index 9ead254d..72376f45 100644 --- a/bundle/config/resources/mlflow_model.go +++ b/bundle/config/resources/mlflow_model.go @@ -58,3 +58,7 @@ func (s *MlflowModel) GetName() string { func (s *MlflowModel) GetURL() string { return s.URL } + +func (s *MlflowModel) IsNil() bool { + return s.Model == nil +} diff --git a/bundle/config/resources/model_serving_endpoint.go b/bundle/config/resources/model_serving_endpoint.go index 7f3ae00c..a3c472b3 100644 --- a/bundle/config/resources/model_serving_endpoint.go +++ b/bundle/config/resources/model_serving_endpoint.go @@ -66,3 +66,7 @@ func (s *ModelServingEndpoint) GetName() string { func (s *ModelServingEndpoint) GetURL() string { return s.URL } + +func (s *ModelServingEndpoint) IsNil() bool { + return s.CreateServingEndpoint == nil +} diff --git a/bundle/config/resources/pipeline.go b/bundle/config/resources/pipeline.go index b3311d8e..eaa4c536 100644 --- a/bundle/config/resources/pipeline.go +++ b/bundle/config/resources/pipeline.go @@ -58,3 +58,7 @@ func (p *Pipeline) GetName() string { func (s *Pipeline) GetURL() string { return s.URL } + +func (s *Pipeline) IsNil() bool { + return s.PipelineSpec == nil +} diff --git a/bundle/config/resources/quality_monitor.go b/bundle/config/resources/quality_monitor.go index 30ec4f91..b1d7e08a 100644 --- a/bundle/config/resources/quality_monitor.go +++ b/bundle/config/resources/quality_monitor.go @@ -62,3 +62,7 @@ func (s *QualityMonitor) GetName() string { func (s *QualityMonitor) GetURL() string { return s.URL } + +func (s *QualityMonitor) IsNil() bool { + return s.CreateMonitor == nil +} diff --git a/bundle/config/resources/registered_model.go b/bundle/config/resources/registered_model.go index c44526d0..8513a79a 100644 --- a/bundle/config/resources/registered_model.go +++ b/bundle/config/resources/registered_model.go @@ -68,3 +68,7 @@ func (s *RegisteredModel) GetName() string { func (s *RegisteredModel) GetURL() string { return s.URL } + +func (s *RegisteredModel) IsNil() bool { + return s.CreateRegisteredModelRequest == nil +} diff --git a/bundle/config/resources/schema.go b/bundle/config/resources/schema.go index a9f905cf..8eadd7e4 100644 --- a/bundle/config/resources/schema.go +++ b/bundle/config/resources/schema.go @@ -56,3 +56,7 @@ func (s *Schema) UnmarshalJSON(b []byte) error { func (s Schema) MarshalJSON() ([]byte, error) { return marshal.Marshal(s) } + +func (s *Schema) IsNil() bool { + return s.CreateSchema == nil +} diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index cd9e7723..135d79da 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -519,6 +519,10 @@ func TestRenderSummary(t *testing.T) { URL: "https://url2", JobSettings: &jobs.JobSettings{Name: "job2-name"}, }, + "job3": { + ID: "3", + URL: "https://url3", // This emulates deleted job + }, }, Pipelines: map[string]*resources.Pipeline{ "pipeline2": { diff --git a/bundle/resources/completion_test.go b/bundle/resources/completion_test.go index 2f7942aa..80412b6f 100644 --- a/bundle/resources/completion_test.go +++ b/bundle/resources/completion_test.go @@ -6,6 +6,8 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" ) @@ -14,11 +16,17 @@ func TestCompletions_SkipDuplicates(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, - "bar": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, + "bar": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "foo": {}, + "foo": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, @@ -36,10 +44,14 @@ func TestCompletions_Filter(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "bar": {}, + "bar": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, diff --git a/bundle/resources/lookup_test.go b/bundle/resources/lookup_test.go index b2eaafd1..0ea5af7a 100644 --- a/bundle/resources/lookup_test.go +++ b/bundle/resources/lookup_test.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -28,8 +29,12 @@ func TestLookup_NotFound(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, - "bar": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, + "bar": { + JobSettings: &jobs.JobSettings{}, + }, }, }, }, @@ -45,10 +50,14 @@ func TestLookup_MultipleFound(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "foo": {}, + "foo": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, @@ -92,10 +101,14 @@ func TestLookup_NominalWithFilters(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "bar": {}, + "bar": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, From 7b9726dd649efa169cd86c3c0d1f0395e4655a38 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 28 Nov 2024 15:33:51 +0000 Subject: [PATCH 24/24] Bump github.com/databricks/databricks-sdk-go from 0.51.0 to 0.52.0 (#1931) Bumps [github.com/databricks/databricks-sdk-go](https://github.com/databricks/databricks-sdk-go) from 0.51.0 to 0.52.0.
Release notes

Sourced from github.com/databricks/databricks-sdk-go's releases.

v0.52.0

Internal Changes

  • Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks (#1089).

API Changes:

OpenAPI SHA: f2385add116e3716c8a90a0b68e204deb40f996c, Date: 2024-11-15

Changelog

Sourced from github.com/databricks/databricks-sdk-go's changelog.

[Release] Release v0.52.0

Internal Changes

  • Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks (#1089).

API Changes:

OpenAPI SHA: f2385add116e3716c8a90a0b68e204deb40f996c, Date: 2024-11-15

Commits

Most Recent Ignore Conditions Applied to This Pull Request | Dependency Name | Ignore Conditions | | --- | --- | | github.com/databricks/databricks-sdk-go | [>= 0.28.a, < 0.29] |
[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/databricks/databricks-sdk-go&package-manager=go_modules&previous-version=0.51.0&new-version=0.52.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
--------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Andrew Nester --- .codegen/_openapi_sha | 2 +- bundle/schema/jsonschema.json | 8 +- cmd/account/budgets/budgets.go | 2 +- .../custom-app-integration.go | 5 +- .../workspace-assignment.go | 2 +- cmd/workspace/apps/apps.go | 8 +- cmd/workspace/apps/overrides.go | 59 ------------ cmd/workspace/credentials/credentials.go | 95 ++++++++++++++----- cmd/workspace/lakeview/lakeview.go | 10 +- .../notification-destinations.go | 5 +- cmd/workspace/repos/repos.go | 6 +- .../token-management/token-management.go | 6 +- go.mod | 2 +- go.sum | 4 +- 14 files changed, 105 insertions(+), 109 deletions(-) delete mode 100644 cmd/workspace/apps/overrides.go diff --git a/.codegen/_openapi_sha b/.codegen/_openapi_sha index 5f4b5086..a2ba58aa 100644 --- a/.codegen/_openapi_sha +++ b/.codegen/_openapi_sha @@ -1 +1 @@ -d25296d2f4aa7bd6195c816fdf82e0f960f775da \ No newline at end of file +f2385add116e3716c8a90a0b68e204deb40f996c \ No newline at end of file diff --git a/bundle/schema/jsonschema.json b/bundle/schema/jsonschema.json index 703daafe..cf003f42 100644 --- a/bundle/schema/jsonschema.json +++ b/bundle/schema/jsonschema.json @@ -817,6 +817,9 @@ "metastore": { "$ref": "#/$defs/string" }, + "notification_destination": { + "$ref": "#/$defs/string" + }, "pipeline": { "$ref": "#/$defs/string" }, @@ -1079,6 +1082,9 @@ "pipelines_development": { "$ref": "#/$defs/bool" }, + "source_linked_deployment": { + "$ref": "#/$defs/bool" + }, "tags": { "$ref": "#/$defs/map/string" }, @@ -2824,7 +2830,7 @@ "anyOf": [ { "type": "object", - "description": "Write-only setting. Specifies the user, service principal or group that the job/pipeline runs as. If not specified, the job/pipeline runs as the user who created the job/pipeline.\n\nExactly one of `user_name`, `service_principal_name`, `group_name` should be specified. If not, an error is thrown.", + "description": "Write-only setting. Specifies the user, service principal or group that the job/pipeline runs as. If not specified, the job/pipeline runs as the user who created the job/pipeline.\n\nEither `user_name` or `service_principal_name` should be specified. If not, an error is thrown.", "properties": { "service_principal_name": { "description": "Application ID of an active service principal. Setting this field requires the `servicePrincipal/user` role.", diff --git a/cmd/account/budgets/budgets.go b/cmd/account/budgets/budgets.go index 87ed41d5..748dc699 100755 --- a/cmd/account/budgets/budgets.go +++ b/cmd/account/budgets/budgets.go @@ -194,7 +194,7 @@ func newGet() *cobra.Command { configuration are specified by ID. Arguments: - BUDGET_ID: The Databricks budget configuration ID.` + BUDGET_ID: The budget configuration ID` cmd.Annotations = make(map[string]string) diff --git a/cmd/account/custom-app-integration/custom-app-integration.go b/cmd/account/custom-app-integration/custom-app-integration.go index 9d16a44d..1eec1018 100755 --- a/cmd/account/custom-app-integration/custom-app-integration.go +++ b/cmd/account/custom-app-integration/custom-app-integration.go @@ -195,7 +195,10 @@ func newGet() *cobra.Command { cmd.Short = `Get OAuth Custom App Integration.` cmd.Long = `Get OAuth Custom App Integration. - Gets the Custom OAuth App Integration for the given integration id.` + Gets the Custom OAuth App Integration for the given integration id. + + Arguments: + INTEGRATION_ID: The OAuth app integration ID.` cmd.Annotations = make(map[string]string) diff --git a/cmd/account/workspace-assignment/workspace-assignment.go b/cmd/account/workspace-assignment/workspace-assignment.go index e09095d3..c5385c92 100755 --- a/cmd/account/workspace-assignment/workspace-assignment.go +++ b/cmd/account/workspace-assignment/workspace-assignment.go @@ -257,7 +257,7 @@ func newUpdate() *cobra.Command { workspace for the specified principal. Arguments: - WORKSPACE_ID: The workspace ID for the account. + WORKSPACE_ID: The workspace ID. PRINCIPAL_ID: The ID of the user, service principal, or group.` cmd.Annotations = make(map[string]string) diff --git a/cmd/workspace/apps/apps.go b/cmd/workspace/apps/apps.go index 514da697..a103ba7a 100755 --- a/cmd/workspace/apps/apps.go +++ b/cmd/workspace/apps/apps.go @@ -942,14 +942,13 @@ func newUpdate() *cobra.Command { // TODO: complex arg: pending_deployment // TODO: array: resources - cmd.Use = "update NAME NAME" + cmd.Use = "update NAME" cmd.Short = `Update an app.` cmd.Long = `Update an app. Updates the app with the supplied name. Arguments: - NAME: The name of the app. NAME: The name of the app. The name must contain only lowercase alphanumeric characters and hyphens. It must be unique within the workspace.` @@ -963,7 +962,7 @@ func newUpdate() *cobra.Command { } return nil } - check := root.ExactArgs(2) + check := root.ExactArgs(1) return check(cmd, args) } @@ -985,9 +984,6 @@ func newUpdate() *cobra.Command { } } updateReq.Name = args[0] - if !cmd.Flags().Changed("json") { - updateReq.App.Name = args[1] - } response, err := w.Apps.Update(ctx, updateReq) if err != nil { diff --git a/cmd/workspace/apps/overrides.go b/cmd/workspace/apps/overrides.go deleted file mode 100644 index debd9f5a..00000000 --- a/cmd/workspace/apps/overrides.go +++ /dev/null @@ -1,59 +0,0 @@ -package apps - -import ( - "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/flags" - "github.com/databricks/databricks-sdk-go/service/apps" - "github.com/spf13/cobra" -) - -// We override apps.Update command beccause currently genkit does not support -// a way to identify that path field (such as name) matches the field in the request body. -// As a result, genkit generates a command with 2 required same fields, update NAME NAME. -// This override should be removed when genkit supports this. -func updateOverride(cmd *cobra.Command, req *apps.UpdateAppRequest) { - cmd.Use = "update NAME" - cmd.Long = `Update an app. - - Updates the app with the supplied name. - - Arguments: - NAME: The name of the app. The name must contain only lowercase alphanumeric - characters and hyphens. It must be unique within the workspace.` - - cmd.Args = func(cmd *cobra.Command, args []string) error { - check := root.ExactArgs(1) - return check(cmd, args) - } - - updateJson := cmd.Flag("json").Value.(*flags.JsonFlag) - cmd.RunE = func(cmd *cobra.Command, args []string) (err error) { - ctx := cmd.Context() - w := root.WorkspaceClient(ctx) - - if cmd.Flags().Changed("json") { - diags := updateJson.Unmarshal(&req.App) - if diags.HasError() { - return diags.Error() - } - if len(diags) > 0 { - err := cmdio.RenderDiagnosticsToErrorOut(ctx, diags) - if err != nil { - return err - } - } - } - - req.Name = args[0] - response, err := w.Apps.Update(ctx, *req) - if err != nil { - return err - } - return cmdio.Render(ctx, response) - } -} - -func init() { - updateOverrides = append(updateOverrides, updateOverride) -} diff --git a/cmd/workspace/credentials/credentials.go b/cmd/workspace/credentials/credentials.go index 869df062..44ee0cf3 100755 --- a/cmd/workspace/credentials/credentials.go +++ b/cmd/workspace/credentials/credentials.go @@ -3,6 +3,8 @@ package credentials import ( + "fmt" + "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/flags" @@ -30,9 +32,6 @@ func New() *cobra.Command { Annotations: map[string]string{ "package": "catalog", }, - - // This service is being previewed; hide from help output. - Hidden: true, } // Add methods @@ -72,21 +71,39 @@ func newCreateCredential() *cobra.Command { // TODO: complex arg: aws_iam_role // TODO: complex arg: azure_managed_identity + // TODO: complex arg: azure_service_principal cmd.Flags().StringVar(&createCredentialReq.Comment, "comment", createCredentialReq.Comment, `Comment associated with the credential.`) - cmd.Flags().StringVar(&createCredentialReq.Name, "name", createCredentialReq.Name, `The credential name.`) - cmd.Flags().Var(&createCredentialReq.Purpose, "purpose", `Indicates the purpose of the credential. Supported values: [SERVICE]`) + // TODO: complex arg: gcp_service_account_key + cmd.Flags().Var(&createCredentialReq.Purpose, "purpose", `Indicates the purpose of the credential. Supported values: [SERVICE, STORAGE]`) + cmd.Flags().BoolVar(&createCredentialReq.ReadOnly, "read-only", createCredentialReq.ReadOnly, `Whether the credential is usable only for read operations.`) cmd.Flags().BoolVar(&createCredentialReq.SkipValidation, "skip-validation", createCredentialReq.SkipValidation, `Optional.`) - cmd.Use = "create-credential" + cmd.Use = "create-credential NAME" cmd.Short = `Create a credential.` cmd.Long = `Create a credential. - Creates a new credential.` + Creates a new credential. The type of credential to be created is determined + by the **purpose** field, which should be either **SERVICE** or **STORAGE**. + + The caller must be a metastore admin or have the metastore privilege + **CREATE_STORAGE_CREDENTIAL** for storage credentials, or + **CREATE_SERVICE_CREDENTIAL** for service credentials. + + Arguments: + NAME: The credential name. The name must be unique among storage and service + credentials within the metastore.` cmd.Annotations = make(map[string]string) cmd.Args = func(cmd *cobra.Command, args []string) error { - check := root.ExactArgs(0) + if cmd.Flags().Changed("json") { + err := root.ExactArgs(0)(cmd, args) + if err != nil { + return fmt.Errorf("when --json flag is specified, no positional arguments are required. Provide 'name' in your JSON input") + } + return nil + } + check := root.ExactArgs(1) return check(cmd, args) } @@ -107,6 +124,9 @@ func newCreateCredential() *cobra.Command { } } } + if !cmd.Flags().Changed("json") { + createCredentialReq.Name = args[0] + } response, err := w.Credentials.CreateCredential(ctx, createCredentialReq) if err != nil { @@ -143,14 +163,14 @@ func newDeleteCredential() *cobra.Command { // TODO: short flags - cmd.Flags().BoolVar(&deleteCredentialReq.Force, "force", deleteCredentialReq.Force, `Force deletion even if there are dependent services.`) + cmd.Flags().BoolVar(&deleteCredentialReq.Force, "force", deleteCredentialReq.Force, `Force an update even if there are dependent services (when purpose is **SERVICE**) or dependent external locations and external tables (when purpose is **STORAGE**).`) cmd.Use = "delete-credential NAME_ARG" cmd.Short = `Delete a credential.` cmd.Long = `Delete a credential. - Deletes a credential from the metastore. The caller must be an owner of the - credential. + Deletes a service or storage credential from the metastore. The caller must be + an owner of the credential. Arguments: NAME_ARG: Name of the credential.` @@ -207,20 +227,29 @@ func newGenerateTemporaryServiceCredential() *cobra.Command { cmd.Flags().Var(&generateTemporaryServiceCredentialJson, "json", `either inline JSON string or @path/to/file.json with request body`) // TODO: complex arg: azure_options - cmd.Flags().StringVar(&generateTemporaryServiceCredentialReq.CredentialName, "credential-name", generateTemporaryServiceCredentialReq.CredentialName, `The name of the service credential used to generate a temporary credential.`) - cmd.Use = "generate-temporary-service-credential" + cmd.Use = "generate-temporary-service-credential CREDENTIAL_NAME" cmd.Short = `Generate a temporary service credential.` cmd.Long = `Generate a temporary service credential. Returns a set of temporary credentials generated using the specified service credential. The caller must be a metastore admin or have the metastore - privilege **ACCESS** on the service credential.` + privilege **ACCESS** on the service credential. + + Arguments: + CREDENTIAL_NAME: The name of the service credential used to generate a temporary credential` cmd.Annotations = make(map[string]string) cmd.Args = func(cmd *cobra.Command, args []string) error { - check := root.ExactArgs(0) + if cmd.Flags().Changed("json") { + err := root.ExactArgs(0)(cmd, args) + if err != nil { + return fmt.Errorf("when --json flag is specified, no positional arguments are required. Provide 'credential_name' in your JSON input") + } + return nil + } + check := root.ExactArgs(1) return check(cmd, args) } @@ -241,6 +270,9 @@ func newGenerateTemporaryServiceCredential() *cobra.Command { } } } + if !cmd.Flags().Changed("json") { + generateTemporaryServiceCredentialReq.CredentialName = args[0] + } response, err := w.Credentials.GenerateTemporaryServiceCredential(ctx, generateTemporaryServiceCredentialReq) if err != nil { @@ -281,8 +313,9 @@ func newGetCredential() *cobra.Command { cmd.Short = `Get a credential.` cmd.Long = `Get a credential. - Gets a credential from the metastore. The caller must be a metastore admin, - the owner of the credential, or have any permission on the credential. + Gets a service or storage credential from the metastore. The caller must be a + metastore admin, the owner of the credential, or have any permission on the + credential. Arguments: NAME_ARG: Name of the credential.` @@ -338,7 +371,7 @@ func newListCredentials() *cobra.Command { cmd.Flags().IntVar(&listCredentialsReq.MaxResults, "max-results", listCredentialsReq.MaxResults, `Maximum number of credentials to return.`) cmd.Flags().StringVar(&listCredentialsReq.PageToken, "page-token", listCredentialsReq.PageToken, `Opaque token to retrieve the next page of results.`) - cmd.Flags().Var(&listCredentialsReq.Purpose, "purpose", `Return only credentials for the specified purpose. Supported values: [SERVICE]`) + cmd.Flags().Var(&listCredentialsReq.Purpose, "purpose", `Return only credentials for the specified purpose. Supported values: [SERVICE, STORAGE]`) cmd.Use = "list-credentials" cmd.Short = `List credentials.` @@ -399,18 +432,20 @@ func newUpdateCredential() *cobra.Command { // TODO: complex arg: aws_iam_role // TODO: complex arg: azure_managed_identity + // TODO: complex arg: azure_service_principal cmd.Flags().StringVar(&updateCredentialReq.Comment, "comment", updateCredentialReq.Comment, `Comment associated with the credential.`) - cmd.Flags().BoolVar(&updateCredentialReq.Force, "force", updateCredentialReq.Force, `Force update even if there are dependent services.`) + cmd.Flags().BoolVar(&updateCredentialReq.Force, "force", updateCredentialReq.Force, `Force an update even if there are dependent services (when purpose is **SERVICE**) or dependent external locations and external tables (when purpose is **STORAGE**).`) cmd.Flags().Var(&updateCredentialReq.IsolationMode, "isolation-mode", `Whether the current securable is accessible from all workspaces or a specific set of workspaces. Supported values: [ISOLATION_MODE_ISOLATED, ISOLATION_MODE_OPEN]`) cmd.Flags().StringVar(&updateCredentialReq.NewName, "new-name", updateCredentialReq.NewName, `New name of credential.`) cmd.Flags().StringVar(&updateCredentialReq.Owner, "owner", updateCredentialReq.Owner, `Username of current owner of credential.`) + cmd.Flags().BoolVar(&updateCredentialReq.ReadOnly, "read-only", updateCredentialReq.ReadOnly, `Whether the credential is usable only for read operations.`) cmd.Flags().BoolVar(&updateCredentialReq.SkipValidation, "skip-validation", updateCredentialReq.SkipValidation, `Supply true to this argument to skip validation of the updated credential.`) cmd.Use = "update-credential NAME_ARG" cmd.Short = `Update a credential.` cmd.Long = `Update a credential. - Updates a credential on the metastore. + Updates a service or storage credential on the metastore. The caller must be the owner of the credential or a metastore admin or have the MANAGE permission. If the caller is a metastore admin, only the @@ -485,7 +520,10 @@ func newValidateCredential() *cobra.Command { // TODO: complex arg: aws_iam_role // TODO: complex arg: azure_managed_identity cmd.Flags().StringVar(&validateCredentialReq.CredentialName, "credential-name", validateCredentialReq.CredentialName, `Required.`) - cmd.Flags().Var(&validateCredentialReq.Purpose, "purpose", `The purpose of the credential. Supported values: [SERVICE]`) + cmd.Flags().StringVar(&validateCredentialReq.ExternalLocationName, "external-location-name", validateCredentialReq.ExternalLocationName, `The name of an existing external location to validate.`) + cmd.Flags().Var(&validateCredentialReq.Purpose, "purpose", `The purpose of the credential. Supported values: [SERVICE, STORAGE]`) + cmd.Flags().BoolVar(&validateCredentialReq.ReadOnly, "read-only", validateCredentialReq.ReadOnly, `Whether the credential is only usable for read operations.`) + cmd.Flags().StringVar(&validateCredentialReq.Url, "url", validateCredentialReq.Url, `The external location url to validate.`) cmd.Use = "validate-credential" cmd.Short = `Validate a credential.` @@ -493,10 +531,19 @@ func newValidateCredential() *cobra.Command { Validates a credential. - Either the __credential_name__ or the cloud-specific credential must be - provided. + For service credentials (purpose is **SERVICE**), either the + __credential_name__ or the cloud-specific credential must be provided. - The caller must be a metastore admin or the credential owner.` + For storage credentials (purpose is **STORAGE**), at least one of + __external_location_name__ and __url__ need to be provided. If only one of + them is provided, it will be used for validation. And if both are provided, + the __url__ will be used for validation, and __external_location_name__ will + be ignored when checking overlapping urls. Either the __credential_name__ or + the cloud-specific credential must be provided. + + The caller must be a metastore admin or the credential owner or have the + required permission on the metastore and the credential (e.g., + **CREATE_EXTERNAL_LOCATION** when purpose is **STORAGE**).` cmd.Annotations = make(map[string]string) diff --git a/cmd/workspace/lakeview/lakeview.go b/cmd/workspace/lakeview/lakeview.go index 239c72b6..35c3bdf4 100755 --- a/cmd/workspace/lakeview/lakeview.go +++ b/cmd/workspace/lakeview/lakeview.go @@ -503,7 +503,7 @@ func newGetPublished() *cobra.Command { Get the current published dashboard. Arguments: - DASHBOARD_ID: UUID identifying the dashboard to be published.` + DASHBOARD_ID: UUID identifying the published dashboard.` cmd.Annotations = make(map[string]string) @@ -737,7 +737,7 @@ func newListSchedules() *cobra.Command { cmd.Long = `List dashboard schedules. Arguments: - DASHBOARD_ID: UUID identifying the dashboard to which the schedule belongs.` + DASHBOARD_ID: UUID identifying the dashboard to which the schedules belongs.` // This command is being previewed; hide from help output. cmd.Hidden = true @@ -795,8 +795,8 @@ func newListSubscriptions() *cobra.Command { cmd.Long = `List schedule subscriptions. Arguments: - DASHBOARD_ID: UUID identifying the dashboard to which the subscription belongs. - SCHEDULE_ID: UUID identifying the schedule to which the subscription belongs.` + DASHBOARD_ID: UUID identifying the dashboard which the subscriptions belongs. + SCHEDULE_ID: UUID identifying the schedule which the subscriptions belongs.` // This command is being previewed; hide from help output. cmd.Hidden = true @@ -1072,7 +1072,7 @@ func newUnpublish() *cobra.Command { Unpublish the dashboard. Arguments: - DASHBOARD_ID: UUID identifying the dashboard to be published.` + DASHBOARD_ID: UUID identifying the published dashboard.` cmd.Annotations = make(map[string]string) diff --git a/cmd/workspace/notification-destinations/notification-destinations.go b/cmd/workspace/notification-destinations/notification-destinations.go index 47076587..b06652c7 100755 --- a/cmd/workspace/notification-destinations/notification-destinations.go +++ b/cmd/workspace/notification-destinations/notification-destinations.go @@ -304,7 +304,10 @@ func newUpdate() *cobra.Command { cmd.Long = `Update a notification destination. Updates a notification destination. Requires workspace admin permissions. At - least one field is required in the request body.` + least one field is required in the request body. + + Arguments: + ID: UUID identifying notification destination.` cmd.Annotations = make(map[string]string) diff --git a/cmd/workspace/repos/repos.go b/cmd/workspace/repos/repos.go index 7dcb1353..79947265 100755 --- a/cmd/workspace/repos/repos.go +++ b/cmd/workspace/repos/repos.go @@ -171,7 +171,7 @@ func newDelete() *cobra.Command { Deletes the specified repo. Arguments: - REPO_ID: ID of the Git folder (repo) object in the workspace.` + REPO_ID: The ID for the corresponding repo to delete.` cmd.Annotations = make(map[string]string) @@ -188,14 +188,14 @@ func newDelete() *cobra.Command { if err != nil { return fmt.Errorf("failed to load names for Repos drop-down. Please manually specify required arguments. Original error: %w", err) } - id, err := cmdio.Select(ctx, names, "ID of the Git folder (repo) object in the workspace") + id, err := cmdio.Select(ctx, names, "The ID for the corresponding repo to delete") if err != nil { return err } args = append(args, id) } if len(args) != 1 { - return fmt.Errorf("expected to have id of the git folder (repo) object in the workspace") + return fmt.Errorf("expected to have the id for the corresponding repo to delete") } _, err = fmt.Sscan(args[0], &deleteReq.RepoId) if err != nil { diff --git a/cmd/workspace/token-management/token-management.go b/cmd/workspace/token-management/token-management.go index c8d57fd6..fcc70c12 100755 --- a/cmd/workspace/token-management/token-management.go +++ b/cmd/workspace/token-management/token-management.go @@ -169,7 +169,7 @@ func newDelete() *cobra.Command { Deletes a token, specified by its ID. Arguments: - TOKEN_ID: The ID of the token to get.` + TOKEN_ID: The ID of the token to revoke.` cmd.Annotations = make(map[string]string) @@ -186,14 +186,14 @@ func newDelete() *cobra.Command { if err != nil { return fmt.Errorf("failed to load names for Token Management drop-down. Please manually specify required arguments. Original error: %w", err) } - id, err := cmdio.Select(ctx, names, "The ID of the token to get") + id, err := cmdio.Select(ctx, names, "The ID of the token to revoke") if err != nil { return err } args = append(args, id) } if len(args) != 1 { - return fmt.Errorf("expected to have the id of the token to get") + return fmt.Errorf("expected to have the id of the token to revoke") } deleteReq.TokenId = args[0] diff --git a/go.mod b/go.mod index 35331313..7141ed76 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ toolchain go1.23.2 require ( github.com/Masterminds/semver/v3 v3.3.1 // MIT github.com/briandowns/spinner v1.23.1 // Apache 2.0 - github.com/databricks/databricks-sdk-go v0.51.0 // Apache 2.0 + github.com/databricks/databricks-sdk-go v0.52.0 // Apache 2.0 github.com/fatih/color v1.18.0 // MIT github.com/ghodss/yaml v1.0.0 // MIT + NOTICE github.com/google/uuid v1.6.0 // BSD-3-Clause diff --git a/go.sum b/go.sum index 312a4d93..5d2c53a3 100644 --- a/go.sum +++ b/go.sum @@ -32,8 +32,8 @@ github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGX github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/cyphar/filepath-securejoin v0.2.4 h1:Ugdm7cg7i6ZK6x3xDF1oEu1nfkyfH53EtKeQYTC3kyg= github.com/cyphar/filepath-securejoin v0.2.4/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4= -github.com/databricks/databricks-sdk-go v0.51.0 h1:tcvB9TID3oUl0O8npccB5c+33tarBiYMBFbq4U4AB6M= -github.com/databricks/databricks-sdk-go v0.51.0/go.mod h1:ds+zbv5mlQG7nFEU5ojLtgN/u0/9YzZmKQES/CfedzU= +github.com/databricks/databricks-sdk-go v0.52.0 h1:WKcj0F+pdx0gjI5xMicjYC4O43S2q5nyTpaGGMFmgHw= +github.com/databricks/databricks-sdk-go v0.52.0/go.mod h1:ds+zbv5mlQG7nFEU5ojLtgN/u0/9YzZmKQES/CfedzU= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=