From 21e158a08743dc50795de3d3a738f6f1a13ae1c4 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 15 Aug 2024 17:51:50 +0200 Subject: [PATCH 1/6] Report all empty resources present in error diagnostic --- .../validate/all_resources_have_values.go | 38 +++++++++++------ .../environments_job_and_pipeline_test.go | 9 ---- bundle/tests/undefined_job/databricks.yml | 8 +++- bundle/tests/undefined_job_test.go | 41 +++++++++++++++---- .../tests/undefined_pipeline/databricks.yml | 8 ---- 5 files changed, 66 insertions(+), 38 deletions(-) delete mode 100644 bundle/tests/undefined_pipeline/databricks.yml diff --git a/bundle/config/validate/all_resources_have_values.go b/bundle/config/validate/all_resources_have_values.go index 019fe48a21..8ceae80e83 100644 --- a/bundle/config/validate/all_resources_have_values.go +++ b/bundle/config/validate/all_resources_have_values.go @@ -23,25 +23,37 @@ func (m *allResourcesHaveValues) Name() string { func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { rv := b.Config.Value().Get("resources") - // Skip if there are no resources block defined, or the resources block is empty. - if rv.Kind() == dyn.KindInvalid || rv.Kind() == dyn.KindNil { - return nil - } + diags := diag.Diagnostics{} - _, err := dyn.MapByPattern( + dyn.MapByPattern( rv, dyn.NewPattern(dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - if v.Kind() == dyn.KindInvalid || v.Kind() == dyn.KindNil { - // Type of the resource, stripped of the trailing 's' to make it - // singular. - rType := strings.TrimSuffix(p[0].Key(), "s") - - rName := p[1].Key() - return v, fmt.Errorf("%s %s is not defined", rType, rName) + if v.Kind() != dyn.KindNil { + return v, nil } + + // Type of the resource, stripped of the trailing 's' to make it + // singular. + rType := strings.TrimSuffix(p[0].Key(), "s") + + // Name of the resource. Eg: "foo" in "jobs.foo". + rName := p[1].Key() + + // Prepend "resources" to the path. + fullPath := dyn.NewPath(dyn.Key("resources")) + fullPath = append(fullPath, p...) + + diags = append(diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("%s %s is not defined", rType, rName), + Locations: v.Locations(), + Paths: []dyn.Path{fullPath}, + }) + return v, nil }, ) - return diag.FromErr(err) + + return diags } diff --git a/bundle/tests/environments_job_and_pipeline_test.go b/bundle/tests/environments_job_and_pipeline_test.go index 0abeb487c6..218d2e4709 100644 --- a/bundle/tests/environments_job_and_pipeline_test.go +++ b/bundle/tests/environments_job_and_pipeline_test.go @@ -1,7 +1,6 @@ package config_tests import ( - "path/filepath" "testing" "github.com/databricks/cli/bundle/config" @@ -15,8 +14,6 @@ func TestJobAndPipelineDevelopmentWithEnvironment(t *testing.T) { assert.Len(t, b.Config.Resources.Pipelines, 1) p := b.Config.Resources.Pipelines["nyc_taxi_pipeline"] - l := b.Config.GetLocation("resources.pipelines.nyc_taxi_pipeline") - assert.Equal(t, "environments_job_and_pipeline/databricks.yml", filepath.ToSlash(l.File)) assert.Equal(t, b.Config.Bundle.Mode, config.Development) assert.True(t, p.Development) require.Len(t, p.Libraries, 1) @@ -30,8 +27,6 @@ func TestJobAndPipelineStagingWithEnvironment(t *testing.T) { assert.Len(t, b.Config.Resources.Pipelines, 1) p := b.Config.Resources.Pipelines["nyc_taxi_pipeline"] - l := b.Config.GetLocation("resources.pipelines.nyc_taxi_pipeline") - assert.Equal(t, "environments_job_and_pipeline/databricks.yml", filepath.ToSlash(l.File)) assert.False(t, p.Development) require.Len(t, p.Libraries, 1) assert.Equal(t, "./dlt/nyc_taxi_loader", p.Libraries[0].Notebook.Path) @@ -44,16 +39,12 @@ func TestJobAndPipelineProductionWithEnvironment(t *testing.T) { assert.Len(t, b.Config.Resources.Pipelines, 1) p := b.Config.Resources.Pipelines["nyc_taxi_pipeline"] - pl := b.Config.GetLocation("resources.pipelines.nyc_taxi_pipeline") - assert.Equal(t, "environments_job_and_pipeline/databricks.yml", filepath.ToSlash(pl.File)) assert.False(t, p.Development) require.Len(t, p.Libraries, 1) assert.Equal(t, "./dlt/nyc_taxi_loader", p.Libraries[0].Notebook.Path) assert.Equal(t, "nyc_taxi_production", p.Target) j := b.Config.Resources.Jobs["pipeline_schedule"] - jl := b.Config.GetLocation("resources.jobs.pipeline_schedule") - assert.Equal(t, "environments_job_and_pipeline/databricks.yml", filepath.ToSlash(jl.File)) assert.Equal(t, "Daily refresh of production pipeline", j.Name) require.Len(t, j.Tasks, 1) assert.NotEmpty(t, j.Tasks[0].PipelineTask.PipelineId) diff --git a/bundle/tests/undefined_job/databricks.yml b/bundle/tests/undefined_job/databricks.yml index 12c19f946f..ffc0e46da9 100644 --- a/bundle/tests/undefined_job/databricks.yml +++ b/bundle/tests/undefined_job/databricks.yml @@ -3,6 +3,12 @@ bundle: resources: jobs: - undefined: + undefined-job: test: name: "Test Job" + + experiments: + undefined-experiment: + + pipelines: + undefined-pipeline: diff --git a/bundle/tests/undefined_job_test.go b/bundle/tests/undefined_job_test.go index 4596f20695..2d3ee11b07 100644 --- a/bundle/tests/undefined_job_test.go +++ b/bundle/tests/undefined_job_test.go @@ -6,17 +6,44 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" ) -func TestUndefinedJobLoadsWithError(t *testing.T) { +func TestUndefinedResourcesLoadWithError(t *testing.T) { b := load(t, "./undefined_job") diags := bundle.Apply(context.Background(), b, validate.AllResourcesHaveValues()) - assert.ErrorContains(t, diags.Error(), "job undefined is not defined") -} -func TestUndefinedPipelineLoadsWithError(t *testing.T) { - b := load(t, "./undefined_pipeline") - diags := bundle.Apply(context.Background(), b, validate.AllResourcesHaveValues()) - assert.ErrorContains(t, diags.Error(), "pipeline undefined is not defined") + assert.Len(t, diags, 3) + assert.Contains(t, diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: "job undefined-job is not defined", + Locations: []dyn.Location{{ + File: "undefined_job/databricks.yml", + Line: 6, + Column: 19, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.jobs.undefined-job")}, + }) + assert.Contains(t, diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: "experiment undefined-experiment is not defined", + Locations: []dyn.Location{{ + File: "undefined_job/databricks.yml", + Line: 11, + Column: 26, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.experiments.undefined-experiment")}, + }) + assert.Contains(t, diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: "pipeline undefined-pipeline is not defined", + Locations: []dyn.Location{{ + File: "undefined_job/databricks.yml", + Line: 14, + Column: 24, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.pipelines.undefined-pipeline")}, + }) } diff --git a/bundle/tests/undefined_pipeline/databricks.yml b/bundle/tests/undefined_pipeline/databricks.yml deleted file mode 100644 index a52fda38c4..0000000000 --- a/bundle/tests/undefined_pipeline/databricks.yml +++ /dev/null @@ -1,8 +0,0 @@ -bundle: - name: undefined-pipeline - -resources: - pipelines: - undefined: - test: - name: "Test Pipeline" From 3592de9bc46a3cc4c8f35a9a1caef5ea8237a8d9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 15 Aug 2024 17:56:32 +0200 Subject: [PATCH 2/6] - --- .../{undefined_job => undefined_resources}/databricks.yml | 0 ...{undefined_job_test.go => undefined_resources_test.go} | 8 ++++---- 2 files changed, 4 insertions(+), 4 deletions(-) rename bundle/tests/{undefined_job => undefined_resources}/databricks.yml (100%) rename bundle/tests/{undefined_job_test.go => undefined_resources_test.go} (87%) diff --git a/bundle/tests/undefined_job/databricks.yml b/bundle/tests/undefined_resources/databricks.yml similarity index 100% rename from bundle/tests/undefined_job/databricks.yml rename to bundle/tests/undefined_resources/databricks.yml diff --git a/bundle/tests/undefined_job_test.go b/bundle/tests/undefined_resources_test.go similarity index 87% rename from bundle/tests/undefined_job_test.go rename to bundle/tests/undefined_resources_test.go index 2d3ee11b07..b0394fd013 100644 --- a/bundle/tests/undefined_job_test.go +++ b/bundle/tests/undefined_resources_test.go @@ -12,7 +12,7 @@ import ( ) func TestUndefinedResourcesLoadWithError(t *testing.T) { - b := load(t, "./undefined_job") + b := load(t, "./undefined_resources") diags := bundle.Apply(context.Background(), b, validate.AllResourcesHaveValues()) assert.Len(t, diags, 3) @@ -20,7 +20,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "job undefined-job is not defined", Locations: []dyn.Location{{ - File: "undefined_job/databricks.yml", + File: "undefined_resources/databricks.yml", Line: 6, Column: 19, }}, @@ -30,7 +30,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "experiment undefined-experiment is not defined", Locations: []dyn.Location{{ - File: "undefined_job/databricks.yml", + File: "undefined_resources/databricks.yml", Line: 11, Column: 26, }}, @@ -40,7 +40,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "pipeline undefined-pipeline is not defined", Locations: []dyn.Location{{ - File: "undefined_job/databricks.yml", + File: "undefined_resources/databricks.yml", Line: 14, Column: 24, }}, From 9c1f33b425b55470bdeceab310b899df882257ce Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 15 Aug 2024 17:58:02 +0200 Subject: [PATCH 3/6] handle errors --- bundle/config/validate/all_resources_have_values.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bundle/config/validate/all_resources_have_values.go b/bundle/config/validate/all_resources_have_values.go index 8ceae80e83..0e064c84f1 100644 --- a/bundle/config/validate/all_resources_have_values.go +++ b/bundle/config/validate/all_resources_have_values.go @@ -25,7 +25,7 @@ func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) di diags := diag.Diagnostics{} - dyn.MapByPattern( + _, err := dyn.MapByPattern( rv, dyn.NewPattern(dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { @@ -54,6 +54,9 @@ func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) di return v, nil }, ) + if err != nil { + diags = append(diags, diag.FromErr(err)...) + } return diags } From 52bec40de92bc73ac2797e2a60c0fb0e448ddf59 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 15 Aug 2024 18:09:51 +0200 Subject: [PATCH 4/6] handle invalid resource block --- bundle/config/validate/all_resources_have_values.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bundle/config/validate/all_resources_have_values.go b/bundle/config/validate/all_resources_have_values.go index 0e064c84f1..71fd0acb68 100644 --- a/bundle/config/validate/all_resources_have_values.go +++ b/bundle/config/validate/all_resources_have_values.go @@ -21,13 +21,11 @@ func (m *allResourcesHaveValues) Name() string { } func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - rv := b.Config.Value().Get("resources") - diags := diag.Diagnostics{} _, err := dyn.MapByPattern( - rv, - dyn.NewPattern(dyn.AnyKey(), dyn.AnyKey()), + b.Config.Value(), + dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { if v.Kind() != dyn.KindNil { return v, nil From d44044056b5ce62436de5f68cbed936dd447af56 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 15 Aug 2024 20:18:14 +0200 Subject: [PATCH 5/6] fix tests --- bundle/config/validate/all_resources_have_values.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/bundle/config/validate/all_resources_have_values.go b/bundle/config/validate/all_resources_have_values.go index 71fd0acb68..7f96e529a7 100644 --- a/bundle/config/validate/all_resources_have_values.go +++ b/bundle/config/validate/all_resources_have_values.go @@ -3,6 +3,7 @@ package validate import ( "context" "fmt" + "slices" "strings" "github.com/databricks/cli/bundle" @@ -33,20 +34,16 @@ func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) di // Type of the resource, stripped of the trailing 's' to make it // singular. - rType := strings.TrimSuffix(p[0].Key(), "s") + rType := strings.TrimSuffix(p[1].Key(), "s") // Name of the resource. Eg: "foo" in "jobs.foo". - rName := p[1].Key() - - // Prepend "resources" to the path. - fullPath := dyn.NewPath(dyn.Key("resources")) - fullPath = append(fullPath, p...) + rName := p[2].Key() diags = append(diags, diag.Diagnostic{ Severity: diag.Error, Summary: fmt.Sprintf("%s %s is not defined", rType, rName), Locations: v.Locations(), - Paths: []dyn.Path{fullPath}, + Paths: []dyn.Path{slices.Clone(p)}, }) return v, nil From e4644ab86e138765d287dc8bfb0958f8e5781d44 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 19 Aug 2024 12:03:23 +0200 Subject: [PATCH 6/6] fix windows tests --- bundle/tests/undefined_resources_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bundle/tests/undefined_resources_test.go b/bundle/tests/undefined_resources_test.go index b0394fd013..3dbacbc254 100644 --- a/bundle/tests/undefined_resources_test.go +++ b/bundle/tests/undefined_resources_test.go @@ -2,6 +2,7 @@ package config_tests import ( "context" + "path/filepath" "testing" "github.com/databricks/cli/bundle" @@ -20,7 +21,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "job undefined-job is not defined", Locations: []dyn.Location{{ - File: "undefined_resources/databricks.yml", + File: filepath.FromSlash("undefined_resources/databricks.yml"), Line: 6, Column: 19, }}, @@ -30,7 +31,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "experiment undefined-experiment is not defined", Locations: []dyn.Location{{ - File: "undefined_resources/databricks.yml", + File: filepath.FromSlash("undefined_resources/databricks.yml"), Line: 11, Column: 26, }}, @@ -40,7 +41,7 @@ func TestUndefinedResourcesLoadWithError(t *testing.T) { Severity: diag.Error, Summary: "pipeline undefined-pipeline is not defined", Locations: []dyn.Location{{ - File: "undefined_resources/databricks.yml", + File: filepath.FromSlash("undefined_resources/databricks.yml"), Line: 14, Column: 24, }},