From 6f5e5e6e4b8ae6601171080d7e5f39570303aa26 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 9 Apr 2024 14:48:00 +0200 Subject: [PATCH 1/7] Added validate mutator to surface additional bundle warnings --- bundle/config/validate/files_to_sync.go | 52 ++++++++++++++ .../validate/job_cluster_key_defined.go | 66 ++++++++++++++++++ bundle/config/validate/validate.go | 42 ++++++++++++ bundle/parallel.go | 37 ++++++++++ bundle/parallel_test.go | 67 +++++++++++++++++++ bundle/tests/job_cluster_key/databricks.yml | 27 ++++++++ bundle/tests/job_cluster_key_test.go | 28 ++++++++ cmd/bundle/validate.go | 2 + 8 files changed, 321 insertions(+) create mode 100644 bundle/config/validate/files_to_sync.go create mode 100644 bundle/config/validate/job_cluster_key_defined.go create mode 100644 bundle/config/validate/validate.go create mode 100644 bundle/parallel.go create mode 100644 bundle/parallel_test.go create mode 100644 bundle/tests/job_cluster_key/databricks.yml create mode 100644 bundle/tests/job_cluster_key_test.go diff --git a/bundle/config/validate/files_to_sync.go b/bundle/config/validate/files_to_sync.go new file mode 100644 index 0000000000..aa7785d9ad --- /dev/null +++ b/bundle/config/validate/files_to_sync.go @@ -0,0 +1,52 @@ +package validate + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/deploy/files" + "github.com/databricks/cli/libs/diag" +) + +func FilesToSync() bundle.Mutator { + return &filesToSync{} +} + +type filesToSync struct { +} + +func (v *filesToSync) Name() string { + return "validate:files_to_sync" +} + +func (v *filesToSync) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + sync, err := files.GetSync(ctx, b) + if err != nil { + return diag.FromErr(err) + } + + diags := diag.Diagnostics{} + fl, err := sync.GetFileList(ctx) + if err != nil { + return diag.FromErr(err) + } + + if len(fl) == 0 { + if len(b.Config.Sync.Exclude) == 0 { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "There are no files to sync, please check your .gitignore", + }) + } else { + loc := location{path: "sync.exclude", b: b} + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "There are no files to sync, please check your .gitignore and sync.exclude configuration", + Location: loc.Location(), + Path: loc.Path(), + }) + } + } + + return diags +} diff --git a/bundle/config/validate/job_cluster_key_defined.go b/bundle/config/validate/job_cluster_key_defined.go new file mode 100644 index 0000000000..ebaa07cd6b --- /dev/null +++ b/bundle/config/validate/job_cluster_key_defined.go @@ -0,0 +1,66 @@ +package validate + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +func JobClusterKeyDefined() bundle.Mutator { + return &jobClusterKeyDefined{} +} + +type jobClusterKeyDefined struct { +} + +func (v *jobClusterKeyDefined) Name() string { + return "validate:job_cluster_key_defined" +} + +func (v *jobClusterKeyDefined) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + // Collect all job_cluster_key references from defined tasks + jobClusterKeys := make(map[string]location) + for i, job := range b.Config.Resources.Jobs { + for j, task := range job.Tasks { + if task.JobClusterKey != "" { + jobClusterKeys[task.JobClusterKey] = location{ + path: fmt.Sprintf("resources.jobs.%s.tasks[%d].job_cluster_key", i, j), + b: b, + } + } + } + } + + if len(jobClusterKeys) == 0 { + return nil + } + + diags := diag.Diagnostics{} + + // Check if all job_cluster_keys are defined + for key, loc := range jobClusterKeys { + if !isJobClusterKeyDefined(key, b) { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("job_cluster_key %s is not defined", key), + Location: loc.Location(), + Path: loc.Path(), + }) + } + } + + return diags +} + +func isJobClusterKeyDefined(key string, b *bundle.Bundle) bool { + for _, job := range b.Config.Resources.Jobs { + for _, cluster := range job.JobClusters { + if cluster.JobClusterKey == key { + return true + } + } + } + return false +} diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go new file mode 100644 index 0000000000..4f94663845 --- /dev/null +++ b/bundle/config/validate/validate.go @@ -0,0 +1,42 @@ +package validate + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +type validate struct { +} + +type location struct { + path string + b *bundle.Bundle +} + +func (l location) Location() dyn.Location { + return l.b.Config.GetLocation(l.path) +} + +func (l location) Path() dyn.Path { + return dyn.MustPathFromString(l.path) +} + +// Apply implements bundle.Mutator. +func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + return bundle.Apply(ctx, b, bundle.Parallel( + JobClusterKeyDefined(), + FilesToSync(), + )) +} + +// Name implements bundle.Mutator. +func (v *validate) Name() string { + return "validate" +} + +func Validate() bundle.Mutator { + return &validate{} +} diff --git a/bundle/parallel.go b/bundle/parallel.go new file mode 100644 index 0000000000..df4f12a13d --- /dev/null +++ b/bundle/parallel.go @@ -0,0 +1,37 @@ +package bundle + +import ( + "context" + "sync" + + "github.com/databricks/cli/libs/diag" +) + +type parallel struct { + mutators []Mutator +} + +func (m *parallel) Name() string { + return "parallel" +} + +func (m *parallel) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { + var wg sync.WaitGroup + diags := diag.Diagnostics{} + wg.Add(len(m.mutators)) + for _, mutator := range m.mutators { + go func(mutator Mutator) { + defer wg.Done() + diags = diags.Extend(mutator.Apply(ctx, b)) + }(mutator) + } + wg.Wait() + return diags +} + +// Parallel runs the given mutators in parallel. +func Parallel(mutators ...Mutator) Mutator { + return ¶llel{ + mutators: mutators, + } +} diff --git a/bundle/parallel_test.go b/bundle/parallel_test.go new file mode 100644 index 0000000000..0e14bcb491 --- /dev/null +++ b/bundle/parallel_test.go @@ -0,0 +1,67 @@ +package bundle + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/diag" + "github.com/stretchr/testify/require" +) + +type addToContainer struct { + container *[]int + err bool +} + +func (m *addToContainer) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { + if m.err { + return diag.Errorf("error") + } + + c := *m.container + c = append(c, 1) + *m.container = c + return nil +} + +func (m *addToContainer) Name() string { + return "addToContainer" +} + +func TestParallelMutatorWork(t *testing.T) { + b := &Bundle{ + Config: config.Root{}, + } + + container := []int{} + m1 := &addToContainer{container: &container} + m2 := &addToContainer{container: &container} + m3 := &addToContainer{container: &container} + + m := Parallel(m1, m2, m3) + + // Apply the mutator + diags := m.Apply(context.Background(), b) + require.Empty(t, diags) + require.Len(t, container, 3) +} + +func TestParallelMutatorWorkWithErrors(t *testing.T) { + b := &Bundle{ + Config: config.Root{}, + } + + container := []int{} + m1 := &addToContainer{container: &container} + m2 := &addToContainer{container: &container, err: true} + m3 := &addToContainer{container: &container} + + m := Parallel(m1, m2, m3) + + // Apply the mutator + diags := m.Apply(context.Background(), b) + require.Len(t, diags, 1) + require.Equal(t, "error", diags[0].Summary) + require.Len(t, container, 2) +} diff --git a/bundle/tests/job_cluster_key/databricks.yml b/bundle/tests/job_cluster_key/databricks.yml new file mode 100644 index 0000000000..bd863db3e5 --- /dev/null +++ b/bundle/tests/job_cluster_key/databricks.yml @@ -0,0 +1,27 @@ +bundle: + name: job_cluster_key + +workspace: + host: https://acme.cloud.databricks.com/ + +targets: + default: + resources: + jobs: + foo: + name: job + tasks: + - task_key: test + job_cluster_key: key + development: + resources: + jobs: + foo: + job_clusters: + - job_cluster_key: key + new_cluster: + node_type_id: i3.xlarge + num_workers: 1 + tasks: + - task_key: test + job_cluster_key: key diff --git a/bundle/tests/job_cluster_key_test.go b/bundle/tests/job_cluster_key_test.go new file mode 100644 index 0000000000..869a83fdec --- /dev/null +++ b/bundle/tests/job_cluster_key_test.go @@ -0,0 +1,28 @@ +package config_tests + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/diag" + "github.com/stretchr/testify/require" +) + +func TestJobClusterKeyNotDefinedTest(t *testing.T) { + b := loadTarget(t, "./job_cluster_key", "default") + + diags := bundle.Apply(context.Background(), b, validate.JobClusterKeyDefined()) + require.Len(t, diags, 1) + require.NoError(t, diags.Error()) + require.Equal(t, diags[0].Severity, diag.Warning) + require.Equal(t, diags[0].Summary, "job_cluster_key key is not defined") +} + +func TestJobClusterKeyDefinedTest(t *testing.T) { + b := loadTarget(t, "./job_cluster_key", "development") + + diags := bundle.Apply(context.Background(), b, validate.JobClusterKeyDefined()) + require.Len(t, diags, 0) +} diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 4a04db4099..0472df4795 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -8,6 +8,7 @@ import ( "text/template" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" @@ -140,6 +141,7 @@ func newValidateCommand() *cobra.Command { } diags = diags.Extend(bundle.Apply(ctx, b, phases.Initialize())) + diags = diags.Extend(bundle.Apply(ctx, b, validate.Validate())) if err := diags.Error(); err != nil { return err } From 5cc14f5c04f64fbe1b90e4bc37170414158c740a Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 14:14:29 +0200 Subject: [PATCH 2/7] read only mutator --- bundle/bundle_read_only.go | 73 +++++++++++++++++ bundle/config/root_read_only.go | 68 ++++++++++++++++ bundle/config/validate/files_to_sync.go | 40 +++++----- .../validate/job_cluster_key_defined.go | 47 +++++------ bundle/config/validate/validate.go | 7 +- .../config/validate/validate_sync_patterns.go | 79 +++++++++++++++++++ bundle/deploy/files/delete.go | 2 +- bundle/deploy/files/sync.go | 24 +++--- bundle/deploy/files/upload.go | 2 +- bundle/deploy/state_pull.go | 2 +- bundle/deploy/state_pull_test.go | 4 +- bundle/deploy/state_update.go | 2 +- bundle/mutator_read_only.go | 29 +++++++ bundle/parallel.go | 17 ++-- bundle/parallel_test.go | 26 +++--- bundle/tests/job_cluster_key_test.go | 4 +- .../sync_include_exclude_no_matches_test.go | 39 +++++++++ cmd/bundle/sync.go | 2 +- cmd/sync/sync.go | 2 +- 19 files changed, 383 insertions(+), 86 deletions(-) create mode 100644 bundle/bundle_read_only.go create mode 100644 bundle/config/root_read_only.go create mode 100644 bundle/config/validate/validate_sync_patterns.go create mode 100644 bundle/mutator_read_only.go create mode 100644 bundle/tests/sync_include_exclude_no_matches_test.go diff --git a/bundle/bundle_read_only.go b/bundle/bundle_read_only.go new file mode 100644 index 0000000000..a97b70d8d6 --- /dev/null +++ b/bundle/bundle_read_only.go @@ -0,0 +1,73 @@ +package bundle + +import ( + "context" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/metadata" + "github.com/databricks/cli/libs/locker" + "github.com/databricks/cli/libs/tags" + "github.com/databricks/cli/libs/terraform" + "github.com/databricks/databricks-sdk-go" + "github.com/hashicorp/terraform-exec/tfexec" +) + +type ReadOnlyBundle struct { + b *Bundle +} + +func ReadOnly(b *Bundle) ReadOnlyBundle { + return ReadOnlyBundle{b: b} +} + +func (r ReadOnlyBundle) Config() config.ReadOnlyConfig { + return config.ReadOnly(r.b.Config) +} + +func (r ReadOnlyBundle) AutoApprove() bool { + return r.b.AutoApprove +} + +func (r ReadOnlyBundle) Locker() *locker.Locker { + return r.b.Locker +} + +func (r ReadOnlyBundle) Metadata() metadata.Metadata { + return r.b.Metadata +} + +func (r ReadOnlyBundle) Plan() *terraform.Plan { + return r.b.Plan +} + +func (r ReadOnlyBundle) RootPath() string { + return r.b.RootPath +} + +func (r ReadOnlyBundle) Tagging() tags.Cloud { + return r.b.Tagging +} + +func (r ReadOnlyBundle) Terraform() *tfexec.Terraform { + return r.b.Terraform +} + +func (r ReadOnlyBundle) WorkspaceClient() *databricks.WorkspaceClient { + return r.b.WorkspaceClient() +} + +func (r ReadOnlyBundle) CacheDir(ctx context.Context, paths ...string) (string, error) { + return r.b.CacheDir(ctx, paths...) +} + +func (r ReadOnlyBundle) InternalDir(ctx context.Context) (string, error) { + return r.b.InternalDir(ctx) +} + +func (r ReadOnlyBundle) SetWorkpaceClient(w *databricks.WorkspaceClient) { + r.b.SetWorkpaceClient(w) +} + +func (r ReadOnlyBundle) GetSyncIncludePatterns(ctx context.Context) ([]string, error) { + return r.b.GetSyncIncludePatterns(ctx) +} diff --git a/bundle/config/root_read_only.go b/bundle/config/root_read_only.go new file mode 100644 index 0000000000..c2eaf1658e --- /dev/null +++ b/bundle/config/root_read_only.go @@ -0,0 +1,68 @@ +package config + +import ( + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/service/jobs" +) + +type ReadOnlyConfig struct { + r Root +} + +func ReadOnly(r Root) ReadOnlyConfig { + return ReadOnlyConfig{r: r} +} + +func (r ReadOnlyConfig) Artifacts() Artifacts { + return r.r.Artifacts +} + +func (r ReadOnlyConfig) Bundle() Bundle { + return r.r.Bundle +} + +func (r ReadOnlyConfig) Environments() map[string]*Target { + return r.r.Environments +} + +func (r ReadOnlyConfig) Experimental() *Experimental { + return r.r.Experimental +} + +func (r ReadOnlyConfig) Include() []string { + return r.r.Include +} + +func (r ReadOnlyConfig) Permissions() []resources.Permission { + return r.r.Permissions +} + +func (r ReadOnlyConfig) Resources() Resources { + return r.r.Resources +} + +func (r ReadOnlyConfig) Targets() map[string]*Target { + return r.r.Targets +} + +func (r ReadOnlyConfig) RunAs() *jobs.JobRunAs { + return r.r.RunAs +} + +func (r ReadOnlyConfig) Sync() Sync { + return r.r.Sync +} + +func (r ReadOnlyConfig) Variables() map[string]*variable.Variable { + return r.r.Variables +} + +func (r ReadOnlyConfig) Workspace() Workspace { + return r.r.Workspace +} + +func (r ReadOnlyConfig) GetLocation(path string) dyn.Location { + return r.r.GetLocation(path) +} diff --git a/bundle/config/validate/files_to_sync.go b/bundle/config/validate/files_to_sync.go index aa7785d9ad..4e6cf75745 100644 --- a/bundle/config/validate/files_to_sync.go +++ b/bundle/config/validate/files_to_sync.go @@ -8,7 +8,7 @@ import ( "github.com/databricks/cli/libs/diag" ) -func FilesToSync() bundle.Mutator { +func FilesToSync() bundle.ReadOnlyMutator { return &filesToSync{} } @@ -19,33 +19,35 @@ func (v *filesToSync) Name() string { return "validate:files_to_sync" } -func (v *filesToSync) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - sync, err := files.GetSync(ctx, b) +func (v *filesToSync) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + sync, err := files.GetSync(ctx, rb) if err != nil { return diag.FromErr(err) } - diags := diag.Diagnostics{} fl, err := sync.GetFileList(ctx) if err != nil { return diag.FromErr(err) } - if len(fl) == 0 { - if len(b.Config.Sync.Exclude) == 0 { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: "There are no files to sync, please check your .gitignore", - }) - } else { - loc := location{path: "sync.exclude", b: b} - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: "There are no files to sync, please check your .gitignore and sync.exclude configuration", - Location: loc.Location(), - Path: loc.Path(), - }) - } + if len(fl) != 0 { + return nil + } + + diags := diag.Diagnostics{} + if len(rb.Config().Sync().Exclude) == 0 { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "There are no files to sync, please check your .gitignore", + }) + } else { + loc := location{path: "sync.exclude", rb: rb} + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "There are no files to sync, please check your .gitignore and sync.exclude configuration", + Location: loc.Location(), + Path: loc.Path(), + }) } return diags diff --git a/bundle/config/validate/job_cluster_key_defined.go b/bundle/config/validate/job_cluster_key_defined.go index ebaa07cd6b..69047ba483 100644 --- a/bundle/config/validate/job_cluster_key_defined.go +++ b/bundle/config/validate/job_cluster_key_defined.go @@ -8,7 +8,7 @@ import ( "github.com/databricks/cli/libs/diag" ) -func JobClusterKeyDefined() bundle.Mutator { +func JobClusterKeyDefined() bundle.ReadOnlyMutator { return &jobClusterKeyDefined{} } @@ -19,43 +19,38 @@ func (v *jobClusterKeyDefined) Name() string { return "validate:job_cluster_key_defined" } -func (v *jobClusterKeyDefined) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Collect all job_cluster_key references from defined tasks - jobClusterKeys := make(map[string]location) - for i, job := range b.Config.Resources.Jobs { - for j, task := range job.Tasks { +func (v *jobClusterKeyDefined) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + for k, job := range rb.Config().Resources().Jobs { + jobClusterKeys := make(map[string]location) + for index, task := range job.Tasks { if task.JobClusterKey != "" { jobClusterKeys[task.JobClusterKey] = location{ - path: fmt.Sprintf("resources.jobs.%s.tasks[%d].job_cluster_key", i, j), - b: b, + path: fmt.Sprintf("resources.jobs.%s.tasks[%d].job_cluster_key", k, index), + rb: rb, } } } - } - - if len(jobClusterKeys) == 0 { - return nil - } - - diags := diag.Diagnostics{} - // Check if all job_cluster_keys are defined - for key, loc := range jobClusterKeys { - if !isJobClusterKeyDefined(key, b) { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("job_cluster_key %s is not defined", key), - Location: loc.Location(), - Path: loc.Path(), - }) + // Check if all job_cluster_keys are defined + for key, loc := range jobClusterKeys { + if !isJobClusterKeyDefined(key, rb) { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("job_cluster_key %s is not defined", key), + Location: loc.Location(), + Path: loc.Path(), + }) + } } } return diags } -func isJobClusterKeyDefined(key string, b *bundle.Bundle) bool { - for _, job := range b.Config.Resources.Jobs { +func isJobClusterKeyDefined(key string, rb bundle.ReadOnlyBundle) bool { + for _, job := range rb.Config().Resources().Jobs { for _, cluster := range job.JobClusters { if cluster.JobClusterKey == key { return true diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index 4f94663845..af7e984a11 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -13,11 +13,11 @@ type validate struct { type location struct { path string - b *bundle.Bundle + rb bundle.ReadOnlyBundle } func (l location) Location() dyn.Location { - return l.b.Config.GetLocation(l.path) + return l.rb.Config().GetLocation(l.path) } func (l location) Path() dyn.Path { @@ -26,9 +26,10 @@ func (l location) Path() dyn.Path { // Apply implements bundle.Mutator. func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - return bundle.Apply(ctx, b, bundle.Parallel( + return bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), bundle.Parallel( JobClusterKeyDefined(), FilesToSync(), + ValidateSyncPatterns(), )) } diff --git a/bundle/config/validate/validate_sync_patterns.go b/bundle/config/validate/validate_sync_patterns.go new file mode 100644 index 0000000000..ad840ab531 --- /dev/null +++ b/bundle/config/validate/validate_sync_patterns.go @@ -0,0 +1,79 @@ +package validate + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/fileset" +) + +func ValidateSyncPatterns() bundle.ReadOnlyMutator { + return &validateSyncPatterns{} +} + +type validateSyncPatterns struct { +} + +func (v *validateSyncPatterns) Name() string { + return "validate:validate_sync_patterns" +} + +func (v *validateSyncPatterns) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + sync := rb.Config().Sync() + if len(sync.Exclude) == 0 && len(sync.Include) == 0 { + return nil + } + + diags := diag.Diagnostics{} + if len(sync.Exclude) != 0 { + for i, exclude := range sync.Exclude { + fs, err := fileset.NewGlobSet(rb.RootPath(), []string{exclude}) + if err != nil { + return diag.FromErr(err) + } + + all, err := fs.All() + if err != nil { + return diag.FromErr(err) + } + + if len(all) == 0 { + loc := location{path: fmt.Sprintf("sync.exclude[%d]", i), rb: rb} + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("Exclude pattern %s does not match any files", exclude), + Location: loc.Location(), + Path: loc.Path(), + }) + } + } + } + + if len(sync.Include) != 0 { + for i, include := range sync.Include { + fs, err := fileset.NewGlobSet(rb.RootPath(), []string{include}) + if err != nil { + return diag.FromErr(err) + } + + all, err := fs.All() + if err != nil { + return diag.FromErr(err) + } + + if len(all) == 0 { + loc := location{path: fmt.Sprintf("sync.include[%d]", i), rb: rb} + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("Include pattern %s does not match any files", include), + Location: loc.Location(), + Path: loc.Path(), + }) + } + } + } + + return diags +} diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 9367e2a624..46c5544635 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -46,7 +46,7 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { } // Clean up sync snapshot file - sync, err := GetSync(ctx, b) + sync, err := GetSync(ctx, bundle.ReadOnly(b)) if err != nil { return diag.FromErr(err) } diff --git a/bundle/deploy/files/sync.go b/bundle/deploy/files/sync.go index e8c54c6332..5f84bdee19 100644 --- a/bundle/deploy/files/sync.go +++ b/bundle/deploy/files/sync.go @@ -8,40 +8,40 @@ import ( "github.com/databricks/cli/libs/sync" ) -func GetSync(ctx context.Context, b *bundle.Bundle) (*sync.Sync, error) { - opts, err := GetSyncOptions(ctx, b) +func GetSync(ctx context.Context, rb bundle.ReadOnlyBundle) (*sync.Sync, error) { + opts, err := GetSyncOptions(ctx, rb) if err != nil { return nil, fmt.Errorf("cannot get sync options: %w", err) } return sync.New(ctx, *opts) } -func GetSyncOptions(ctx context.Context, b *bundle.Bundle) (*sync.SyncOptions, error) { - cacheDir, err := b.CacheDir(ctx) +func GetSyncOptions(ctx context.Context, rb bundle.ReadOnlyBundle) (*sync.SyncOptions, error) { + cacheDir, err := rb.CacheDir(ctx) if err != nil { return nil, fmt.Errorf("cannot get bundle cache directory: %w", err) } - includes, err := b.GetSyncIncludePatterns(ctx) + includes, err := rb.GetSyncIncludePatterns(ctx) if err != nil { return nil, fmt.Errorf("cannot get list of sync includes: %w", err) } opts := &sync.SyncOptions{ - LocalPath: b.RootPath, - RemotePath: b.Config.Workspace.FilePath, + LocalPath: rb.RootPath(), + RemotePath: rb.Config().Workspace().FilePath, Include: includes, - Exclude: b.Config.Sync.Exclude, - Host: b.WorkspaceClient().Config.Host, + Exclude: rb.Config().Sync().Exclude, + Host: rb.WorkspaceClient().Config.Host, Full: false, SnapshotBasePath: cacheDir, - WorkspaceClient: b.WorkspaceClient(), + WorkspaceClient: rb.WorkspaceClient(), } - if b.Config.Workspace.CurrentUser != nil { - opts.CurrentUser = b.Config.Workspace.CurrentUser.User + if rb.Config().Workspace().CurrentUser != nil { + opts.CurrentUser = rb.Config().Workspace().CurrentUser.User } return opts, nil diff --git a/bundle/deploy/files/upload.go b/bundle/deploy/files/upload.go index 58cb3c0f03..fa20ed4eaf 100644 --- a/bundle/deploy/files/upload.go +++ b/bundle/deploy/files/upload.go @@ -18,7 +18,7 @@ func (m *upload) Name() string { func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { cmdio.LogString(ctx, fmt.Sprintf("Uploading bundle files to %s...", b.Config.Workspace.FilePath)) - sync, err := GetSync(ctx, b) + sync, err := GetSync(ctx, bundle.ReadOnly(b)) if err != nil { return diag.FromErr(err) } diff --git a/bundle/deploy/state_pull.go b/bundle/deploy/state_pull.go index bae457ea09..57b38ec6c4 100644 --- a/bundle/deploy/state_pull.go +++ b/bundle/deploy/state_pull.go @@ -79,7 +79,7 @@ func (s *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic } // Create a new snapshot based on the deployment state file. - opts, err := files.GetSyncOptions(ctx, b) + opts, err := files.GetSyncOptions(ctx, bundle.ReadOnly(b)) if err != nil { return diag.FromErr(err) } diff --git a/bundle/deploy/state_pull_test.go b/bundle/deploy/state_pull_test.go index 80acb254f4..ca48347313 100644 --- a/bundle/deploy/state_pull_test.go +++ b/bundle/deploy/state_pull_test.go @@ -85,7 +85,7 @@ func testStatePull(t *testing.T, opts statePullOpts) { } if opts.withExistingSnapshot { - opts, err := files.GetSyncOptions(ctx, b) + opts, err := files.GetSyncOptions(ctx, bundle.ReadOnly(b)) require.NoError(t, err) snapshotPath, err := sync.SnapshotPath(opts) @@ -127,7 +127,7 @@ func testStatePull(t *testing.T, opts statePullOpts) { } if opts.expects.snapshotState != nil { - syncOpts, err := files.GetSyncOptions(ctx, b) + syncOpts, err := files.GetSyncOptions(ctx, bundle.ReadOnly(b)) require.NoError(t, err) snapshotPath, err := sync.SnapshotPath(syncOpts) diff --git a/bundle/deploy/state_update.go b/bundle/deploy/state_update.go index cf2e9ac9ee..885e47a7aa 100644 --- a/bundle/deploy/state_update.go +++ b/bundle/deploy/state_update.go @@ -39,7 +39,7 @@ func (s *stateUpdate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnost state.Version = DeploymentStateVersion // Get the current file list. - sync, err := files.GetSync(ctx, b) + sync, err := files.GetSync(ctx, bundle.ReadOnly(b)) if err != nil { return diag.FromErr(err) } diff --git a/bundle/mutator_read_only.go b/bundle/mutator_read_only.go new file mode 100644 index 0000000000..ee4e36e0fa --- /dev/null +++ b/bundle/mutator_read_only.go @@ -0,0 +1,29 @@ +package bundle + +import ( + "context" + + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/log" +) + +// ReadOnlyMutator is the interface type that allows access to bundle configuration but does not allow any mutations. +type ReadOnlyMutator interface { + // Name returns the mutators name. + Name() string + + // Apply access the specified read-only bundle object. + Apply(context.Context, ReadOnlyBundle) diag.Diagnostics +} + +func ApplyReadOnly(ctx context.Context, rb ReadOnlyBundle, m ReadOnlyMutator) diag.Diagnostics { + ctx = log.NewContext(ctx, log.GetLogger(ctx).With("mutator (read-only)", m.Name())) + + log.Debugf(ctx, "ApplyReadOnly") + diags := m.Apply(ctx, rb) + if err := diags.Error(); err != nil { + log.Errorf(ctx, "Error: %s", err) + } + + return diags +} diff --git a/bundle/parallel.go b/bundle/parallel.go index df4f12a13d..5e6d06530f 100644 --- a/bundle/parallel.go +++ b/bundle/parallel.go @@ -8,21 +8,26 @@ import ( ) type parallel struct { - mutators []Mutator + mutators []ReadOnlyMutator } func (m *parallel) Name() string { return "parallel" } -func (m *parallel) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { +func (m *parallel) Apply(ctx context.Context, rb ReadOnlyBundle) diag.Diagnostics { var wg sync.WaitGroup - diags := diag.Diagnostics{} + var mu sync.Mutex + var diags diag.Diagnostics + wg.Add(len(m.mutators)) for _, mutator := range m.mutators { - go func(mutator Mutator) { + go func(mutator ReadOnlyMutator) { defer wg.Done() - diags = diags.Extend(mutator.Apply(ctx, b)) + + mu.Lock() + diags = diags.Extend(ApplyReadOnly(ctx, rb, mutator)) + mu.Unlock() }(mutator) } wg.Wait() @@ -30,7 +35,7 @@ func (m *parallel) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { } // Parallel runs the given mutators in parallel. -func Parallel(mutators ...Mutator) Mutator { +func Parallel(mutators ...ReadOnlyMutator) ReadOnlyMutator { return ¶llel{ mutators: mutators, } diff --git a/bundle/parallel_test.go b/bundle/parallel_test.go index 0e14bcb491..be1e336377 100644 --- a/bundle/parallel_test.go +++ b/bundle/parallel_test.go @@ -11,16 +11,17 @@ import ( type addToContainer struct { container *[]int + value int err bool } -func (m *addToContainer) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { +func (m *addToContainer) Apply(ctx context.Context, b ReadOnlyBundle) diag.Diagnostics { if m.err { return diag.Errorf("error") } c := *m.container - c = append(c, 1) + c = append(c, m.value) *m.container = c return nil } @@ -35,16 +36,19 @@ func TestParallelMutatorWork(t *testing.T) { } container := []int{} - m1 := &addToContainer{container: &container} - m2 := &addToContainer{container: &container} - m3 := &addToContainer{container: &container} + m1 := &addToContainer{container: &container, value: 1} + m2 := &addToContainer{container: &container, value: 2} + m3 := &addToContainer{container: &container, value: 3} m := Parallel(m1, m2, m3) // Apply the mutator - diags := m.Apply(context.Background(), b) + diags := ApplyReadOnly(context.Background(), ReadOnly(b), m) require.Empty(t, diags) require.Len(t, container, 3) + require.Contains(t, container, 1) + require.Contains(t, container, 2) + require.Contains(t, container, 3) } func TestParallelMutatorWorkWithErrors(t *testing.T) { @@ -53,15 +57,17 @@ func TestParallelMutatorWorkWithErrors(t *testing.T) { } container := []int{} - m1 := &addToContainer{container: &container} - m2 := &addToContainer{container: &container, err: true} - m3 := &addToContainer{container: &container} + m1 := &addToContainer{container: &container, value: 1} + m2 := &addToContainer{container: &container, err: true, value: 2} + m3 := &addToContainer{container: &container, value: 3} m := Parallel(m1, m2, m3) // Apply the mutator - diags := m.Apply(context.Background(), b) + diags := ApplyReadOnly(context.Background(), ReadOnly(b), m) require.Len(t, diags, 1) require.Equal(t, "error", diags[0].Summary) require.Len(t, container, 2) + require.Contains(t, container, 1) + require.Contains(t, container, 3) } diff --git a/bundle/tests/job_cluster_key_test.go b/bundle/tests/job_cluster_key_test.go index 869a83fdec..5a8b368e52 100644 --- a/bundle/tests/job_cluster_key_test.go +++ b/bundle/tests/job_cluster_key_test.go @@ -13,7 +13,7 @@ import ( func TestJobClusterKeyNotDefinedTest(t *testing.T) { b := loadTarget(t, "./job_cluster_key", "default") - diags := bundle.Apply(context.Background(), b, validate.JobClusterKeyDefined()) + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), validate.JobClusterKeyDefined()) require.Len(t, diags, 1) require.NoError(t, diags.Error()) require.Equal(t, diags[0].Severity, diag.Warning) @@ -23,6 +23,6 @@ func TestJobClusterKeyNotDefinedTest(t *testing.T) { func TestJobClusterKeyDefinedTest(t *testing.T) { b := loadTarget(t, "./job_cluster_key", "development") - diags := bundle.Apply(context.Background(), b, validate.JobClusterKeyDefined()) + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), validate.JobClusterKeyDefined()) require.Len(t, diags, 0) } diff --git a/bundle/tests/sync_include_exclude_no_matches_test.go b/bundle/tests/sync_include_exclude_no_matches_test.go new file mode 100644 index 0000000000..a215db04e1 --- /dev/null +++ b/bundle/tests/sync_include_exclude_no_matches_test.go @@ -0,0 +1,39 @@ +package config_tests + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/diag" + "github.com/stretchr/testify/require" +) + +func TestSyncIncludeExcludeNoMatchesTest(t *testing.T) { + b := loadTarget(t, "./override_sync", "development") + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), validate.ValidateSyncPatterns()) + require.Len(t, diags, 3) + require.NoError(t, diags.Error()) + require.Equal(t, diags[0].Severity, diag.Warning) + require.Equal(t, diags[0].Summary, "Exclude pattern dist does not match any files") + require.Equal(t, diags[0].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[0].Location.Line, 17) + require.Equal(t, diags[0].Location.Column, 11) + require.Equal(t, diags[0].Path.String(), "sync.exclude[0]") + + require.Equal(t, diags[1].Severity, diag.Warning) + require.Equal(t, diags[1].Summary, "Include pattern src/* does not match any files") + require.Equal(t, diags[1].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[1].Location.Line, 9) + require.Equal(t, diags[1].Location.Column, 7) + require.Equal(t, diags[1].Path.String(), "sync.include[0]") + + require.Equal(t, diags[2].Severity, diag.Warning) + require.Equal(t, diags[2].Summary, "Include pattern tests/* does not match any files") + require.Equal(t, diags[2].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[2].Location.Line, 15) + require.Equal(t, diags[2].Location.Column, 11) + require.Equal(t, diags[2].Path.String(), "sync.include[1]") +} diff --git a/cmd/bundle/sync.go b/cmd/bundle/sync.go index 0818aecf73..72ad8eb3a1 100644 --- a/cmd/bundle/sync.go +++ b/cmd/bundle/sync.go @@ -21,7 +21,7 @@ type syncFlags struct { } func (f *syncFlags) syncOptionsFromBundle(cmd *cobra.Command, b *bundle.Bundle) (*sync.SyncOptions, error) { - opts, err := files.GetSyncOptions(cmd.Context(), b) + opts, err := files.GetSyncOptions(cmd.Context(), bundle.ReadOnly(b)) if err != nil { return nil, fmt.Errorf("cannot get sync options: %w", err) } diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index 6899d6fe1f..42550722b7 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -30,7 +30,7 @@ func (f *syncFlags) syncOptionsFromBundle(cmd *cobra.Command, args []string, b * return nil, fmt.Errorf("SRC and DST are not configurable in the context of a bundle") } - opts, err := files.GetSyncOptions(cmd.Context(), b) + opts, err := files.GetSyncOptions(cmd.Context(), bundle.ReadOnly(b)) if err != nil { return nil, fmt.Errorf("cannot get sync options: %w", err) } From f898ebc1c3ee301e1a6a7743d9a7a8ae65fb04d9 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 14:24:11 +0200 Subject: [PATCH 3/7] fix --- bundle/parallel.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bundle/parallel.go b/bundle/parallel.go index 5e6d06530f..ebb91661a9 100644 --- a/bundle/parallel.go +++ b/bundle/parallel.go @@ -24,9 +24,10 @@ func (m *parallel) Apply(ctx context.Context, rb ReadOnlyBundle) diag.Diagnostic for _, mutator := range m.mutators { go func(mutator ReadOnlyMutator) { defer wg.Done() + d := ApplyReadOnly(ctx, rb, mutator) mu.Lock() - diags = diags.Extend(ApplyReadOnly(ctx, rb, mutator)) + diags = diags.Extend(d) mu.Unlock() }(mutator) } From 053678440c3d4974562aeeb36facda1949ff396f Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 14:28:17 +0200 Subject: [PATCH 4/7] fix test --- bundle/tests/sync_include_exclude_no_matches_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bundle/tests/sync_include_exclude_no_matches_test.go b/bundle/tests/sync_include_exclude_no_matches_test.go index a215db04e1..51575426df 100644 --- a/bundle/tests/sync_include_exclude_no_matches_test.go +++ b/bundle/tests/sync_include_exclude_no_matches_test.go @@ -2,6 +2,7 @@ package config_tests import ( "context" + "path/filepath" "testing" "github.com/databricks/cli/bundle" @@ -18,21 +19,21 @@ func TestSyncIncludeExcludeNoMatchesTest(t *testing.T) { require.NoError(t, diags.Error()) require.Equal(t, diags[0].Severity, diag.Warning) require.Equal(t, diags[0].Summary, "Exclude pattern dist does not match any files") - require.Equal(t, diags[0].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[0].Location.File, filepath.Join("override_sync", "databricks.yml")) require.Equal(t, diags[0].Location.Line, 17) require.Equal(t, diags[0].Location.Column, 11) require.Equal(t, diags[0].Path.String(), "sync.exclude[0]") require.Equal(t, diags[1].Severity, diag.Warning) require.Equal(t, diags[1].Summary, "Include pattern src/* does not match any files") - require.Equal(t, diags[1].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[1].Location.File, filepath.Join("override_sync", "databricks.yml")) require.Equal(t, diags[1].Location.Line, 9) require.Equal(t, diags[1].Location.Column, 7) require.Equal(t, diags[1].Path.String(), "sync.include[0]") require.Equal(t, diags[2].Severity, diag.Warning) require.Equal(t, diags[2].Summary, "Include pattern tests/* does not match any files") - require.Equal(t, diags[2].Location.File, "override_sync/databricks.yml") + require.Equal(t, diags[2].Location.File, filepath.Join("override_sync", "databricks.yml")) require.Equal(t, diags[2].Location.Line, 15) require.Equal(t, diags[2].Location.Column, 11) require.Equal(t, diags[2].Path.String(), "sync.include[1]") From 2368525753e41af2c03f4e98ee5377b6f8c8a2df Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 14:50:42 +0200 Subject: [PATCH 5/7] parallel sync validate --- .../config/validate/validate_sync_patterns.go | 66 +++++++++---------- .../sync_include_exclude_no_matches_test.go | 20 +++--- 2 files changed, 42 insertions(+), 44 deletions(-) diff --git a/bundle/config/validate/validate_sync_patterns.go b/bundle/config/validate/validate_sync_patterns.go index ad840ab531..667f8bdaa8 100644 --- a/bundle/config/validate/validate_sync_patterns.go +++ b/bundle/config/validate/validate_sync_patterns.go @@ -3,10 +3,12 @@ package validate import ( "context" "fmt" + "sync" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/fileset" + "golang.org/x/sync/errgroup" ) func ValidateSyncPatterns() bundle.ReadOnlyMutator { @@ -21,59 +23,57 @@ func (v *validateSyncPatterns) Name() string { } func (v *validateSyncPatterns) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { - sync := rb.Config().Sync() - if len(sync.Exclude) == 0 && len(sync.Include) == 0 { + s := rb.Config().Sync() + if len(s.Exclude) == 0 && len(s.Include) == 0 { return nil } - diags := diag.Diagnostics{} - if len(sync.Exclude) != 0 { - for i, exclude := range sync.Exclude { - fs, err := fileset.NewGlobSet(rb.RootPath(), []string{exclude}) - if err != nil { - return diag.FromErr(err) - } - - all, err := fs.All() - if err != nil { - return diag.FromErr(err) - } + diags, err := checkPatterns(s.Exclude, "sync.exclude", rb) + if err != nil { + return diag.FromErr(err) + } - if len(all) == 0 { - loc := location{path: fmt.Sprintf("sync.exclude[%d]", i), rb: rb} - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("Exclude pattern %s does not match any files", exclude), - Location: loc.Location(), - Path: loc.Path(), - }) - } - } + includeDiags, err := checkPatterns(s.Include, "sync.include", rb) + if err != nil { + return diag.FromErr(err) } - if len(sync.Include) != 0 { - for i, include := range sync.Include { - fs, err := fileset.NewGlobSet(rb.RootPath(), []string{include}) + return diags.Extend(includeDiags) +} + +func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (diag.Diagnostics, error) { + var mu sync.Mutex + var errs errgroup.Group + var diags diag.Diagnostics + + for i, pattern := range patterns { + index := i + p := pattern + errs.Go(func() error { + fs, err := fileset.NewGlobSet(rb.RootPath(), []string{p}) if err != nil { - return diag.FromErr(err) + return err } all, err := fs.All() if err != nil { - return diag.FromErr(err) + return err } if len(all) == 0 { - loc := location{path: fmt.Sprintf("sync.include[%d]", i), rb: rb} + loc := location{path: fmt.Sprintf("%s[%d]", path, index), rb: rb} + mu.Lock() diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, - Summary: fmt.Sprintf("Include pattern %s does not match any files", include), + Summary: fmt.Sprintf("Pattern %s does not match any files", p), Location: loc.Location(), Path: loc.Path(), }) + mu.Unlock() } - } + return nil + }) } - return diags + return diags, errs.Wait() } diff --git a/bundle/tests/sync_include_exclude_no_matches_test.go b/bundle/tests/sync_include_exclude_no_matches_test.go index 51575426df..7004830f99 100644 --- a/bundle/tests/sync_include_exclude_no_matches_test.go +++ b/bundle/tests/sync_include_exclude_no_matches_test.go @@ -17,24 +17,22 @@ func TestSyncIncludeExcludeNoMatchesTest(t *testing.T) { diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), validate.ValidateSyncPatterns()) require.Len(t, diags, 3) require.NoError(t, diags.Error()) + require.Equal(t, diags[0].Severity, diag.Warning) - require.Equal(t, diags[0].Summary, "Exclude pattern dist does not match any files") + require.Equal(t, diags[0].Summary, "Pattern dist does not match any files") require.Equal(t, diags[0].Location.File, filepath.Join("override_sync", "databricks.yml")) require.Equal(t, diags[0].Location.Line, 17) require.Equal(t, diags[0].Location.Column, 11) require.Equal(t, diags[0].Path.String(), "sync.exclude[0]") + summaries := []string{ + diags[1].Summary, + diags[2].Summary, + } + require.Equal(t, diags[1].Severity, diag.Warning) - require.Equal(t, diags[1].Summary, "Include pattern src/* does not match any files") - require.Equal(t, diags[1].Location.File, filepath.Join("override_sync", "databricks.yml")) - require.Equal(t, diags[1].Location.Line, 9) - require.Equal(t, diags[1].Location.Column, 7) - require.Equal(t, diags[1].Path.String(), "sync.include[0]") + require.Contains(t, summaries, diags[1].Summary) require.Equal(t, diags[2].Severity, diag.Warning) - require.Equal(t, diags[2].Summary, "Include pattern tests/* does not match any files") - require.Equal(t, diags[2].Location.File, filepath.Join("override_sync", "databricks.yml")) - require.Equal(t, diags[2].Location.Line, 15) - require.Equal(t, diags[2].Location.Column, 11) - require.Equal(t, diags[2].Path.String(), "sync.include[1]") + require.Contains(t, summaries, diags[2].Summary) } From 85dd7e61939de5715b18e57b3a52d80a9835517d Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 14:54:13 +0200 Subject: [PATCH 6/7] fix tests --- bundle/tests/sync_include_exclude_no_matches_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bundle/tests/sync_include_exclude_no_matches_test.go b/bundle/tests/sync_include_exclude_no_matches_test.go index 7004830f99..135e2faaca 100644 --- a/bundle/tests/sync_include_exclude_no_matches_test.go +++ b/bundle/tests/sync_include_exclude_no_matches_test.go @@ -2,6 +2,7 @@ package config_tests import ( "context" + "fmt" "path/filepath" "testing" @@ -26,8 +27,8 @@ func TestSyncIncludeExcludeNoMatchesTest(t *testing.T) { require.Equal(t, diags[0].Path.String(), "sync.exclude[0]") summaries := []string{ - diags[1].Summary, - diags[2].Summary, + fmt.Sprintf("Pattern %s does not match any files", filepath.Join("src", "*")), + fmt.Sprintf("Pattern %s does not match any files", filepath.Join("tests", "*")), } require.Equal(t, diags[1].Severity, diag.Warning) From 40d451f6deacb0b8df72d26a6ded2f90ca0baec2 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 18 Apr 2024 12:46:25 +0200 Subject: [PATCH 7/7] fixes --- bundle/bundle_read_only.go | 41 +------- bundle/config/root.go | 2 +- bundle/config/root_read_only.go | 68 ------------- bundle/config/validate/files_to_sync.go | 2 +- .../validate/job_cluster_key_defined.go | 48 ++++----- .../validate/job_cluster_key_defined_test.go | 97 +++++++++++++++++++ .../config/validate/validate_sync_patterns.go | 2 +- bundle/deploy/files/sync.go | 8 +- 8 files changed, 126 insertions(+), 142 deletions(-) delete mode 100644 bundle/config/root_read_only.go create mode 100644 bundle/config/validate/job_cluster_key_defined_test.go diff --git a/bundle/bundle_read_only.go b/bundle/bundle_read_only.go index a97b70d8d6..e4a4f99366 100644 --- a/bundle/bundle_read_only.go +++ b/bundle/bundle_read_only.go @@ -4,12 +4,7 @@ import ( "context" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/metadata" - "github.com/databricks/cli/libs/locker" - "github.com/databricks/cli/libs/tags" - "github.com/databricks/cli/libs/terraform" "github.com/databricks/databricks-sdk-go" - "github.com/hashicorp/terraform-exec/tfexec" ) type ReadOnlyBundle struct { @@ -20,38 +15,14 @@ func ReadOnly(b *Bundle) ReadOnlyBundle { return ReadOnlyBundle{b: b} } -func (r ReadOnlyBundle) Config() config.ReadOnlyConfig { - return config.ReadOnly(r.b.Config) -} - -func (r ReadOnlyBundle) AutoApprove() bool { - return r.b.AutoApprove -} - -func (r ReadOnlyBundle) Locker() *locker.Locker { - return r.b.Locker -} - -func (r ReadOnlyBundle) Metadata() metadata.Metadata { - return r.b.Metadata -} - -func (r ReadOnlyBundle) Plan() *terraform.Plan { - return r.b.Plan +func (r ReadOnlyBundle) Config() config.Root { + return r.b.Config } func (r ReadOnlyBundle) RootPath() string { return r.b.RootPath } -func (r ReadOnlyBundle) Tagging() tags.Cloud { - return r.b.Tagging -} - -func (r ReadOnlyBundle) Terraform() *tfexec.Terraform { - return r.b.Terraform -} - func (r ReadOnlyBundle) WorkspaceClient() *databricks.WorkspaceClient { return r.b.WorkspaceClient() } @@ -60,14 +31,6 @@ func (r ReadOnlyBundle) CacheDir(ctx context.Context, paths ...string) (string, return r.b.CacheDir(ctx, paths...) } -func (r ReadOnlyBundle) InternalDir(ctx context.Context) (string, error) { - return r.b.InternalDir(ctx) -} - -func (r ReadOnlyBundle) SetWorkpaceClient(w *databricks.WorkspaceClient) { - r.b.SetWorkpaceClient(w) -} - func (r ReadOnlyBundle) GetSyncIncludePatterns(ctx context.Context) ([]string, error) { return r.b.GetSyncIncludePatterns(ctx) } diff --git a/bundle/config/root.go b/bundle/config/root.go index 18b548d643..b5a481cde2 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -440,7 +440,7 @@ func validateVariableOverrides(root, target dyn.Value) (err error) { // Best effort to get the location of configuration value at the specified path. // This function is useful to annotate error messages with the location, because // we don't want to fail with a different error message if we cannot retrieve the location. -func (r *Root) GetLocation(path string) dyn.Location { +func (r Root) GetLocation(path string) dyn.Location { v, err := dyn.Get(r.value, path) if err != nil { return dyn.Location{} diff --git a/bundle/config/root_read_only.go b/bundle/config/root_read_only.go deleted file mode 100644 index c2eaf1658e..0000000000 --- a/bundle/config/root_read_only.go +++ /dev/null @@ -1,68 +0,0 @@ -package config - -import ( - "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/config/variable" - "github.com/databricks/cli/libs/dyn" - "github.com/databricks/databricks-sdk-go/service/jobs" -) - -type ReadOnlyConfig struct { - r Root -} - -func ReadOnly(r Root) ReadOnlyConfig { - return ReadOnlyConfig{r: r} -} - -func (r ReadOnlyConfig) Artifacts() Artifacts { - return r.r.Artifacts -} - -func (r ReadOnlyConfig) Bundle() Bundle { - return r.r.Bundle -} - -func (r ReadOnlyConfig) Environments() map[string]*Target { - return r.r.Environments -} - -func (r ReadOnlyConfig) Experimental() *Experimental { - return r.r.Experimental -} - -func (r ReadOnlyConfig) Include() []string { - return r.r.Include -} - -func (r ReadOnlyConfig) Permissions() []resources.Permission { - return r.r.Permissions -} - -func (r ReadOnlyConfig) Resources() Resources { - return r.r.Resources -} - -func (r ReadOnlyConfig) Targets() map[string]*Target { - return r.r.Targets -} - -func (r ReadOnlyConfig) RunAs() *jobs.JobRunAs { - return r.r.RunAs -} - -func (r ReadOnlyConfig) Sync() Sync { - return r.r.Sync -} - -func (r ReadOnlyConfig) Variables() map[string]*variable.Variable { - return r.r.Variables -} - -func (r ReadOnlyConfig) Workspace() Workspace { - return r.r.Workspace -} - -func (r ReadOnlyConfig) GetLocation(path string) dyn.Location { - return r.r.GetLocation(path) -} diff --git a/bundle/config/validate/files_to_sync.go b/bundle/config/validate/files_to_sync.go index 4e6cf75745..d53e382432 100644 --- a/bundle/config/validate/files_to_sync.go +++ b/bundle/config/validate/files_to_sync.go @@ -35,7 +35,7 @@ func (v *filesToSync) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag. } diags := diag.Diagnostics{} - if len(rb.Config().Sync().Exclude) == 0 { + if len(rb.Config().Sync.Exclude) == 0 { diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, Summary: "There are no files to sync, please check your .gitignore", diff --git a/bundle/config/validate/job_cluster_key_defined.go b/bundle/config/validate/job_cluster_key_defined.go index 69047ba483..37ed3f417e 100644 --- a/bundle/config/validate/job_cluster_key_defined.go +++ b/bundle/config/validate/job_cluster_key_defined.go @@ -22,40 +22,32 @@ func (v *jobClusterKeyDefined) Name() string { func (v *jobClusterKeyDefined) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { diags := diag.Diagnostics{} - for k, job := range rb.Config().Resources().Jobs { - jobClusterKeys := make(map[string]location) - for index, task := range job.Tasks { - if task.JobClusterKey != "" { - jobClusterKeys[task.JobClusterKey] = location{ - path: fmt.Sprintf("resources.jobs.%s.tasks[%d].job_cluster_key", k, index), - rb: rb, - } + for k, job := range rb.Config().Resources.Jobs { + jobClusterKeys := make(map[string]bool) + for _, cluster := range job.JobClusters { + if cluster.JobClusterKey != "" { + jobClusterKeys[cluster.JobClusterKey] = true } } - // Check if all job_cluster_keys are defined - for key, loc := range jobClusterKeys { - if !isJobClusterKeyDefined(key, rb) { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("job_cluster_key %s is not defined", key), - Location: loc.Location(), - Path: loc.Path(), - }) + for index, task := range job.Tasks { + if task.JobClusterKey != "" { + if _, ok := jobClusterKeys[task.JobClusterKey]; !ok { + loc := location{ + path: fmt.Sprintf("resources.jobs.%s.tasks[%d].job_cluster_key", k, index), + rb: rb, + } + + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("job_cluster_key %s is not defined", task.JobClusterKey), + Location: loc.Location(), + Path: loc.Path(), + }) + } } } } return diags } - -func isJobClusterKeyDefined(key string, rb bundle.ReadOnlyBundle) bool { - for _, job := range rb.Config().Resources().Jobs { - for _, cluster := range job.JobClusters { - if cluster.JobClusterKey == key { - return true - } - } - } - return false -} diff --git a/bundle/config/validate/job_cluster_key_defined_test.go b/bundle/config/validate/job_cluster_key_defined_test.go new file mode 100644 index 0000000000..176b0fedc1 --- /dev/null +++ b/bundle/config/validate/job_cluster_key_defined_test.go @@ -0,0 +1,97 @@ +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/libs/diag" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/require" +) + +func TestJobClusterKeyDefined(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": { + JobSettings: &jobs.JobSettings{ + Name: "job1", + JobClusters: []jobs.JobCluster{ + {JobClusterKey: "do-not-exist"}, + }, + Tasks: []jobs.Task{ + {JobClusterKey: "do-not-exist"}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined()) + require.Len(t, diags, 0) + require.NoError(t, diags.Error()) +} + +func TestJobClusterKeyNotDefined(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": { + JobSettings: &jobs.JobSettings{ + Name: "job1", + Tasks: []jobs.Task{ + {JobClusterKey: "do-not-exist"}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined()) + require.Len(t, diags, 1) + require.NoError(t, diags.Error()) + require.Equal(t, diags[0].Severity, diag.Warning) + require.Equal(t, diags[0].Summary, "job_cluster_key do-not-exist is not defined") +} + +func TestJobClusterKeyDefinedInDifferentJob(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": { + JobSettings: &jobs.JobSettings{ + Name: "job1", + Tasks: []jobs.Task{ + {JobClusterKey: "do-not-exist"}, + }, + }, + }, + "job2": { + JobSettings: &jobs.JobSettings{ + Name: "job2", + JobClusters: []jobs.JobCluster{ + {JobClusterKey: "do-not-exist"}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined()) + require.Len(t, diags, 1) + require.NoError(t, diags.Error()) + require.Equal(t, diags[0].Severity, diag.Warning) + require.Equal(t, diags[0].Summary, "job_cluster_key do-not-exist is not defined") +} diff --git a/bundle/config/validate/validate_sync_patterns.go b/bundle/config/validate/validate_sync_patterns.go index 667f8bdaa8..58acf6ae47 100644 --- a/bundle/config/validate/validate_sync_patterns.go +++ b/bundle/config/validate/validate_sync_patterns.go @@ -23,7 +23,7 @@ func (v *validateSyncPatterns) Name() string { } func (v *validateSyncPatterns) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { - s := rb.Config().Sync() + s := rb.Config().Sync if len(s.Exclude) == 0 && len(s.Include) == 0 { return nil } diff --git a/bundle/deploy/files/sync.go b/bundle/deploy/files/sync.go index 5f84bdee19..d78ab2d749 100644 --- a/bundle/deploy/files/sync.go +++ b/bundle/deploy/files/sync.go @@ -29,9 +29,9 @@ func GetSyncOptions(ctx context.Context, rb bundle.ReadOnlyBundle) (*sync.SyncOp opts := &sync.SyncOptions{ LocalPath: rb.RootPath(), - RemotePath: rb.Config().Workspace().FilePath, + RemotePath: rb.Config().Workspace.FilePath, Include: includes, - Exclude: rb.Config().Sync().Exclude, + Exclude: rb.Config().Sync.Exclude, Host: rb.WorkspaceClient().Config.Host, Full: false, @@ -40,8 +40,8 @@ func GetSyncOptions(ctx context.Context, rb bundle.ReadOnlyBundle) (*sync.SyncOp WorkspaceClient: rb.WorkspaceClient(), } - if rb.Config().Workspace().CurrentUser != nil { - opts.CurrentUser = rb.Config().Workspace().CurrentUser.User + if rb.Config().Workspace.CurrentUser != nil { + opts.CurrentUser = rb.Config().Workspace.CurrentUser.User } return opts, nil