From 242d4b51edd783341304aaa86184725102337268 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Tue, 20 Aug 2024 05:52:00 +0530 Subject: [PATCH] Report all empty resources present in error diagnostic (#1685) ## Changes This PR addressed post-merge feedback from https://github.com/databricks/cli/pull/1673. ## Tests Unit tests, and manually. ``` Error: experiment undefined-experiment is not defined at resources.experiments.undefined-experiment in databricks.yml:11:26 Error: job undefined-job is not defined at resources.jobs.undefined-job in databricks.yml:6:19 Error: pipeline undefined-pipeline is not defined at resources.pipelines.undefined-pipeline in databricks.yml:14:24 Name: undefined-job Target: default Found 3 errors ``` --- .../validate/all_resources_have_values.go | 42 ++++++++++------ .../environments_job_and_pipeline_test.go | 9 ---- bundle/tests/undefined_job/databricks.yml | 8 --- bundle/tests/undefined_job_test.go | 22 -------- .../tests/undefined_pipeline/databricks.yml | 8 --- .../tests/undefined_resources/databricks.yml | 14 ++++++ bundle/tests/undefined_resources_test.go | 50 +++++++++++++++++++ 7 files changed, 90 insertions(+), 63 deletions(-) delete mode 100644 bundle/tests/undefined_job/databricks.yml delete mode 100644 bundle/tests/undefined_job_test.go delete mode 100644 bundle/tests/undefined_pipeline/databricks.yml create mode 100644 bundle/tests/undefined_resources/databricks.yml create mode 100644 bundle/tests/undefined_resources_test.go diff --git a/bundle/config/validate/all_resources_have_values.go b/bundle/config/validate/all_resources_have_values.go index 019fe48a..7f96e529 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" @@ -21,27 +22,36 @@ 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( - 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.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[1].Key(), "s") + + // Name of the resource. Eg: "foo" in "jobs.foo". + 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{slices.Clone(p)}, + }) + return v, nil }, ) - return diag.FromErr(err) + if err != nil { + diags = append(diags, 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 0abeb487..218d2e47 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 deleted file mode 100644 index 12c19f94..00000000 --- a/bundle/tests/undefined_job/databricks.yml +++ /dev/null @@ -1,8 +0,0 @@ -bundle: - name: undefined-job - -resources: - jobs: - undefined: - test: - name: "Test Job" diff --git a/bundle/tests/undefined_job_test.go b/bundle/tests/undefined_job_test.go deleted file mode 100644 index 4596f206..00000000 --- a/bundle/tests/undefined_job_test.go +++ /dev/null @@ -1,22 +0,0 @@ -package config_tests - -import ( - "context" - "testing" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/validate" - "github.com/stretchr/testify/assert" -) - -func TestUndefinedJobLoadsWithError(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") -} diff --git a/bundle/tests/undefined_pipeline/databricks.yml b/bundle/tests/undefined_pipeline/databricks.yml deleted file mode 100644 index a52fda38..00000000 --- a/bundle/tests/undefined_pipeline/databricks.yml +++ /dev/null @@ -1,8 +0,0 @@ -bundle: - name: undefined-pipeline - -resources: - pipelines: - undefined: - test: - name: "Test Pipeline" diff --git a/bundle/tests/undefined_resources/databricks.yml b/bundle/tests/undefined_resources/databricks.yml new file mode 100644 index 00000000..ffc0e46d --- /dev/null +++ b/bundle/tests/undefined_resources/databricks.yml @@ -0,0 +1,14 @@ +bundle: + name: undefined-job + +resources: + jobs: + undefined-job: + test: + name: "Test Job" + + experiments: + undefined-experiment: + + pipelines: + undefined-pipeline: diff --git a/bundle/tests/undefined_resources_test.go b/bundle/tests/undefined_resources_test.go new file mode 100644 index 00000000..3dbacbc2 --- /dev/null +++ b/bundle/tests/undefined_resources_test.go @@ -0,0 +1,50 @@ +package config_tests + +import ( + "context" + "path/filepath" + "testing" + + "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 TestUndefinedResourcesLoadWithError(t *testing.T) { + b := load(t, "./undefined_resources") + diags := bundle.Apply(context.Background(), b, validate.AllResourcesHaveValues()) + + 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: filepath.FromSlash("undefined_resources/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: filepath.FromSlash("undefined_resources/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: filepath.FromSlash("undefined_resources/databricks.yml"), + Line: 14, + Column: 24, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.pipelines.undefined-pipeline")}, + }) +}