From aa51b63352a9fc4207ee7833ad40893b8a12ee1d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 14:45:14 +0200 Subject: [PATCH 01/35] Modify SetLocation test utility to take full locations as argument --- bundle/artifacts/expand_globs_test.go | 7 ++-- .../expand_pipeline_glob_paths_test.go | 5 +-- .../config/mutator/rewrite_sync_paths_test.go | 25 ++++++++------- bundle/config/mutator/sync_infer_root_test.go | 3 +- bundle/config/mutator/translate_paths_test.go | 32 +++++++++---------- bundle/deploy/metadata/compute_test.go | 7 ++-- bundle/internal/bundletest/location.go | 6 ++-- .../libraries/expand_glob_references_test.go | 7 ++-- 8 files changed, 48 insertions(+), 44 deletions(-) diff --git a/bundle/artifacts/expand_globs_test.go b/bundle/artifacts/expand_globs_test.go index c9c478448f..1665a48063 100644 --- a/bundle/artifacts/expand_globs_test.go +++ b/bundle/artifacts/expand_globs_test.go @@ -10,6 +10,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -36,7 +37,7 @@ func TestExpandGlobs_Nominal(t *testing.T) { }, } - bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml")) + bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) ctx := context.Background() diags := bundle.Apply(ctx, b, bundle.Seq( @@ -77,7 +78,7 @@ func TestExpandGlobs_InvalidPattern(t *testing.T) { }, } - bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml")) + bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) ctx := context.Background() diags := bundle.Apply(ctx, b, bundle.Seq( @@ -125,7 +126,7 @@ func TestExpandGlobs_NoMatches(t *testing.T) { }, } - bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml")) + bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) ctx := context.Background() diags := bundle.Apply(ctx, b, bundle.Seq( diff --git a/bundle/config/mutator/expand_pipeline_glob_paths_test.go b/bundle/config/mutator/expand_pipeline_glob_paths_test.go index d1671c256f..07dd20215a 100644 --- a/bundle/config/mutator/expand_pipeline_glob_paths_test.go +++ b/bundle/config/mutator/expand_pipeline_glob_paths_test.go @@ -10,6 +10,7 @@ import ( "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/dyn" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/require" @@ -105,8 +106,8 @@ func TestExpandGlobPathsInPipelines(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) - bundletest.SetLocation(b, "resources.pipelines.pipeline.libraries[3]", filepath.Join(dir, "relative", "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) + bundletest.SetLocation(b, "resources.pipelines.pipeline.libraries[3]", []dyn.Location{{File: filepath.Join(dir, "relative", "resource.yml")}}) m := ExpandPipelineGlobPaths() diags := bundle.Apply(context.Background(), b, m) diff --git a/bundle/config/mutator/rewrite_sync_paths_test.go b/bundle/config/mutator/rewrite_sync_paths_test.go index fa7f124b79..a66f2763a4 100644 --- a/bundle/config/mutator/rewrite_sync_paths_test.go +++ b/bundle/config/mutator/rewrite_sync_paths_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" ) @@ -33,12 +34,12 @@ func TestRewriteSyncPathsRelative(t *testing.T) { }, } - bundletest.SetLocation(b, "sync.paths[0]", "./databricks.yml") - bundletest.SetLocation(b, "sync.paths[1]", "./databricks.yml") - bundletest.SetLocation(b, "sync.include[0]", "./file.yml") - bundletest.SetLocation(b, "sync.include[1]", "./a/file.yml") - bundletest.SetLocation(b, "sync.exclude[0]", "./a/b/file.yml") - bundletest.SetLocation(b, "sync.exclude[1]", "./a/b/c/file.yml") + bundletest.SetLocation(b, "sync.paths[0]", []dyn.Location{{File: "./databricks.yml"}}) + bundletest.SetLocation(b, "sync.paths[1]", []dyn.Location{{File: "./databricks.yml"}}) + bundletest.SetLocation(b, "sync.include[0]", []dyn.Location{{File: "./file.yml"}}) + bundletest.SetLocation(b, "sync.include[1]", []dyn.Location{{File: "./a/file.yml"}}) + bundletest.SetLocation(b, "sync.exclude[0]", []dyn.Location{{File: "./a/b/file.yml"}}) + bundletest.SetLocation(b, "sync.exclude[1]", []dyn.Location{{File: "./a/b/c/file.yml"}}) diags := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) assert.NoError(t, diags.Error()) @@ -72,12 +73,12 @@ func TestRewriteSyncPathsAbsolute(t *testing.T) { }, } - bundletest.SetLocation(b, "sync.paths[0]", "/tmp/dir/databricks.yml") - bundletest.SetLocation(b, "sync.paths[1]", "/tmp/dir/databricks.yml") - bundletest.SetLocation(b, "sync.include[0]", "/tmp/dir/file.yml") - bundletest.SetLocation(b, "sync.include[1]", "/tmp/dir/a/file.yml") - bundletest.SetLocation(b, "sync.exclude[0]", "/tmp/dir/a/b/file.yml") - bundletest.SetLocation(b, "sync.exclude[1]", "/tmp/dir/a/b/c/file.yml") + bundletest.SetLocation(b, "sync.paths[0]", []dyn.Location{{File: "/tmp/dir/databricks.yml"}}) + bundletest.SetLocation(b, "sync.paths[1]", []dyn.Location{{File: "/tmp/dir/databricks.yml"}}) + bundletest.SetLocation(b, "sync.include[0]", []dyn.Location{{File: "/tmp/dir/file.yml"}}) + bundletest.SetLocation(b, "sync.include[1]", []dyn.Location{{File: "/tmp/dir/a/file.yml"}}) + bundletest.SetLocation(b, "sync.exclude[0]", []dyn.Location{{File: "/tmp/dir/a/b/file.yml"}}) + bundletest.SetLocation(b, "sync.exclude[1]", []dyn.Location{{File: "/tmp/dir/a/b/c/file.yml"}}) diags := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) assert.NoError(t, diags.Error()) diff --git a/bundle/config/mutator/sync_infer_root_test.go b/bundle/config/mutator/sync_infer_root_test.go index 383e56769e..85e40adc67 100644 --- a/bundle/config/mutator/sync_infer_root_test.go +++ b/bundle/config/mutator/sync_infer_root_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -184,7 +185,7 @@ func TestSyncInferRoot_Error(t *testing.T) { }, } - bundletest.SetLocation(b, "sync.paths", "databricks.yml") + bundletest.SetLocation(b, "sync.paths", []dyn.Location{{File: "databricks.yml"}}) ctx := context.Background() diags := bundle.Apply(ctx, b, mutator.SyncInferRoot()) diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 50fcd3b07b..c03cee73e0 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -82,7 +82,7 @@ func TestTranslatePathsSkippedWithGitSource(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, diags.Error()) @@ -210,7 +210,7 @@ func TestTranslatePaths(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, diags.Error()) @@ -346,8 +346,8 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { }, } - bundletest.SetLocation(b, "resources.jobs", filepath.Join(dir, "job/resource.yml")) - bundletest.SetLocation(b, "resources.pipelines", filepath.Join(dir, "pipeline/resource.yml")) + bundletest.SetLocation(b, "resources.jobs", []dyn.Location{{File: filepath.Join(dir, "job/resource.yml")}}) + bundletest.SetLocation(b, "resources.pipelines", []dyn.Location{{File: filepath.Join(dir, "pipeline/resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, diags.Error()) @@ -408,7 +408,7 @@ func TestTranslatePathsOutsideSyncRoot(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "../resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "../resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, diags.Error(), "is not contained in sync root path") @@ -439,7 +439,7 @@ func TestJobNotebookDoesNotExistError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, diags.Error(), "notebook ./doesnt_exist.py not found") @@ -470,7 +470,7 @@ func TestJobFileDoesNotExistError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, diags.Error(), "file ./doesnt_exist.py not found") @@ -501,7 +501,7 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, diags.Error(), "notebook ./doesnt_exist.py not found") @@ -532,7 +532,7 @@ func TestPipelineFileDoesNotExistError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, diags.Error(), "file ./doesnt_exist.py not found") @@ -567,7 +567,7 @@ func TestJobSparkPythonTaskWithNotebookSourceError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, diags.Error(), `expected a file for "resources.jobs.job.tasks[0].spark_python_task.python_file" but got a notebook`) @@ -602,7 +602,7 @@ func TestJobNotebookTaskWithFileSourceError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, diags.Error(), `expected a notebook for "resources.jobs.job.tasks[0].notebook_task.notebook_path" but got a file`) @@ -637,7 +637,7 @@ func TestPipelineNotebookLibraryWithFileSourceError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, diags.Error(), `expected a notebook for "resources.pipelines.pipeline.libraries[0].notebook.path" but got a file`) @@ -672,7 +672,7 @@ func TestPipelineFileLibraryWithNotebookSourceError(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, diags.Error(), `expected a file for "resources.pipelines.pipeline.libraries[0].file.path" but got a notebook`) @@ -710,7 +710,7 @@ func TestTranslatePathJobEnvironments(t *testing.T) { }, } - bundletest.SetLocation(b, "resources.jobs", filepath.Join(dir, "job/resource.yml")) + bundletest.SetLocation(b, "resources.jobs", []dyn.Location{{File: filepath.Join(dir, "job/resource.yml")}}) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, diags.Error()) @@ -753,8 +753,8 @@ func TestTranslatePathWithComplexVariables(t *testing.T) { }, } - bundletest.SetLocation(b, "variables", filepath.Join(dir, "variables/variables.yml")) - bundletest.SetLocation(b, "resources.jobs", filepath.Join(dir, "job/resource.yml")) + bundletest.SetLocation(b, "variables", []dyn.Location{{File: filepath.Join(dir, "variables/variables.yml")}}) + bundletest.SetLocation(b, "resources.jobs", []dyn.Location{{File: filepath.Join(dir, "job/resource.yml")}}) ctx := context.Background() // Assign the variables to the dynamic configuration. diff --git a/bundle/deploy/metadata/compute_test.go b/bundle/deploy/metadata/compute_test.go index 6d43f845b1..2c2c723769 100644 --- a/bundle/deploy/metadata/compute_test.go +++ b/bundle/deploy/metadata/compute_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/bundle/metadata" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -55,9 +56,9 @@ func TestComputeMetadataMutator(t *testing.T) { }, } - bundletest.SetLocation(b, "resources.jobs.my-job-1", "a/b/c") - bundletest.SetLocation(b, "resources.jobs.my-job-2", "d/e/f") - bundletest.SetLocation(b, "resources.pipelines.my-pipeline", "abc") + bundletest.SetLocation(b, "resources.jobs.my-job-1", []dyn.Location{{File: "a/b/c"}}) + bundletest.SetLocation(b, "resources.jobs.my-job-2", []dyn.Location{{File: "d/e/f"}}) + bundletest.SetLocation(b, "resources.pipelines.my-pipeline", []dyn.Location{{File: "abc"}}) expectedMetadata := metadata.Metadata{ Version: metadata.Version, diff --git a/bundle/internal/bundletest/location.go b/bundle/internal/bundletest/location.go index 380d6e17d2..2ffd621bf5 100644 --- a/bundle/internal/bundletest/location.go +++ b/bundle/internal/bundletest/location.go @@ -8,15 +8,13 @@ import ( // SetLocation sets the location of all values in the bundle to the given path. // This is useful for testing where we need to associate configuration // with the path it is loaded from. -func SetLocation(b *bundle.Bundle, prefix string, filePath string) { +func SetLocation(b *bundle.Bundle, prefix string, locations []dyn.Location) { start := dyn.MustPathFromString(prefix) b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { return dyn.Walk(root, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // If the path has the given prefix, set the location. if p.HasPrefix(start) { - return v.WithLocations([]dyn.Location{{ - File: filePath, - }}), nil + return v.WithLocations(locations), nil } // The path is not nested under the given prefix. diff --git a/bundle/libraries/expand_glob_references_test.go b/bundle/libraries/expand_glob_references_test.go index e7f2e16935..2dfbddb743 100644 --- a/bundle/libraries/expand_glob_references_test.go +++ b/bundle/libraries/expand_glob_references_test.go @@ -10,6 +10,7 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -61,7 +62,7 @@ func TestGlobReferencesExpandedForTaskLibraries(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, ExpandGlobReferences()) require.Empty(t, diags) @@ -146,7 +147,7 @@ func TestGlobReferencesExpandedForForeachTaskLibraries(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, ExpandGlobReferences()) require.Empty(t, diags) @@ -221,7 +222,7 @@ func TestGlobReferencesExpandedForEnvironmentsDeps(t *testing.T) { }, } - bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "resource.yml")}}) diags := bundle.Apply(context.Background(), b, ExpandGlobReferences()) require.Empty(t, diags) From 667fe6bf38267709d186da257a79d6fabcc4e87a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 19 Sep 2024 11:41:40 +0200 Subject: [PATCH 02/35] Add detection and info diagnostic for convention for `..yml` files --- bundle/config/loader/process_include.go | 116 ++++++++++++++ bundle/config/loader/process_include_test.go | 150 ++++++++++++++++++- 2 files changed, 263 insertions(+), 3 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 7cf9a17d77..3ce64b299e 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -3,12 +3,84 @@ package loader import ( "context" "fmt" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "golang.org/x/exp/maps" ) +// Steps: +// 1. Return info diag here if convention not followed +// 2. Add unit test for this mutator that convention is followed. Also add mutators for the dynamic extensions computation. +// 3. Add INFO rendering to the validate command +// 4. Add unit test that the INFO rendering is correct +// 5. Manually test the info diag. + +// TODO: Since we are skipping environemnts here, we should return a warning +// if environemnts is used (is that already the case?). And explain in the PR that +// we are choosing to not gather resources from environments. + +// TODO: Talk in the PR about how this synergizes with the validate all unique +// keys mutator. + +var resourceTypes = []string{ + "job", + "pipeline", + "model", + "experiment", + "model_serving_endpoint", + "registered_model", + "quality_monitor", + "schema", +} + +type resource struct { + typ string + l dyn.Location + p dyn.Path +} + +func gatherResources(r *config.Root) (map[string]resource, error) { + res := map[string]resource{} + + // Gather all resources defined in the "resources" block. + _, err := dyn.MapByPattern( + r.Value(), + dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), + func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + // The key for the resource. Eg: "my_job" for jobs.my_job. + k := p[2].Key() + // The type of the resource. Eg: "job" for jobs.my_job. + typ := strings.TrimSuffix(p[1].Key(), "s") + + // We don't care if duplicate keys are defined across resources. That'll + // cause an error that is caught by a separate mutator that validates + // unique keys across all resources. + res[k] = resource{typ: typ, l: v.Location(), p: p} + return v, nil + }) + if err != nil { + return nil, err + } + + // Gather all resources defined in a target block. + _, err = dyn.MapByPattern( + r.Value(), + dyn.NewPattern(dyn.Key("targets"), dyn.AnyKey(), dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), + func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + k := p[4].Key() + typ := strings.TrimSuffix(p[3].Key(), "s") + + res[k] = resource{typ: typ, l: v.Location(), p: p} + return v, nil + }, + ) + return res, err +} + type processInclude struct { fullPath string relPath string @@ -31,6 +103,50 @@ func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnos if diags.HasError() { return diags } + + for _, typ := range resourceTypes { + ext := fmt.Sprintf("%s.yml", typ) + + // File does not match this resource type. Check the next one. + if !strings.HasSuffix(m.relPath, ".yml") { + continue + } + + resources, err := gatherResources(this) + if err != nil { + return diag.FromErr(err) + } + + // file only has one resource defined, and the resource is of the correct + // type. Thus it matches the recommendation we have for extensions like + // .job.yml, .pipeline.yml, etc. + keys := maps.Keys(resources) + if len(resources) == 1 && resources[keys[0]].typ == typ { + continue + } + + msg := strings.Builder{} + msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the extension %s.\n", typ, ext)) + msg.WriteString("The following resources are defined or configured in this file:\n") + for k, v := range resources { + msg.WriteString(fmt.Sprintf(" - %s (%s)\n", k, v.typ)) + } + + locations := []dyn.Location{} + paths := []dyn.Path{} + for _, r := range resources { + locations = append(locations, []dyn.Location{r.l}...) + paths = append(paths, []dyn.Path{r.p}...) + } + + diags = append(diags, diag.Diagnostic{ + Severity: diag.Info, + Summary: msg.String(), + Locations: locations, + Paths: paths, + }) + } + err := b.Config.Merge(this) if err != nil { diags = diags.Extend(diag.FromErr(err)) diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index da4da9ff66..e44e4f349e 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -1,13 +1,17 @@ -package loader_test +package loader import ( "context" "path/filepath" + "reflect" + "strings" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/loader" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -22,7 +26,7 @@ func TestProcessInclude(t *testing.T) { }, } - m := loader.ProcessInclude(filepath.Join(b.RootPath, "host.yml"), "host.yml") + m := ProcessInclude(filepath.Join(b.RootPath, "host.yml"), "host.yml") assert.Equal(t, "ProcessInclude(host.yml)", m.Name()) // Assert the host value prior to applying the mutator @@ -33,3 +37,143 @@ func TestProcessInclude(t *testing.T) { require.NoError(t, diags.Error()) assert.Equal(t, "bar", b.Config.Workspace.Host) } + +func TestResourceNames(t *testing.T) { + names := []string{} + typ := reflect.TypeOf(config.Resources{}) + for i := 0; i < typ.NumField(); i++ { + field := typ.Field(i) + jsonTags := strings.Split(field.Tag.Get("json"), ",") + singularName := strings.TrimSuffix(jsonTags[0], "s") + names = append(names, singularName) + } + + // Assert the contents of the two lists are equal. Please add the singular + // name of your resource to resourceNames global if you are adding a new + // resource. + assert.Equal(t, len(resourceTypes), len(names)) + for _, name := range names { + assert.Contains(t, resourceTypes, name) + } +} + +func TestGatherResources(t *testing.T) { + // TODO: Add location tests? + tcases := []struct { + name string + resources config.Resources + targets map[string]*config.Target + filenames map[string]string + expected map[string]resource + }{ + { + name: "empty", + resources: config.Resources{}, + expected: map[string]resource{}, + }, + { + name: "one job", + resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, + }, + // TODO CONTINUE: Setting file names for the resources defined here. + // and testing they are correctly aggregated. + expected: map[string]resource{ + "job1": { + typ: "job", + p: dyn.MustPathFromString("resources.jobs.job1"), + }, + }, + }, + // { + // name: "three jobs", + // resources: config.Resources{ + // Jobs: map[string]*resources.Job{ + // "job1": {}, + // "job2": {}, + // "job3": {}, + // }, + // }, + // expected: []resource{ + // {"job1", "job"}, + // {"job2", "job"}, + // {"job3", "job"}, + // }, + // }, + // { + // name: "only one experiment target", + // resources: config.Resources{}, + // targets: map[string]*config.Target{ + // "target1": { + // Resources: &config.Resources{ + // Experiments: map[string]*resources.MlflowExperiment{ + // "experiment1": {}, + // }, + // }, + // }, + // }, + // expected: []resource{ + // {"experiment1", "experiment"}, + // }, + // }, + // { + // name: "multiple resources", + // resources: config.Resources{ + // Jobs: map[string]*resources.Job{ + // "job1": {}, + // }, + // Pipelines: map[string]*resources.Pipeline{ + // "pipeline1": {}, + // "pipeline2": {}, + // }, + // Experiments: map[string]*resources.MlflowExperiment{ + // "experiment1": {}, + // }, + // }, + // targets: map[string]*config.Target{ + // "target1": { + // Resources: &config.Resources{ + // ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ + // "model_serving_endpoint1": {}, + // }, + // }, + // }, + // "target2": { + // Resources: &config.Resources{ + // RegisteredModels: map[string]*resources.RegisteredModel{ + // "registered_model1": {}, + // }, + // }, + // }, + // }, + // expected: []resource{ + // {"job1", "job"}, + // {"pipeline1", "pipeline"}, + // {"pipeline2", "pipeline"}, + // {"experiment1", "experiment"}, + // {"model_serving_endpoint1", "model_serving_endpoint"}, + // {"registered_model1", "registered_model"}, + // }, + // }, + } + + for _, tc := range tcases { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{} + + bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + b.Config.Resources = tc.resources + b.Config.Targets = tc.targets + return nil + }) + + res, err := gatherResources(&b.Config) + require.NoError(t, err) + + assert.Equal(t, tc.expected, res) + }) + } + +} From 458dbfb04d5c87372333d7cfde228fd168ed67e6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 19 Sep 2024 11:42:25 +0200 Subject: [PATCH 03/35] - --- bundle/config/loader/process_include.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 3ce64b299e..4ec9eba34b 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -19,6 +19,8 @@ import ( // 4. Add unit test that the INFO rendering is correct // 5. Manually test the info diag. +// TODO: Should we detect and enforce this convention for .yaml files as well? + // TODO: Since we are skipping environemnts here, we should return a warning // if environemnts is used (is that already the case?). And explain in the PR that // we are choosing to not gather resources from environments. From 5c351d7ea33c3f7eb57e8ebb21ca58b13e9a9f5a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 14:39:28 +0200 Subject: [PATCH 04/35] more wip on making it work --- bundle/config/loader/process_include.go | 137 ++++++----- bundle/config/loader/process_include_test.go | 241 ++++++++++--------- bundle/internal/bundletest/location.go | 3 +- 3 files changed, 210 insertions(+), 171 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 4ec9eba34b..cc13d6900a 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" - "golang.org/x/exp/maps" ) // Steps: @@ -27,6 +26,7 @@ import ( // TODO: Talk in the PR about how this synergizes with the validate all unique // keys mutator. +// Should I add a new abstraction for dyn values here? var resourceTypes = []string{ "job", @@ -39,16 +39,29 @@ var resourceTypes = []string{ "schema", } -type resource struct { - typ string - l dyn.Location - p dyn.Path +func validateFileFormat(r *config.Root, filePath string) diag.Diagnostics { + for _, typ := range resourceTypes { + for _, ext := range []string{fmt.Sprintf(".%s.yml", typ), fmt.Sprintf(".%s.yaml", typ)} { + if strings.HasSuffix(filePath, ext) { + return validateSingleResourceDefined(r, ext, typ) + } + } + } + + return nil } -func gatherResources(r *config.Root) (map[string]resource, error) { - res := map[string]resource{} +func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnostics { + type resource struct { + path dyn.Path + value dyn.Value + typ string + key string + } + + resources := []resource{} - // Gather all resources defined in the "resources" block. + // Gather all resources defined in the resources block. _, err := dyn.MapByPattern( r.Value(), dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), @@ -58,14 +71,11 @@ func gatherResources(r *config.Root) (map[string]resource, error) { // The type of the resource. Eg: "job" for jobs.my_job. typ := strings.TrimSuffix(p[1].Key(), "s") - // We don't care if duplicate keys are defined across resources. That'll - // cause an error that is caught by a separate mutator that validates - // unique keys across all resources. - res[k] = resource{typ: typ, l: v.Location(), p: p} + resources = append(resources, resource{path: p, value: v, typ: typ, key: k}) return v, nil }) if err != nil { - return nil, err + return diag.FromErr(err) } // Gather all resources defined in a target block. @@ -73,14 +83,65 @@ func gatherResources(r *config.Root) (map[string]resource, error) { r.Value(), dyn.NewPattern(dyn.Key("targets"), dyn.AnyKey(), dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + // The key for the resource. Eg: "my_job" for jobs.my_job. k := p[4].Key() + // The type of the resource. Eg: "job" for jobs.my_job. typ := strings.TrimSuffix(p[3].Key(), "s") - res[k] = resource{typ: typ, l: v.Location(), p: p} + resources = append(resources, resource{path: p, value: v, typ: typ, key: k}) return v, nil + }) + if err != nil { + return diag.FromErr(err) + } + + valid := true + seenKeys := map[string]struct{}{} + for _, rr := range resources { + if len(seenKeys) == 0 { + seenKeys[rr.key] = struct{}{} + continue + } + + if _, ok := seenKeys[rr.key]; !ok { + valid = false + break + } + + if rr.typ != typ { + valid = false + break + } + } + + // The file only contains one resource defined in its resources or targets block, + // and the resource is of the correct type. + if valid { + return nil + } + + msg := strings.Builder{} + msg.WriteString(fmt.Sprintf("We recommend only defining a single %s when a file has the %s extension.", typ, ext)) + msg.WriteString("The following resources are defined or configured in this file:\n") + for _, r := range resources { + msg.WriteString(fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)) + } + + locations := []dyn.Location{} + paths := []dyn.Path{} + for _, rr := range resources { + locations = append(locations, rr.value.Locations()...) + paths = append(paths, rr.path) + } + + return diag.Diagnostics{ + { + Severity: diag.Info, + Summary: msg.String(), + Locations: locations, + Paths: paths, }, - ) - return res, err + } } type processInclude struct { @@ -106,48 +167,8 @@ func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnos return diags } - for _, typ := range resourceTypes { - ext := fmt.Sprintf("%s.yml", typ) - - // File does not match this resource type. Check the next one. - if !strings.HasSuffix(m.relPath, ".yml") { - continue - } - - resources, err := gatherResources(this) - if err != nil { - return diag.FromErr(err) - } - - // file only has one resource defined, and the resource is of the correct - // type. Thus it matches the recommendation we have for extensions like - // .job.yml, .pipeline.yml, etc. - keys := maps.Keys(resources) - if len(resources) == 1 && resources[keys[0]].typ == typ { - continue - } - - msg := strings.Builder{} - msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the extension %s.\n", typ, ext)) - msg.WriteString("The following resources are defined or configured in this file:\n") - for k, v := range resources { - msg.WriteString(fmt.Sprintf(" - %s (%s)\n", k, v.typ)) - } - - locations := []dyn.Location{} - paths := []dyn.Path{} - for _, r := range resources { - locations = append(locations, []dyn.Location{r.l}...) - paths = append(paths, []dyn.Path{r.p}...) - } - - diags = append(diags, diag.Diagnostic{ - Severity: diag.Info, - Summary: msg.String(), - Locations: locations, - Paths: paths, - }) - } + // Add any diagnostics associated with the file format. + diags = append(diags, validateFileFormat(this, m.relPath)...) err := b.Config.Merge(this) if err != nil { diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index e44e4f349e..275b29fcc5 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -10,8 +10,6 @@ import ( "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/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -57,123 +55,142 @@ func TestResourceNames(t *testing.T) { } } -func TestGatherResources(t *testing.T) { - // TODO: Add location tests? - tcases := []struct { - name string - resources config.Resources - targets map[string]*config.Target - filenames map[string]string - expected map[string]resource - }{ - { - name: "empty", - resources: config.Resources{}, - expected: map[string]resource{}, - }, - { - name: "one job", - resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - }, +func TestValidateFileFormat(t *testing.T) { + onlyJob := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, }, - // TODO CONTINUE: Setting file names for the resources defined here. - // and testing they are correctly aggregated. - expected: map[string]resource{ - "job1": { - typ: "job", - p: dyn.MustPathFromString("resources.jobs.job1"), + }, + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, }, }, }, - // { - // name: "three jobs", - // resources: config.Resources{ - // Jobs: map[string]*resources.Job{ - // "job1": {}, - // "job2": {}, - // "job3": {}, - // }, - // }, - // expected: []resource{ - // {"job1", "job"}, - // {"job2", "job"}, - // {"job3", "job"}, - // }, - // }, - // { - // name: "only one experiment target", - // resources: config.Resources{}, - // targets: map[string]*config.Target{ - // "target1": { - // Resources: &config.Resources{ - // Experiments: map[string]*resources.MlflowExperiment{ - // "experiment1": {}, - // }, - // }, - // }, - // }, - // expected: []resource{ - // {"experiment1", "experiment"}, - // }, - // }, - // { - // name: "multiple resources", - // resources: config.Resources{ - // Jobs: map[string]*resources.Job{ - // "job1": {}, - // }, - // Pipelines: map[string]*resources.Pipeline{ - // "pipeline1": {}, - // "pipeline2": {}, - // }, - // Experiments: map[string]*resources.MlflowExperiment{ - // "experiment1": {}, - // }, - // }, - // targets: map[string]*config.Target{ - // "target1": { - // Resources: &config.Resources{ - // ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ - // "model_serving_endpoint1": {}, - // }, - // }, - // }, - // "target2": { - // Resources: &config.Resources{ - // RegisteredModels: map[string]*resources.RegisteredModel{ - // "registered_model1": {}, - // }, - // }, - // }, - // }, - // expected: []resource{ - // {"job1", "job"}, - // {"pipeline1", "pipeline"}, - // {"pipeline2", "pipeline"}, - // {"experiment1", "experiment"}, - // {"model_serving_endpoint1", "model_serving_endpoint"}, - // {"registered_model1", "registered_model"}, - // }, - // }, } +} - for _, tc := range tcases { - t.Run(tc.name, func(t *testing.T) { - b := &bundle.Bundle{} +// func TestGatherResources(t *testing.T) { +// // TODO: Add location tests? +// tcases := []struct { +// name string +// resources config.Resources +// targets map[string]*config.Target +// filenames map[string]string +// expected map[string]resource +// }{ +// { +// name: "empty", +// resources: config.Resources{}, +// expected: map[string]resource{}, +// }, +// { +// name: "one job", +// resources: config.Resources{ +// Jobs: map[string]*resources.Job{ +// "job1": {}, +// }, +// }, +// // TODO CONTINUE: Setting file names for the resources defined here. +// // and testing they are correctly aggregated. +// expected: map[string]resource{ +// "job1": { +// typ: "job", +// p: dyn.MustPathFromString("resources.jobs.job1"), +// }, +// }, +// }, +// { +// name: "three jobs", +// resources: config.Resources{ +// Jobs: map[string]*resources.Job{ +// "job1": {}, +// "job2": {}, +// "job3": {}, +// }, +// }, +// expected: []resource{ +// {"job1", "job"}, +// {"job2", "job"}, +// {"job3", "job"}, +// }, +// }, +// { +// name: "only one experiment target", +// resources: config.Resources{}, +// targets: map[string]*config.Target{ +// "target1": { +// Resources: &config.Resources{ +// Experiments: map[string]*resources.MlflowExperiment{ +// "experiment1": {}, +// }, +// }, +// }, +// }, +// expected: []resource{ +// {"experiment1", "experiment"}, +// }, +// }, +// { +// name: "multiple resources", +// resources: config.Resources{ +// Jobs: map[string]*resources.Job{ +// "job1": {}, +// }, +// Pipelines: map[string]*resources.Pipeline{ +// "pipeline1": {}, +// "pipeline2": {}, +// }, +// Experiments: map[string]*resources.MlflowExperiment{ +// "experiment1": {}, +// }, +// }, +// targets: map[string]*config.Target{ +// "target1": { +// Resources: &config.Resources{ +// ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ +// "model_serving_endpoint1": {}, +// }, +// }, +// }, +// "target2": { +// Resources: &config.Resources{ +// RegisteredModels: map[string]*resources.RegisteredModel{ +// "registered_model1": {}, +// }, +// }, +// }, +// }, +// expected: []resource{ +// {"job1", "job"}, +// {"pipeline1", "pipeline"}, +// {"pipeline2", "pipeline"}, +// {"experiment1", "experiment"}, +// {"model_serving_endpoint1", "model_serving_endpoint"}, +// {"registered_model1", "registered_model"}, +// }, +// }, +// } - bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - b.Config.Resources = tc.resources - b.Config.Targets = tc.targets - return nil - }) +// for _, tc := range tcases { +// t.Run(tc.name, func(t *testing.T) { +// b := &bundle.Bundle{} - res, err := gatherResources(&b.Config) - require.NoError(t, err) +// bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +// b.Config.Resources = tc.resources +// b.Config.Targets = tc.targets +// return nil +// }) - assert.Equal(t, tc.expected, res) - }) - } +// res, err := gatherResources(&b.Config) +// require.NoError(t, err) -} +// assert.Equal(t, tc.expected, res) +// }) +// } + +// } diff --git a/bundle/internal/bundletest/location.go b/bundle/internal/bundletest/location.go index 2ffd621bf5..681262700f 100644 --- a/bundle/internal/bundletest/location.go +++ b/bundle/internal/bundletest/location.go @@ -8,7 +8,8 @@ import ( // SetLocation sets the location of all values in the bundle to the given path. // This is useful for testing where we need to associate configuration // with the path it is loaded from. -func SetLocation(b *bundle.Bundle, prefix string, locations []dyn.Location) { +// TODO: Go patch the location. +func SetLocation(b *bundle.Bundle, prefix string, filePath string) { start := dyn.MustPathFromString(prefix) b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { return dyn.Walk(root, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { From e9426bea8c0f4ec661f82e71d2231c507992fe98 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 14:50:38 +0200 Subject: [PATCH 05/35] - --- bundle/internal/bundletest/location.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/internal/bundletest/location.go b/bundle/internal/bundletest/location.go index 681262700f..2ffd621bf5 100644 --- a/bundle/internal/bundletest/location.go +++ b/bundle/internal/bundletest/location.go @@ -8,8 +8,7 @@ import ( // SetLocation sets the location of all values in the bundle to the given path. // This is useful for testing where we need to associate configuration // with the path it is loaded from. -// TODO: Go patch the location. -func SetLocation(b *bundle.Bundle, prefix string, filePath string) { +func SetLocation(b *bundle.Bundle, prefix string, locations []dyn.Location) { start := dyn.MustPathFromString(prefix) b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { return dyn.Walk(root, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { From fa6b68c4104a3af9c2a149484234b6559c60c55f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 16:39:02 +0200 Subject: [PATCH 06/35] add unit tests --- bundle/config/loader/process_include.go | 24 +- bundle/config/loader/process_include_test.go | 424 +++++++++++++----- .../testdata/{ => basic}/databricks.yml | 0 .../loader/testdata/{ => basic}/host.yml | 0 .../loader/testdata/format/databricks.yml | 2 + .../config/loader/testdata/format/foo.job.yml | 7 + 6 files changed, 333 insertions(+), 124 deletions(-) rename bundle/config/loader/testdata/{ => basic}/databricks.yml (100%) rename bundle/config/loader/testdata/{ => basic}/host.yml (100%) create mode 100644 bundle/config/loader/testdata/format/databricks.yml create mode 100644 bundle/config/loader/testdata/format/foo.job.yml diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index cc13d6900a..85f3fd70d6 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -3,12 +3,14 @@ package loader import ( "context" "fmt" + "sort" "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "golang.org/x/exp/maps" ) // Steps: @@ -121,10 +123,21 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos } msg := strings.Builder{} - msg.WriteString(fmt.Sprintf("We recommend only defining a single %s when a file has the %s extension.", typ, ext)) + msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.\n", typ, ext)) + + // Dedup the list of resources before adding them the diagnostic message. This + // is needed because we do not dedup earlier when gathering the resources and + // it's valid to define the same resource in both the resources and targets block. msg.WriteString("The following resources are defined or configured in this file:\n") + setOfLines := map[string]struct{}{} for _, r := range resources { - msg.WriteString(fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)) + setOfLines[fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)] = struct{}{} + } + // Sort the line s to print to make the output deterministic. + listOfLines := maps.Keys(setOfLines) + sort.Strings(listOfLines) + for _, l := range listOfLines { + msg.WriteString(l) } locations := []dyn.Location{} @@ -133,6 +146,13 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos locations = append(locations, rr.value.Locations()...) paths = append(paths, rr.path) } + // Sort the locations and paths to make the output deterministic. + sort.Slice(locations, func(i, j int) bool { + return locations[i].String() < locations[j].String() + }) + sort.Slice(paths, func(i, j int) bool { + return paths[i].String() < paths[j].String() + }) return diag.Diagnostics{ { diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 275b29fcc5..ebd9235757 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -10,13 +10,16 @@ import ( "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/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestProcessInclude(t *testing.T) { b := &bundle.Bundle{ - RootPath: "testdata", + RootPath: "testdata/basic", Config: config.Root{ Workspace: config.Workspace{ Host: "foo", @@ -36,6 +39,38 @@ func TestProcessInclude(t *testing.T) { assert.Equal(t, "bar", b.Config.Workspace.Host) } +func TestProcessIncludeValidatesFileFormat(t *testing.T) { + b := &bundle.Bundle{ + RootPath: "testdata/format", + Config: config.Root{ + Bundle: config.Bundle{ + Name: "format_test", + }, + }, + } + + m := ProcessInclude(filepath.Join(b.RootPath, "foo.job.yml"), "foo.job.yml") + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + // Assert that the diagnostics contain the expected information + assert.Len(t, diags, 1) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", + Locations: []dyn.Location{ + {File: "testdata/format/foo.job.yml", Line: 4, Column: 7}, + {File: "testdata/format/foo.job.yml", Line: 7, Column: 7}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.bar"), + dyn.MustPathFromString("resources.jobs.foo"), + }, + }, + }, diags) +} + func TestResourceNames(t *testing.T) { names := []string{} typ := reflect.TypeOf(config.Resources{}) @@ -72,125 +107,270 @@ func TestValidateFileFormat(t *testing.T) { }, }, } -} + onlyJobBundle := bundle.Bundle{Config: onlyJob} + + onlyPipeline := config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline1": {}, + }, + }, + } + onlyPipelineBundle := bundle.Bundle{Config: onlyPipeline} + + bothJobAndPipeline := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, + }, + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline1": {}, + }, + }, + }, + }, + } + bothJobAndPipelineBundle := bundle.Bundle{Config: bothJobAndPipeline} + + twoJobs := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + "job2": {}, + }, + }, + } + twoJobsBundle := bundle.Bundle{Config: twoJobs} + + twoJobsTopLevelAndTarget := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, + }, + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job2": {}, + }, + }, + }, + }, + } + twoJobsTopLevelAndTargetBundle := bundle.Bundle{Config: twoJobsTopLevelAndTarget} + + twoJobsInTarget := config.Root{ + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + "job2": {}, + }, + }, + }, + }, + } + twoJobsInTargetBundle := bundle.Bundle{Config: twoJobsInTarget} + + tcases := []struct { + name string + bundle *bundle.Bundle + expected diag.Diagnostics + fileName string + locations map[string]dyn.Location + }{ + { + name: "single job", + bundle: &onlyJobBundle, + expected: nil, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + }, + }, + { + name: "single pipeline", + bundle: &onlyPipelineBundle, + expected: nil, + fileName: "foo.pipeline.yml", + locations: map[string]dyn.Location{ + "resources.pipelines.pipeline1": {File: "foo.pipeline.yaml", Line: 1, Column: 1}, + }, + }, + { + name: "single job but extension is pipeline", + bundle: &onlyJobBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n", + Locations: []dyn.Location{ + {File: "foo.pipeline.yml", Line: 1, Column: 1}, + {File: "foo.pipeline.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), + }, + }, + }, + fileName: "foo.pipeline.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.pipeline.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job1": {File: "foo.pipeline.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline", + bundle: &bothJobAndPipelineBundle, + expected: nil, + fileName: "foo.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline but extension is job", + bundle: &bothJobAndPipelineBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline but extension is experiment", + bundle: &bothJobAndPipelineBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: "foo.experiment.yml", Line: 1, Column: 1}, + {File: "foo.experiment.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), + }, + }, + }, + fileName: "foo.experiment.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.experiment.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.experiment.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs", + bundle: &twoJobsBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs but extension is simple yaml", + bundle: &twoJobsBundle, + expected: nil, + fileName: "foo.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, + "resources.jobs.job2": {File: "foo.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs in top level and target", + bundle: &twoJobsTopLevelAndTargetBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs in target", + bundle: &twoJobsInTargetBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString(("targets.target1.resources.jobs.job1")), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "targets.target1.resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + } -// func TestGatherResources(t *testing.T) { -// // TODO: Add location tests? -// tcases := []struct { -// name string -// resources config.Resources -// targets map[string]*config.Target -// filenames map[string]string -// expected map[string]resource -// }{ -// { -// name: "empty", -// resources: config.Resources{}, -// expected: map[string]resource{}, -// }, -// { -// name: "one job", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// }, -// }, -// // TODO CONTINUE: Setting file names for the resources defined here. -// // and testing they are correctly aggregated. -// expected: map[string]resource{ -// "job1": { -// typ: "job", -// p: dyn.MustPathFromString("resources.jobs.job1"), -// }, -// }, -// }, -// { -// name: "three jobs", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// "job2": {}, -// "job3": {}, -// }, -// }, -// expected: []resource{ -// {"job1", "job"}, -// {"job2", "job"}, -// {"job3", "job"}, -// }, -// }, -// { -// name: "only one experiment target", -// resources: config.Resources{}, -// targets: map[string]*config.Target{ -// "target1": { -// Resources: &config.Resources{ -// Experiments: map[string]*resources.MlflowExperiment{ -// "experiment1": {}, -// }, -// }, -// }, -// }, -// expected: []resource{ -// {"experiment1", "experiment"}, -// }, -// }, -// { -// name: "multiple resources", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// }, -// Pipelines: map[string]*resources.Pipeline{ -// "pipeline1": {}, -// "pipeline2": {}, -// }, -// Experiments: map[string]*resources.MlflowExperiment{ -// "experiment1": {}, -// }, -// }, -// targets: map[string]*config.Target{ -// "target1": { -// Resources: &config.Resources{ -// ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ -// "model_serving_endpoint1": {}, -// }, -// }, -// }, -// "target2": { -// Resources: &config.Resources{ -// RegisteredModels: map[string]*resources.RegisteredModel{ -// "registered_model1": {}, -// }, -// }, -// }, -// }, -// expected: []resource{ -// {"job1", "job"}, -// {"pipeline1", "pipeline"}, -// {"pipeline2", "pipeline"}, -// {"experiment1", "experiment"}, -// {"model_serving_endpoint1", "model_serving_endpoint"}, -// {"registered_model1", "registered_model"}, -// }, -// }, -// } - -// for _, tc := range tcases { -// t.Run(tc.name, func(t *testing.T) { -// b := &bundle.Bundle{} - -// bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { -// b.Config.Resources = tc.resources -// b.Config.Targets = tc.targets -// return nil -// }) - -// res, err := gatherResources(&b.Config) -// require.NoError(t, err) - -// assert.Equal(t, tc.expected, res) -// }) -// } - -// } + for _, tc := range tcases { + t.Run(tc.name, func(t *testing.T) { + for k, v := range tc.locations { + bundletest.SetLocation(tc.bundle, k, []dyn.Location{v}) + } + + diags := validateFileFormat(&tc.bundle.Config, tc.fileName) + assert.Equal(t, tc.expected, diags) + }) + } +} diff --git a/bundle/config/loader/testdata/databricks.yml b/bundle/config/loader/testdata/basic/databricks.yml similarity index 100% rename from bundle/config/loader/testdata/databricks.yml rename to bundle/config/loader/testdata/basic/databricks.yml diff --git a/bundle/config/loader/testdata/host.yml b/bundle/config/loader/testdata/basic/host.yml similarity index 100% rename from bundle/config/loader/testdata/host.yml rename to bundle/config/loader/testdata/basic/host.yml diff --git a/bundle/config/loader/testdata/format/databricks.yml b/bundle/config/loader/testdata/format/databricks.yml new file mode 100644 index 0000000000..a621514c88 --- /dev/null +++ b/bundle/config/loader/testdata/format/databricks.yml @@ -0,0 +1,2 @@ +bundle: + name: format_test diff --git a/bundle/config/loader/testdata/format/foo.job.yml b/bundle/config/loader/testdata/format/foo.job.yml new file mode 100644 index 0000000000..591aaa08c7 --- /dev/null +++ b/bundle/config/loader/testdata/format/foo.job.yml @@ -0,0 +1,7 @@ +resources: + jobs: + foo: + name: foo + + bar: + name: bar From ccbd19942249f0de47db6ebd0418e4d2375e7a32 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:29:10 +0200 Subject: [PATCH 07/35] add info block --- bundle/config/loader/process_include.go | 3 - bundle/render/render_text_output.go | 20 +++++++ bundle/render/render_text_output_test.go | 73 +++++++++++++++++++++++- 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 85f3fd70d6..71867ea01a 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -14,13 +14,10 @@ import ( ) // Steps: -// 1. Return info diag here if convention not followed -// 2. Add unit test for this mutator that convention is followed. Also add mutators for the dynamic extensions computation. // 3. Add INFO rendering to the validate command // 4. Add unit test that the INFO rendering is correct // 5. Manually test the info diag. -// TODO: Should we detect and enforce this convention for .yaml files as well? // TODO: Since we are skipping environemnts here, we should return a warning // if environemnts is used (is that already the case?). And explain in the PR that diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index ea0b9a944f..529be80735 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -56,6 +56,20 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} ` +const infoTemplate = `{{ "Info" | blue }}: {{ .Summary }} +{{- range $index, $element := .Paths }} + {{ if eq $index 0 }}at {{else}} {{ end}}{{ $element.String | green }} +{{- end }} +{{- range $index, $element := .Locations }} + {{ if eq $index 0 }}in {{else}} {{ end}}{{ $element.String | cyan }} +{{- end }} +{{- if .Detail }} + +{{ .Detail }} +{{- end }} + +` + const summaryTemplate = `{{- if .Name -}} Name: {{ .Name | bold }} {{- if .Target }} @@ -94,6 +108,9 @@ func buildTrailer(diags diag.Diagnostics) string { if warnings := len(diags.Filter(diag.Warning)); warnings > 0 { parts = append(parts, color.YellowString(pluralize(warnings, "warning", "warnings"))) } + if infos := len(diags.Filter(diag.Info)); infos > 0 { + parts = append(parts, color.BlueString(pluralize(infos, "info", "infos"))) + } if len(parts) > 0 { return fmt.Sprintf("Found %s", strings.Join(parts, " and ")) } else { @@ -130,6 +147,7 @@ func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnosti func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { errorT := template.Must(template.New("error").Funcs(renderFuncMap).Parse(errorTemplate)) warningT := template.Must(template.New("warning").Funcs(renderFuncMap).Parse(warningTemplate)) + infoT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(infoTemplate)) // Print errors and warnings. for _, d := range diags { @@ -139,6 +157,8 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) t = errorT case diag.Warning: t = warningT + case diag.Info: + t = infoT } for i := range d.Locations { diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index 976f86e79c..a64ab1e0e7 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -45,6 +45,19 @@ func TestRenderTextOutput(t *testing.T) { "\n" + "Found 1 error\n", }, + { + name: "nil bundle and 1 info", + diags: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "info", + }, + }, + opts: RenderOptions{RenderSummaryTable: true}, + expected: "Info: info\n" + + "\n" + + "Found 1 info\n", + }, { name: "bundle during 'load' and 1 error", bundle: loadingBundle, @@ -84,7 +97,7 @@ func TestRenderTextOutput(t *testing.T) { "Found 2 warnings\n", }, { - name: "bundle during 'load' and 2 errors, 1 warning with details", + name: "bundle during 'load' and 2 errors, 1 warning and 1 info with details", bundle: loadingBundle, diags: diag.Diagnostics{ diag.Diagnostic{ @@ -105,6 +118,12 @@ func TestRenderTextOutput(t *testing.T) { Detail: "detail (3)", Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}}, }, + diag.Diagnostic{ + Severity: diag.Info, + Summary: "info (4)", + Detail: "detail (4)", + Locations: []dyn.Location{{File: "foo.py", Line: 4, Column: 1}}, + }, }, opts: RenderOptions{RenderSummaryTable: true}, expected: "Error: error (1)\n" + @@ -122,10 +141,15 @@ func TestRenderTextOutput(t *testing.T) { "\n" + "detail (3)\n" + "\n" + + "Info: info (4)\n" + + " in foo.py:4:1\n" + + "\n" + + "detail (4)\n" + + "\n" + "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 2 errors and 1 warning\n", + "Found 2 errors and 1 warning and 1 info\n", }, { name: "bundle during 'init'", @@ -158,7 +182,7 @@ func TestRenderTextOutput(t *testing.T) { "Validation OK!\n", }, { - name: "nil bundle without summary with 1 error and 1 warning", + name: "nil bundle without summary with 1 error, 1 warning and 1 info", bundle: nil, diags: diag.Diagnostics{ diag.Diagnostic{ @@ -173,6 +197,12 @@ func TestRenderTextOutput(t *testing.T) { Detail: "detail (2)", Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}}, }, + diag.Diagnostic{ + Severity: diag.Info, + Summary: "info (3)", + Detail: "detail (3)", + Locations: []dyn.Location{{File: "foo.py", Line: 5, Column: 1}}, + }, }, opts: RenderOptions{RenderSummaryTable: false}, expected: "Error: error (1)\n" + @@ -184,6 +214,11 @@ func TestRenderTextOutput(t *testing.T) { " in foo.py:3:1\n" + "\n" + "detail (2)\n" + + "\n" + + "Info: info (3)\n" + + " in foo.py:5:1\n" + + "\n" + + "detail (3)\n" + "\n", }, } @@ -304,6 +339,38 @@ func TestRenderDiagnostics(t *testing.T) { "\n" + "'name' is required\n\n", }, + { + name: "error with multiple paths and locations", + diags: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "summary", + Detail: "detail", + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.xxx"), + dyn.MustPathFromString("resources.jobs.yyy"), + }, + Locations: []dyn.Location{ + { + File: "foo.yaml", + Line: 1, + Column: 2, + }, + { + File: "bar.yaml", + Line: 3, + Column: 4, + }, + }, + }, + }, + expected: "Info: summary\n" + + " at resources.jobs.xxx\n" + + " resources.jobs.yyy\n" + + " in foo.yaml:1:2\n" + + " bar.yaml:3:4\n\n" + + "detail\n\n", + }, } for _, tc := range testCases { From 3571410209417bc5b2b5b7c0bfb27b66a77ce228 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:31:50 +0200 Subject: [PATCH 08/35] cleanup todos --- bundle/config/loader/process_include.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 71867ea01a..99defbc5c4 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -13,20 +13,6 @@ import ( "golang.org/x/exp/maps" ) -// Steps: -// 3. Add INFO rendering to the validate command -// 4. Add unit test that the INFO rendering is correct -// 5. Manually test the info diag. - - -// TODO: Since we are skipping environemnts here, we should return a warning -// if environemnts is used (is that already the case?). And explain in the PR that -// we are choosing to not gather resources from environments. - -// TODO: Talk in the PR about how this synergizes with the validate all unique -// keys mutator. -// Should I add a new abstraction for dyn values here? - var resourceTypes = []string{ "job", "pipeline", From f3d7f09150da4e1a49cb8e6f13aac0d58e17791f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:39:20 +0200 Subject: [PATCH 09/35] fix failing tests --- bundle/config/loader/entry_point_test.go | 2 +- bundle/config/loader/process_include.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/bundle/config/loader/entry_point_test.go b/bundle/config/loader/entry_point_test.go index 80271f0b74..540ecde88d 100644 --- a/bundle/config/loader/entry_point_test.go +++ b/bundle/config/loader/entry_point_test.go @@ -18,7 +18,7 @@ func TestEntryPointNoRootPath(t *testing.T) { func TestEntryPoint(t *testing.T) { b := &bundle.Bundle{ - RootPath: "testdata", + RootPath: "testdata/basic", } diags := bundle.Apply(context.Background(), b, loader.EntryPoint()) require.NoError(t, diags.Error()) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 99defbc5c4..e47a97502d 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -22,6 +22,7 @@ var resourceTypes = []string{ "registered_model", "quality_monitor", "schema", + "cluster", } func validateFileFormat(r *config.Root, filePath string) diag.Diagnostics { From 5bf916357ab6b68fde54e3cf4df25be8be0de952 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:42:03 +0200 Subject: [PATCH 10/35] return if diags has error --- bundle/config/loader/process_include.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index e47a97502d..a3e84711b7 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -173,6 +173,9 @@ func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnos // Add any diagnostics associated with the file format. diags = append(diags, validateFileFormat(this, m.relPath)...) + if diags.HasError() { + return diags + } err := b.Config.Merge(this) if err != nil { From e31dcc2724590069b8099457cb091edc7b319610 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:47:48 +0200 Subject: [PATCH 11/35] fix logic for dedup --- bundle/config/loader/process_include.go | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index a3e84711b7..47dabbde7a 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -81,28 +81,21 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos return diag.FromErr(err) } - valid := true + typeMatch := true seenKeys := map[string]struct{}{} for _, rr := range resources { - if len(seenKeys) == 0 { - seenKeys[rr.key] = struct{}{} - continue - } - - if _, ok := seenKeys[rr.key]; !ok { - valid = false - break - } - + // case: The resource is not of the correct type. if rr.typ != typ { - valid = false + typeMatch = false break } + + seenKeys[rr.key] = struct{}{} } - // The file only contains one resource defined in its resources or targets block, - // and the resource is of the correct type. - if valid { + // Format matches. There's less than or equal to one resource defined in the file. + // The resource is also of the correct type. + if typeMatch && len(seenKeys) <= 1 { return nil } From 894f4aab4ee3ec7eefaf7898a17fddb0917e7bfd Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 17:54:13 +0200 Subject: [PATCH 12/35] fix windows test --- bundle/config/loader/process_include_test.go | 4 ++-- bundle/render/render_text_output_test.go | 14 +++----------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index ebd9235757..1451d6fedd 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -60,8 +60,8 @@ func TestProcessIncludeValidatesFileFormat(t *testing.T) { Severity: diag.Info, Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", Locations: []dyn.Location{ - {File: "testdata/format/foo.job.yml", Line: 4, Column: 7}, - {File: "testdata/format/foo.job.yml", Line: 7, Column: 7}, + {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 7, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.bar"), diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index a64ab1e0e7..8d72298f43 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -340,7 +340,7 @@ func TestRenderDiagnostics(t *testing.T) { "'name' is required\n\n", }, { - name: "error with multiple paths and locations", + name: "info with multiple paths and locations", diags: diag.Diagnostics{ { Severity: diag.Info, @@ -351,16 +351,8 @@ func TestRenderDiagnostics(t *testing.T) { dyn.MustPathFromString("resources.jobs.yyy"), }, Locations: []dyn.Location{ - { - File: "foo.yaml", - Line: 1, - Column: 2, - }, - { - File: "bar.yaml", - Line: 3, - Column: 4, - }, + {File: "foo.yaml", Line: 1, Column: 2}, + {File: "bar.yaml", Line: 3, Column: 4}, }, }, }, From fd01824ee1bfb56c76aa33826de5b4e991a261f2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 15:39:41 +0200 Subject: [PATCH 13/35] - --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 47dabbde7a..15fb5e4c80 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -93,7 +93,7 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos seenKeys[rr.key] = struct{}{} } - // Format matches. There's less than or equal to one resource defined in the file. + // Format matches. There's at most one resource defined in the file. // The resource is also of the correct type. if typeMatch && len(seenKeys) <= 1 { return nil From 5308ad8bf33629a8dd9e9c1040770ade1c57764a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 16:03:49 +0200 Subject: [PATCH 14/35] split into summary and detail --- bundle/config/loader/process_include.go | 29 ++++++++++---------- bundle/config/loader/process_include_test.go | 21 +++++++++----- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 15fb5e4c80..33b95d5b83 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -3,6 +3,7 @@ package loader import ( "context" "fmt" + "slices" "sort" "strings" @@ -10,7 +11,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" - "golang.org/x/exp/maps" ) var resourceTypes = []string{ @@ -99,22 +99,20 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos return nil } - msg := strings.Builder{} - msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.\n", typ, ext)) - - // Dedup the list of resources before adding them the diagnostic message. This - // is needed because we do not dedup earlier when gathering the resources and - // it's valid to define the same resource in both the resources and targets block. - msg.WriteString("The following resources are defined or configured in this file:\n") - setOfLines := map[string]struct{}{} + detail := strings.Builder{} + detail.WriteString("The following resources are defined or configured in this file:\n") + lines := []string{} for _, r := range resources { - setOfLines[fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)] = struct{}{} + lines = append(lines, fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)) } // Sort the line s to print to make the output deterministic. - listOfLines := maps.Keys(setOfLines) - sort.Strings(listOfLines) - for _, l := range listOfLines { - msg.WriteString(l) + sort.Strings(lines) + // Compact the lines before writing them to the message to remove any duplicate lines. + // This is needed because we do not dedup earlier when gathering the resources + // and it's valid to define the same resource in both the resources and targets block. + lines = slices.Compact(lines) + for _, l := range lines { + detail.WriteString(l) } locations := []dyn.Location{} @@ -134,7 +132,8 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos return diag.Diagnostics{ { Severity: diag.Info, - Summary: msg.String(), + Summary: fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.", typ, ext), + Detail: detail.String(), Locations: locations, Paths: paths, }, diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 1451d6fedd..221fb7f63a 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -58,7 +58,8 @@ func TestProcessIncludeValidatesFileFormat(t *testing.T) { assert.Equal(t, diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", Locations: []dyn.Location{ {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 4, Column: 7}, {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 7, Column: 7}, @@ -209,7 +210,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n", + Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n", Locations: []dyn.Location{ {File: "foo.pipeline.yml", Line: 1, Column: 1}, {File: "foo.pipeline.yml", Line: 2, Column: 2}, @@ -242,7 +244,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ {File: "foo.job.yml", Line: 1, Column: 1}, {File: "foo.job.yml", Line: 2, Column: 2}, @@ -265,7 +268,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ {File: "foo.experiment.yml", Line: 1, Column: 1}, {File: "foo.experiment.yml", Line: 2, Column: 2}, @@ -288,7 +292,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ {File: "foo.job.yml", Line: 1, Column: 1}, {File: "foo.job.yml", Line: 2, Column: 2}, @@ -321,7 +326,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ {File: "foo.job.yml", Line: 1, Column: 1}, {File: "foo.job.yml", Line: 2, Column: 2}, @@ -344,7 +350,8 @@ func TestValidateFileFormat(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ {File: "foo.job.yml", Line: 1, Column: 1}, {File: "foo.job.yml", Line: 2, Column: 2}, From d14b1e2cca795f897880da6975ef156781a79eee Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 16:07:32 +0200 Subject: [PATCH 15/35] directly pass config value --- bundle/config/loader/process_include.go | 12 ++++++------ bundle/config/loader/process_include_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 33b95d5b83..1e1a62147c 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -25,11 +25,11 @@ var resourceTypes = []string{ "cluster", } -func validateFileFormat(r *config.Root, filePath string) diag.Diagnostics { +func validateFileFormat(configRoot dyn.Value, filePath string) diag.Diagnostics { for _, typ := range resourceTypes { for _, ext := range []string{fmt.Sprintf(".%s.yml", typ), fmt.Sprintf(".%s.yaml", typ)} { if strings.HasSuffix(filePath, ext) { - return validateSingleResourceDefined(r, ext, typ) + return validateSingleResourceDefined(configRoot, ext, typ) } } } @@ -37,7 +37,7 @@ func validateFileFormat(r *config.Root, filePath string) diag.Diagnostics { return nil } -func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnostics { +func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.Diagnostics { type resource struct { path dyn.Path value dyn.Value @@ -49,7 +49,7 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos // Gather all resources defined in the resources block. _, err := dyn.MapByPattern( - r.Value(), + configRoot, dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // The key for the resource. Eg: "my_job" for jobs.my_job. @@ -66,7 +66,7 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos // Gather all resources defined in a target block. _, err = dyn.MapByPattern( - r.Value(), + configRoot, dyn.NewPattern(dyn.Key("targets"), dyn.AnyKey(), dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // The key for the resource. Eg: "my_job" for jobs.my_job. @@ -164,7 +164,7 @@ func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnos } // Add any diagnostics associated with the file format. - diags = append(diags, validateFileFormat(this, m.relPath)...) + diags = append(diags, validateFileFormat(this.Value(), m.relPath)...) if diags.HasError() { return diags } diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 221fb7f63a..664b669ae1 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -376,7 +376,7 @@ func TestValidateFileFormat(t *testing.T) { bundletest.SetLocation(tc.bundle, k, []dyn.Location{v}) } - diags := validateFileFormat(&tc.bundle.Config, tc.fileName) + diags := validateFileFormat(tc.bundle.Config.Value(), tc.fileName) assert.Equal(t, tc.expected, diags) }) } From 4734249854d7eab987cb151b7a524d4fe429b764 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 16:51:46 +0200 Subject: [PATCH 16/35] convert inline tests to files --- bundle/config/loader/process_include_test.go | 431 +++++------------- .../job_and_pipeline.experiment.yml | 11 + .../format_fail/job_and_pipeline.job.yml | 11 + .../format_fail/second_job_in_target.job.yml | 11 + .../format_fail/single_job.pipeline.yaml | 11 + .../testdata/format_fail/two_jobs.job.yml | 7 + .../format_fail/two_jobs_in_target.job.yml | 8 + .../testdata/format_pass/job_and_pipeline.yml | 11 + .../testdata/format_pass/one_job.job.yml | 11 + .../format_pass/one_pipeline.pipeline.yaml | 5 + .../loader/testdata/format_pass/two_job.yml | 7 + 11 files changed, 212 insertions(+), 312 deletions(-) create mode 100644 bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml create mode 100644 bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml create mode 100644 bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml create mode 100644 bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml create mode 100644 bundle/config/loader/testdata/format_fail/two_jobs.job.yml create mode 100644 bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml create mode 100644 bundle/config/loader/testdata/format_pass/job_and_pipeline.yml create mode 100644 bundle/config/loader/testdata/format_pass/one_job.job.yml create mode 100644 bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml create mode 100644 bundle/config/loader/testdata/format_pass/two_job.yml diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 664b669ae1..1b90b19a0b 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -9,8 +9,6 @@ import ( "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/stretchr/testify/assert" @@ -39,345 +37,154 @@ func TestProcessInclude(t *testing.T) { assert.Equal(t, "bar", b.Config.Workspace.Host) } -func TestProcessIncludeValidatesFileFormat(t *testing.T) { - b := &bundle.Bundle{ - RootPath: "testdata/format", - Config: config.Root{ - Bundle: config.Bundle{ - Name: "format_test", - }, - }, - } - - m := ProcessInclude(filepath.Join(b.RootPath, "foo.job.yml"), "foo.job.yml") - diags := bundle.Apply(context.Background(), b, m) - require.NoError(t, diags.Error()) - - // Assert that the diagnostics contain the expected information - assert.Len(t, diags, 1) - assert.Equal(t, diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", - Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 4, Column: 7}, - {File: filepath.FromSlash("testdata/format/foo.job.yml"), Line: 7, Column: 7}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.bar"), - dyn.MustPathFromString("resources.jobs.foo"), - }, - }, - }, diags) -} - -func TestResourceNames(t *testing.T) { - names := []string{} - typ := reflect.TypeOf(config.Resources{}) - for i := 0; i < typ.NumField(); i++ { - field := typ.Field(i) - jsonTags := strings.Split(field.Tag.Get("json"), ",") - singularName := strings.TrimSuffix(jsonTags[0], "s") - names = append(names, singularName) - } - - // Assert the contents of the two lists are equal. Please add the singular - // name of your resource to resourceNames global if you are adding a new - // resource. - assert.Equal(t, len(resourceTypes), len(names)) - for _, name := range names { - assert.Contains(t, resourceTypes, name) - } -} - -func TestValidateFileFormat(t *testing.T) { - onlyJob := config.Root{ - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - }, - }, - Targets: map[string]*config.Target{ - "target1": { - Resources: &config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, +func TestProcessIncludeFormatPass(t *testing.T) { + for _, fileName := range []string{ + "one_job.job.yml", + "one_pipeline.pipeline.yaml", + "two_job.yml", + "job_and_pipeline.yml", + } { + t.Run(fileName, func(t *testing.T) { + b := &bundle.Bundle{ + RootPath: "testdata/format_pass", + Config: config.Root{ + Bundle: config.Bundle{ + Name: "format_test", }, }, - }, - }, - } - onlyJobBundle := bundle.Bundle{Config: onlyJob} + } - onlyPipeline := config.Root{ - Resources: config.Resources{ - Pipelines: map[string]*resources.Pipeline{ - "pipeline1": {}, - }, - }, + m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) + diags := bundle.Apply(context.Background(), b, m) + assert.Empty(t, diags) + }) } - onlyPipelineBundle := bundle.Bundle{Config: onlyPipeline} +} - bothJobAndPipeline := config.Root{ - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - }, - }, - Targets: map[string]*config.Target{ - "target1": { - Resources: &config.Resources{ - Pipelines: map[string]*resources.Pipeline{ - "pipeline1": {}, - }, +func TestProcessIncludeFormatFail(t *testing.T) { + for fileName, expectedDiags := range map[string]diag.Diagnostics{ + "single_job.pipeline.yaml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yaml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/single_job.pipeline.yaml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/single_job.pipeline.yaml"), Line: 4, Column: 7}, }, - }, - }, - } - bothJobAndPipelineBundle := bundle.Bundle{Config: bothJobAndPipeline} - - twoJobs := config.Root{ - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - "job2": {}, - }, - }, - } - twoJobsBundle := bundle.Bundle{Config: twoJobs} - - twoJobsTopLevelAndTarget := config.Root{ - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - }, - }, - Targets: map[string]*config.Target{ - "target1": { - Resources: &config.Resources{ - Jobs: map[string]*resources.Job{ - "job2": {}, - }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), }, }, }, - } - twoJobsTopLevelAndTargetBundle := bundle.Bundle{Config: twoJobsTopLevelAndTarget} - - twoJobsInTarget := config.Root{ - Targets: map[string]*config.Target{ - "target1": { - Resources: &config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {}, - "job2": {}, - }, + "job_and_pipeline.job.yml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 12, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 5, Column: 7}, }, - }, - }, - } - twoJobsInTargetBundle := bundle.Bundle{Config: twoJobsInTarget} - - tcases := []struct { - name string - bundle *bundle.Bundle - expected diag.Diagnostics - fileName string - locations map[string]dyn.Location - }{ - { - name: "single job", - bundle: &onlyJobBundle, - expected: nil, - fileName: "foo.job.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, - }, - }, - { - name: "single pipeline", - bundle: &onlyPipelineBundle, - expected: nil, - fileName: "foo.pipeline.yml", - locations: map[string]dyn.Location{ - "resources.pipelines.pipeline1": {File: "foo.pipeline.yaml", Line: 1, Column: 1}, - }, - }, - { - name: "single job but extension is pipeline", - bundle: &onlyJobBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n", - Locations: []dyn.Location{ - {File: "foo.pipeline.yml", Line: 1, Column: 1}, - {File: "foo.pipeline.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.job1"), - dyn.MustPathFromString("targets.target1.resources.jobs.job1"), - }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.pipelines.pipeline1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), }, }, - fileName: "foo.pipeline.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.pipeline.yml", Line: 1, Column: 1}, - "targets.target1.resources.jobs.job1": {File: "foo.pipeline.yml", Line: 2, Column: 2}, - }, }, - { - name: "job and pipeline", - bundle: &bothJobAndPipelineBundle, - expected: nil, - fileName: "foo.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, - "targets.target1.resources.pipelines.pipeline1": {File: "foo.yml", Line: 2, Column: 2}, - }, - }, - { - name: "job and pipeline but extension is job", - bundle: &bothJobAndPipelineBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", - Locations: []dyn.Location{ - {File: "foo.job.yml", Line: 1, Column: 1}, - {File: "foo.job.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.job1"), - dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), - }, + "job_and_pipeline.experiment.yml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 12, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 5, Column: 7}, }, - }, - fileName: "foo.job.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, - "targets.target1.resources.pipelines.pipeline1": {File: "foo.job.yml", Line: 2, Column: 2}, - }, - }, - { - name: "job and pipeline but extension is experiment", - bundle: &bothJobAndPipelineBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", - Locations: []dyn.Location{ - {File: "foo.experiment.yml", Line: 1, Column: 1}, - {File: "foo.experiment.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.job1"), - dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), - }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.pipelines.pipeline1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), }, }, - fileName: "foo.experiment.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.experiment.yml", Line: 1, Column: 1}, - "targets.target1.resources.pipelines.pipeline1": {File: "foo.experiment.yml", Line: 2, Column: 2}, - }, }, - { - name: "two jobs", - bundle: &twoJobsBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", - Locations: []dyn.Location{ - {File: "foo.job.yml", Line: 1, Column: 1}, - {File: "foo.job.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.job1"), - dyn.MustPathFromString("resources.jobs.job2"), - }, + "two_jobs.job.yml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 5, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 8, Column: 7}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("resources.jobs.job2"), }, - }, - fileName: "foo.job.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, - "resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, }, }, - { - name: "two jobs but extension is simple yaml", - bundle: &twoJobsBundle, - expected: nil, - fileName: "foo.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, - "resources.jobs.job2": {File: "foo.yml", Line: 2, Column: 2}, + "second_job_in_target.job.yml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 12, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 5, Column: 7}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), + }, }, }, - { - name: "two jobs in top level and target", - bundle: &twoJobsTopLevelAndTargetBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", - Locations: []dyn.Location{ - {File: "foo.job.yml", Line: 1, Column: 1}, - {File: "foo.job.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString("resources.jobs.job1"), - dyn.MustPathFromString("targets.target1.resources.jobs.job2"), - }, + "two_jobs_in_target.job.yml": { + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/two_jobs_in_target.job.yml"), Line: 6, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/two_jobs_in_target.job.yml"), Line: 8, Column: 11}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), }, - }, - fileName: "foo.job.yml", - locations: map[string]dyn.Location{ - "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, - "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, }, }, - { - name: "two jobs in target", - bundle: &twoJobsInTargetBundle, - expected: diag.Diagnostics{ - { - Severity: diag.Info, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", - Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", - Locations: []dyn.Location{ - {File: "foo.job.yml", Line: 1, Column: 1}, - {File: "foo.job.yml", Line: 2, Column: 2}, - }, - Paths: []dyn.Path{ - dyn.MustPathFromString(("targets.target1.resources.jobs.job1")), - dyn.MustPathFromString("targets.target1.resources.jobs.job2"), - }, + } { + b := &bundle.Bundle{ + RootPath: "testdata/format_fail", + Config: config.Root{ + Bundle: config.Bundle{ + Name: "format_test", }, }, - fileName: "foo.job.yml", - locations: map[string]dyn.Location{ - "targets.target1.resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, - "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, - }, - }, + } + + m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) + diags := bundle.Apply(context.Background(), b, m) + require.Len(t, diags, 1) + assert.Equal(t, expectedDiags, diags) } +} - for _, tc := range tcases { - t.Run(tc.name, func(t *testing.T) { - for k, v := range tc.locations { - bundletest.SetLocation(tc.bundle, k, []dyn.Location{v}) - } +func TestResourceNames(t *testing.T) { + names := []string{} + typ := reflect.TypeOf(config.Resources{}) + for i := 0; i < typ.NumField(); i++ { + field := typ.Field(i) + jsonTags := strings.Split(field.Tag.Get("json"), ",") + singularName := strings.TrimSuffix(jsonTags[0], "s") + names = append(names, singularName) + } - diags := validateFileFormat(tc.bundle.Config.Value(), tc.fileName) - assert.Equal(t, tc.expected, diags) - }) + // Assert the contents of the two lists are equal. Please add the singular + // name of your resource to resourceNames global if you are adding a new + // resource. + assert.Equal(t, len(resourceTypes), len(names)) + for _, name := range names { + assert.Contains(t, resourceTypes, name) } } diff --git a/bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml b/bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml new file mode 100644 index 0000000000..0867fcae40 --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml @@ -0,0 +1,11 @@ +resources: + pipelines: + pipeline1: + name: pipeline1 + +targets: + target1: + resources: + jobs: + job1: + name: job1 diff --git a/bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml b/bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml new file mode 100644 index 0000000000..0867fcae40 --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml @@ -0,0 +1,11 @@ +resources: + pipelines: + pipeline1: + name: pipeline1 + +targets: + target1: + resources: + jobs: + job1: + name: job1 diff --git a/bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml b/bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml new file mode 100644 index 0000000000..628b9879f8 --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml @@ -0,0 +1,11 @@ +resources: + jobs: + job1: + name: job1 + +targets: + target1: + resources: + jobs: + job2: + name: job2 diff --git a/bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml b/bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml new file mode 100644 index 0000000000..91af87cdca --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml @@ -0,0 +1,11 @@ +resources: + jobs: + job1: + name: job1 + +targets: + target1: + resources: + jobs: + job1: + description: job1 diff --git a/bundle/config/loader/testdata/format_fail/two_jobs.job.yml b/bundle/config/loader/testdata/format_fail/two_jobs.job.yml new file mode 100644 index 0000000000..81ff90a757 --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/two_jobs.job.yml @@ -0,0 +1,7 @@ +resources: + jobs: + job1: + name: job1 + + job2: + name: job2 diff --git a/bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml b/bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml new file mode 100644 index 0000000000..3b489c1f7a --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml @@ -0,0 +1,8 @@ +targets: + target1: + resources: + jobs: + job1: + description: job1 + job2: + description: job2 diff --git a/bundle/config/loader/testdata/format_pass/job_and_pipeline.yml b/bundle/config/loader/testdata/format_pass/job_and_pipeline.yml new file mode 100644 index 0000000000..0867fcae40 --- /dev/null +++ b/bundle/config/loader/testdata/format_pass/job_and_pipeline.yml @@ -0,0 +1,11 @@ +resources: + pipelines: + pipeline1: + name: pipeline1 + +targets: + target1: + resources: + jobs: + job1: + name: job1 diff --git a/bundle/config/loader/testdata/format_pass/one_job.job.yml b/bundle/config/loader/testdata/format_pass/one_job.job.yml new file mode 100644 index 0000000000..91af87cdca --- /dev/null +++ b/bundle/config/loader/testdata/format_pass/one_job.job.yml @@ -0,0 +1,11 @@ +resources: + jobs: + job1: + name: job1 + +targets: + target1: + resources: + jobs: + job1: + description: job1 diff --git a/bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml b/bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml new file mode 100644 index 0000000000..6b16641e96 --- /dev/null +++ b/bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml @@ -0,0 +1,5 @@ +# TODO: Remove all the schema inlined references +resources: + pipelines: + pipeline1: + name: pipeline1 diff --git a/bundle/config/loader/testdata/format_pass/two_job.yml b/bundle/config/loader/testdata/format_pass/two_job.yml new file mode 100644 index 0000000000..81ff90a757 --- /dev/null +++ b/bundle/config/loader/testdata/format_pass/two_job.yml @@ -0,0 +1,7 @@ +resources: + jobs: + job1: + name: job1 + + job2: + name: job2 From d9cf582287f73f059dce81268924e75f2d0f69e1 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:14:43 +0200 Subject: [PATCH 17/35] fix failing tests --- bundle/config/loader/process_include.go | 2 +- bundle/config/loader/process_include_test.go | 52 ++++++++++---------- bundle/render/render_text_output.go | 12 ++--- bundle/render/render_text_output_test.go | 26 +++++----- libs/diag/severity.go | 1 + 5 files changed, 48 insertions(+), 45 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 1e1a62147c..411279d71f 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -131,7 +131,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D return diag.Diagnostics{ { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.", typ, ext), Detail: detail.String(), Locations: locations, diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 1b90b19a0b..b9c84b6d40 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -65,7 +65,7 @@ func TestProcessIncludeFormatFail(t *testing.T) { for fileName, expectedDiags := range map[string]diag.Diagnostics{ "single_job.pipeline.yaml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yaml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n", Locations: []dyn.Location{ @@ -80,12 +80,12 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, "job_and_pipeline.job.yml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 12, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 5, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.pipelines.pipeline1"), @@ -95,12 +95,12 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, "job_and_pipeline.experiment.yml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 12, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 5, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.pipelines.pipeline1"), @@ -110,12 +110,12 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, "two_jobs.job.yml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 5, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 8, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 7, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.job1"), @@ -125,12 +125,12 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, "second_job_in_target.job.yml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 12, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 5, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.job1"), @@ -140,7 +140,7 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, "two_jobs_in_target.job.yml": { { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "We recommend only defining a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ @@ -154,19 +154,21 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, }, } { - b := &bundle.Bundle{ - RootPath: "testdata/format_fail", - Config: config.Root{ - Bundle: config.Bundle{ - Name: "format_test", + t.Run(fileName, func(t *testing.T) { + b := &bundle.Bundle{ + RootPath: "testdata/format_fail", + Config: config.Root{ + Bundle: config.Bundle{ + Name: "format_test", + }, }, - }, - } + } - m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) - diags := bundle.Apply(context.Background(), b, m) - require.Len(t, diags, 1) - assert.Equal(t, expectedDiags, diags) + m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) + diags := bundle.Apply(context.Background(), b, m) + require.Len(t, diags, 1) + assert.Equal(t, expectedDiags, diags) + }) } } diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 529be80735..772781d7ad 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -56,7 +56,7 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} ` -const infoTemplate = `{{ "Info" | blue }}: {{ .Summary }} +const infoTemplate = `{{ "Recommendation" | blue }}: {{ .Summary }} {{- range $index, $element := .Paths }} {{ if eq $index 0 }}at {{else}} {{ end}}{{ $element.String | green }} {{- end }} @@ -108,8 +108,8 @@ func buildTrailer(diags diag.Diagnostics) string { if warnings := len(diags.Filter(diag.Warning)); warnings > 0 { parts = append(parts, color.YellowString(pluralize(warnings, "warning", "warnings"))) } - if infos := len(diags.Filter(diag.Info)); infos > 0 { - parts = append(parts, color.BlueString(pluralize(infos, "info", "infos"))) + if recommendations := len(diags.Filter(diag.Recommendation)); recommendations > 0 { + parts = append(parts, color.BlueString(pluralize(recommendations, "recommendation", "recommendations"))) } if len(parts) > 0 { return fmt.Sprintf("Found %s", strings.Join(parts, " and ")) @@ -147,7 +147,7 @@ func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnosti func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { errorT := template.Must(template.New("error").Funcs(renderFuncMap).Parse(errorTemplate)) warningT := template.Must(template.New("warning").Funcs(renderFuncMap).Parse(warningTemplate)) - infoT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(infoTemplate)) + recommendationT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(infoTemplate)) // Print errors and warnings. for _, d := range diags { @@ -157,8 +157,8 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) t = errorT case diag.Warning: t = warningT - case diag.Info: - t = infoT + case diag.Recommendation: + t = recommendationT } for i := range d.Locations { diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index 8d72298f43..86321e0f17 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -49,14 +49,14 @@ func TestRenderTextOutput(t *testing.T) { name: "nil bundle and 1 info", diags: diag.Diagnostics{ { - Severity: diag.Info, - Summary: "info", + Severity: diag.Recommendation, + Summary: "recommendation", }, }, opts: RenderOptions{RenderSummaryTable: true}, - expected: "Info: info\n" + + expected: "Recommendation: recommendation\n" + "\n" + - "Found 1 info\n", + "Found 1 recommendation\n", }, { name: "bundle during 'load' and 1 error", @@ -119,8 +119,8 @@ func TestRenderTextOutput(t *testing.T) { Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}}, }, diag.Diagnostic{ - Severity: diag.Info, - Summary: "info (4)", + Severity: diag.Recommendation, + Summary: "recommendation (4)", Detail: "detail (4)", Locations: []dyn.Location{{File: "foo.py", Line: 4, Column: 1}}, }, @@ -141,7 +141,7 @@ func TestRenderTextOutput(t *testing.T) { "\n" + "detail (3)\n" + "\n" + - "Info: info (4)\n" + + "Recommendation: recommendation (4)\n" + " in foo.py:4:1\n" + "\n" + "detail (4)\n" + @@ -149,7 +149,7 @@ func TestRenderTextOutput(t *testing.T) { "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 2 errors and 1 warning and 1 info\n", + "Found 2 errors and 1 warning and 1 recommendation\n", }, { name: "bundle during 'init'", @@ -198,8 +198,8 @@ func TestRenderTextOutput(t *testing.T) { Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}}, }, diag.Diagnostic{ - Severity: diag.Info, - Summary: "info (3)", + Severity: diag.Recommendation, + Summary: "recommendation (3)", Detail: "detail (3)", Locations: []dyn.Location{{File: "foo.py", Line: 5, Column: 1}}, }, @@ -215,7 +215,7 @@ func TestRenderTextOutput(t *testing.T) { "\n" + "detail (2)\n" + "\n" + - "Info: info (3)\n" + + "Recommendation: recommendation (3)\n" + " in foo.py:5:1\n" + "\n" + "detail (3)\n" + @@ -343,7 +343,7 @@ func TestRenderDiagnostics(t *testing.T) { name: "info with multiple paths and locations", diags: diag.Diagnostics{ { - Severity: diag.Info, + Severity: diag.Recommendation, Summary: "summary", Detail: "detail", Paths: []dyn.Path{ @@ -356,7 +356,7 @@ func TestRenderDiagnostics(t *testing.T) { }, }, }, - expected: "Info: summary\n" + + expected: "Recommendation: summary\n" + " at resources.jobs.xxx\n" + " resources.jobs.yyy\n" + " in foo.yaml:1:2\n" + diff --git a/libs/diag/severity.go b/libs/diag/severity.go index d25c128069..0e88085f56 100644 --- a/libs/diag/severity.go +++ b/libs/diag/severity.go @@ -6,4 +6,5 @@ const ( Error Severity = iota Warning Info + Recommendation ) From 6040e5826235f4d8b30753aa83e112de7e0868f1 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:23:51 +0200 Subject: [PATCH 18/35] complete info to recommendation --- bundle/render/render_text_output.go | 12 +++- bundle/render/render_text_output_test.go | 76 ++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 772781d7ad..f005e5b476 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -111,9 +111,15 @@ func buildTrailer(diags diag.Diagnostics) string { if recommendations := len(diags.Filter(diag.Recommendation)); recommendations > 0 { parts = append(parts, color.BlueString(pluralize(recommendations, "recommendation", "recommendations"))) } - if len(parts) > 0 { - return fmt.Sprintf("Found %s", strings.Join(parts, " and ")) - } else { + switch { + case len(parts) >= 2: + first := strings.Join(parts[:len(parts)-1], ", ") + last := parts[len(parts)-1] + return fmt.Sprintf("Found %s and %s", first, last) + case len(parts) == 1: + return fmt.Sprintf("Found %s", parts[0]) + default: + // No diagnostics to print. return color.GreenString("Validation OK!") } } diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index 86321e0f17..8921f02f2b 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -46,7 +46,7 @@ func TestRenderTextOutput(t *testing.T) { "Found 1 error\n", }, { - name: "nil bundle and 1 info", + name: "nil bundle and 1 recommendation", diags: diag.Diagnostics{ { Severity: diag.Recommendation, @@ -97,7 +97,7 @@ func TestRenderTextOutput(t *testing.T) { "Found 2 warnings\n", }, { - name: "bundle during 'load' and 2 errors, 1 warning and 1 info with details", + name: "bundle during 'load' and 2 errors, 1 warning and 1 recommendation with details", bundle: loadingBundle, diags: diag.Diagnostics{ diag.Diagnostic{ @@ -149,7 +149,73 @@ func TestRenderTextOutput(t *testing.T) { "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 2 errors and 1 warning and 1 recommendation\n", + "Found 2 errors, 1 warning and 1 recommendation\n", + }, + { + name: "bundle during 'load' and 1 errors, 2 warning and 2 recommendations with details", + bundle: loadingBundle, + diags: diag.Diagnostics{ + diag.Diagnostic{ + Severity: diag.Error, + Summary: "error (1)", + Detail: "detail (1)", + Locations: []dyn.Location{{File: "foo.py", Line: 1, Column: 1}}, + }, + diag.Diagnostic{ + Severity: diag.Warning, + Summary: "warning (2)", + Detail: "detail (2)", + Locations: []dyn.Location{{File: "foo.py", Line: 2, Column: 1}}, + }, + diag.Diagnostic{ + Severity: diag.Warning, + Summary: "warning (3)", + Detail: "detail (3)", + Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}}, + }, + diag.Diagnostic{ + Severity: diag.Recommendation, + Summary: "recommendation (4)", + Detail: "detail (4)", + Locations: []dyn.Location{{File: "foo.py", Line: 4, Column: 1}}, + }, + diag.Diagnostic{ + Severity: diag.Recommendation, + Summary: "recommendation (5)", + Detail: "detail (5)", + Locations: []dyn.Location{{File: "foo.py", Line: 5, Column: 1}}, + }, + }, + opts: RenderOptions{RenderSummaryTable: true}, + expected: "Error: error (1)\n" + + " in foo.py:1:1\n" + + "\n" + + "detail (1)\n" + + "\n" + + "Warning: warning (2)\n" + + " in foo.py:2:1\n" + + "\n" + + "detail (2)\n" + + "\n" + + "Warning: warning (3)\n" + + " in foo.py:3:1\n" + + "\n" + + "detail (3)\n" + + "\n" + + "Recommendation: recommendation (4)\n" + + " in foo.py:4:1\n" + + "\n" + + "detail (4)\n" + + "\n" + + "Recommendation: recommendation (5)\n" + + " in foo.py:5:1\n" + + "\n" + + "detail (5)\n" + + "\n" + + "Name: test-bundle\n" + + "Target: test-target\n" + + "\n" + + "Found 1 error, 2 warnings and 2 recommendations\n", }, { name: "bundle during 'init'", @@ -182,7 +248,7 @@ func TestRenderTextOutput(t *testing.T) { "Validation OK!\n", }, { - name: "nil bundle without summary with 1 error, 1 warning and 1 info", + name: "nil bundle without summary with 1 error, 1 warning and 1 recommendation", bundle: nil, diags: diag.Diagnostics{ diag.Diagnostic{ @@ -340,7 +406,7 @@ func TestRenderDiagnostics(t *testing.T) { "'name' is required\n\n", }, { - name: "info with multiple paths and locations", + name: "recommendation with multiple paths and locations", diags: diag.Diagnostics{ { Severity: diag.Recommendation, From fec73db1116c8e346ca3d73429718481c707318e Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:47:07 +0200 Subject: [PATCH 19/35] address comments --- bundle/config/loader/process_include.go | 24 ++++++-------------- bundle/config/loader/process_include_test.go | 21 ----------------- bundle/config/resources.go | 18 +++++++++++++++ bundle/config/resources_test.go | 16 +++++++++++++ 4 files changed, 41 insertions(+), 38 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 411279d71f..1f8b61d6fb 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -13,23 +13,12 @@ import ( "github.com/databricks/cli/libs/dyn" ) -var resourceTypes = []string{ - "job", - "pipeline", - "model", - "experiment", - "model_serving_endpoint", - "registered_model", - "quality_monitor", - "schema", - "cluster", -} - func validateFileFormat(configRoot dyn.Value, filePath string) diag.Diagnostics { - for _, typ := range resourceTypes { - for _, ext := range []string{fmt.Sprintf(".%s.yml", typ), fmt.Sprintf(".%s.yaml", typ)} { + for _, resourceDescription := range config.SupportedResources() { + singularName := resourceDescription.SingularName + for _, ext := range []string{fmt.Sprintf(".%s.yml", singularName), fmt.Sprintf(".%s.yaml", singularName)} { if strings.HasSuffix(filePath, ext) { - return validateSingleResourceDefined(configRoot, ext, typ) + return validateSingleResourceDefined(configRoot, ext, singularName) } } } @@ -46,6 +35,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D } resources := []resource{} + supportedResources := config.SupportedResources() // Gather all resources defined in the resources block. _, err := dyn.MapByPattern( @@ -55,7 +45,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D // The key for the resource. Eg: "my_job" for jobs.my_job. k := p[2].Key() // The type of the resource. Eg: "job" for jobs.my_job. - typ := strings.TrimSuffix(p[1].Key(), "s") + typ := supportedResources[p[1].Key()].SingularName resources = append(resources, resource{path: p, value: v, typ: typ, key: k}) return v, nil @@ -72,7 +62,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D // The key for the resource. Eg: "my_job" for jobs.my_job. k := p[4].Key() // The type of the resource. Eg: "job" for jobs.my_job. - typ := strings.TrimSuffix(p[3].Key(), "s") + typ := supportedResources[p[3].Key()].SingularName resources = append(resources, resource{path: p, value: v, typ: typ, key: k}) return v, nil diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index b9c84b6d40..dabe66fdcc 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -3,8 +3,6 @@ package loader import ( "context" "path/filepath" - "reflect" - "strings" "testing" "github.com/databricks/cli/bundle" @@ -171,22 +169,3 @@ func TestProcessIncludeFormatFail(t *testing.T) { }) } } - -func TestResourceNames(t *testing.T) { - names := []string{} - typ := reflect.TypeOf(config.Resources{}) - for i := 0; i < typ.NumField(); i++ { - field := typ.Field(i) - jsonTags := strings.Split(field.Tag.Get("json"), ",") - singularName := strings.TrimSuffix(jsonTags[0], "s") - names = append(names, singularName) - } - - // Assert the contents of the two lists are equal. Please add the singular - // name of your resource to resourceNames global if you are adding a new - // resource. - assert.Equal(t, len(resourceTypes), len(names)) - for _, name := range names { - assert.Contains(t, resourceTypes, name) - } -} diff --git a/bundle/config/resources.go b/bundle/config/resources.go index a3afb7fc36..0f16ad40b6 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -59,3 +59,21 @@ func (r *Resources) FindResourceByConfigKey(key string) (ConfigResource, error) return found[0], nil } + +type ResourceDescription struct { + SingularName string +} + +func SupportedResources() map[string]ResourceDescription { + return map[string]ResourceDescription{ + "jobs": {SingularName: "job"}, + "pipelines": {SingularName: "pipeline"}, + "models": {SingularName: "model"}, + "experiments": {SingularName: "experiment"}, + "model_serving_endpoints": {SingularName: "model_serving_endpoint"}, + "registered_models": {SingularName: "registered_model"}, + "quality_monitors": {SingularName: "quality_monitor"}, + "schemas": {SingularName: "schema"}, + "clusters": {SingularName: "cluster"}, + } +} diff --git a/bundle/config/resources_test.go b/bundle/config/resources_test.go index 6860d73daa..c1b76118ce 100644 --- a/bundle/config/resources_test.go +++ b/bundle/config/resources_test.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" "reflect" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -61,3 +62,18 @@ func TestCustomMarshallerIsImplemented(t *testing.T) { }, "Resource %s does not have a custom unmarshaller", field.Name) } } + +func TestSupportedResources(t *testing.T) { + expected := map[string]ResourceDescription{} + typ := reflect.TypeOf(Resources{}) + for i := 0; i < typ.NumField(); i++ { + field := typ.Field(i) + jsonTags := strings.Split(field.Tag.Get("json"), ",") + singularName := strings.TrimSuffix(jsonTags[0], "s") + expected[jsonTags[0]] = ResourceDescription{SingularName: singularName} + } + + // Please add your resource to the SupportedResources() function in resources.go + // if you are adding a new resource. + assert.Equal(t, expected, SupportedResources()) +} From 32038580327ec2257779b703b97be320645c36fe Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:48:15 +0200 Subject: [PATCH 20/35] add comment --- bundle/config/resources.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 0f16ad40b6..dc51a7cafb 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -64,6 +64,7 @@ type ResourceDescription struct { SingularName string } +// The keys of the map corresponds to the resource key in the bundle configuration. func SupportedResources() map[string]ResourceDescription { return map[string]ResourceDescription{ "jobs": {SingularName: "job"}, From 28917ebdab3f925c8c66117b9a4d68f68585f1f4 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:55:09 +0200 Subject: [PATCH 21/35] - --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 1f8b61d6fb..060b7e331f 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -95,7 +95,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D for _, r := range resources { lines = append(lines, fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)) } - // Sort the line s to print to make the output deterministic. + // Sort the lines to print to make the output deterministic. sort.Strings(lines) // Compact the lines before writing them to the message to remove any duplicate lines. // This is needed because we do not dedup earlier when gathering the resources From 4373e7b513eaa70c509f7cd8cb6f91d896a42c94 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 26 Sep 2024 17:57:24 +0200 Subject: [PATCH 22/35] make unit test package separate --- bundle/config/loader/process_include_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index dabe66fdcc..b052fe8530 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -1,4 +1,4 @@ -package loader +package loader_test import ( "context" @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/loader" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" @@ -23,7 +24,7 @@ func TestProcessInclude(t *testing.T) { }, } - m := ProcessInclude(filepath.Join(b.RootPath, "host.yml"), "host.yml") + m := loader.ProcessInclude(filepath.Join(b.RootPath, "host.yml"), "host.yml") assert.Equal(t, "ProcessInclude(host.yml)", m.Name()) // Assert the host value prior to applying the mutator @@ -52,7 +53,7 @@ func TestProcessIncludeFormatPass(t *testing.T) { }, } - m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) + m := loader.ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) diags := bundle.Apply(context.Background(), b, m) assert.Empty(t, diags) }) @@ -162,7 +163,7 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, } - m := ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) + m := loader.ProcessInclude(filepath.Join(b.RootPath, fileName), fileName) diags := bundle.Apply(context.Background(), b, m) require.Len(t, diags, 1) assert.Equal(t, expectedDiags, diags) From 65ad2f0557011552a3e98370a9c844ed1af7feb5 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 12:04:33 +0200 Subject: [PATCH 23/35] add test for multiple resources --- bundle/config/loader/process_include_test.go | 46 +++++++++++++++++++ ...tiple_resources.model_serving_endpoint.yml | 43 +++++++++++++++++ .../format_pass/multiple_resources.yml | 43 +++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml create mode 100644 bundle/config/loader/testdata/format_pass/multiple_resources.yml diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index b052fe8530..85adac2d90 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -42,6 +42,7 @@ func TestProcessIncludeFormatPass(t *testing.T) { "one_pipeline.pipeline.yaml", "two_job.yml", "job_and_pipeline.yml", + "multiple_resources.yml", } { t.Run(fileName, func(t *testing.T) { b := &bundle.Bundle{ @@ -152,6 +153,51 @@ func TestProcessIncludeFormatFail(t *testing.T) { }, }, }, + "multiple_resources.model_serving_endpoint.yml": { + { + Severity: diag.Recommendation, + Summary: "We recommend only defining a single model_serving_endpoint in a file with the .model_serving_endpoint.yml extension.", + Detail: `The following resources are defined or configured in this file: + - experiment1 (experiment) + - job1 (job) + - job2 (job) + - job3 (job) + - model1 (model) + - model_serving_endpoint1 (model_serving_endpoint) + - pipeline1 (pipeline) + - pipeline2 (pipeline) + - quality_monitor1 (quality_monitor) + - registered_model1 (registered_model) + - schema1 (schema) +`, + Locations: []dyn.Location{ + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 12, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 14, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 18, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 22, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 24, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 28, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 35, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 39, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 43, Column: 11}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 8, Column: 7}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.experiments.experiment1"), + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("resources.jobs.job2"), + dyn.MustPathFromString("resources.model_serving_endpoints.model_serving_endpoint1"), + dyn.MustPathFromString("resources.models.model1"), + dyn.MustPathFromString("resources.pipelines.pipeline1"), + dyn.MustPathFromString("resources.pipelines.pipeline2"), + dyn.MustPathFromString("resources.schemas.schema1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job3"), + dyn.MustPathFromString("targets.target1.resources.quality_monitors.quality_monitor1"), + dyn.MustPathFromString("targets.target1.resources.registered_models.registered_model1"), + }, + }, + }, } { t.Run(fileName, func(t *testing.T) { b := &bundle.Bundle{ diff --git a/bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml b/bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml new file mode 100644 index 0000000000..dc8e837c62 --- /dev/null +++ b/bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml @@ -0,0 +1,43 @@ +resources: + experiments: + experiment1: + name: experiment1 + + model_serving_endpoints: + model_serving_endpoint1: + name: model_serving_endpoint1 + + jobs: + job1: + name: job1 + job2: + name: job2 + + models: + model1: + name: model1 + + pipelines: + pipeline1: + name: pipeline1 + pipeline2: + name: pipeline2 + + schemas: + schema1: + name: schema1 + +targets: + target1: + resources: + quality_monitors: + quality_monitor1: + baseline_table_name: quality_monitor1 + + jobs: + job3: + name: job3 + + registered_models: + registered_model1: + name: registered_model1 diff --git a/bundle/config/loader/testdata/format_pass/multiple_resources.yml b/bundle/config/loader/testdata/format_pass/multiple_resources.yml new file mode 100644 index 0000000000..dc8e837c62 --- /dev/null +++ b/bundle/config/loader/testdata/format_pass/multiple_resources.yml @@ -0,0 +1,43 @@ +resources: + experiments: + experiment1: + name: experiment1 + + model_serving_endpoints: + model_serving_endpoint1: + name: model_serving_endpoint1 + + jobs: + job1: + name: job1 + job2: + name: job2 + + models: + model1: + name: model1 + + pipelines: + pipeline1: + name: pipeline1 + pipeline2: + name: pipeline2 + + schemas: + schema1: + name: schema1 + +targets: + target1: + resources: + quality_monitors: + quality_monitor1: + baseline_table_name: quality_monitor1 + + jobs: + job3: + name: job3 + + registered_models: + registered_model1: + name: registered_model1 From b4bd47f4ba2335d849c3c37a5e54ef7e1320137c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 12:55:54 +0200 Subject: [PATCH 24/35] fail -> not match --- bundle/config/loader/process_include_test.go | 8 ++++---- .../{format_pass => format_match}/job_and_pipeline.yml | 0 .../{format_pass => format_match}/multiple_resources.yml | 0 .../{format_pass => format_match}/one_job.job.yml | 0 .../one_pipeline.pipeline.yaml | 0 .../testdata/{format_pass => format_match}/two_job.yml | 0 .../job_and_pipeline.experiment.yml | 0 .../job_and_pipeline.job.yml | 0 .../multiple_resources.model_serving_endpoint.yml | 0 .../second_job_in_target.job.yml | 0 .../single_job.pipeline.yaml | 0 .../{format_fail => format_not_match}/two_jobs.job.yml | 0 .../two_jobs_in_target.job.yml | 0 13 files changed, 4 insertions(+), 4 deletions(-) rename bundle/config/loader/testdata/{format_pass => format_match}/job_and_pipeline.yml (100%) rename bundle/config/loader/testdata/{format_pass => format_match}/multiple_resources.yml (100%) rename bundle/config/loader/testdata/{format_pass => format_match}/one_job.job.yml (100%) rename bundle/config/loader/testdata/{format_pass => format_match}/one_pipeline.pipeline.yaml (100%) rename bundle/config/loader/testdata/{format_pass => format_match}/two_job.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/job_and_pipeline.experiment.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/job_and_pipeline.job.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/multiple_resources.model_serving_endpoint.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/second_job_in_target.job.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/single_job.pipeline.yaml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/two_jobs.job.yml (100%) rename bundle/config/loader/testdata/{format_fail => format_not_match}/two_jobs_in_target.job.yml (100%) diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 85adac2d90..7f2ff2a440 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -36,7 +36,7 @@ func TestProcessInclude(t *testing.T) { assert.Equal(t, "bar", b.Config.Workspace.Host) } -func TestProcessIncludeFormatPass(t *testing.T) { +func TestProcessIncludeFormatMatch(t *testing.T) { for _, fileName := range []string{ "one_job.job.yml", "one_pipeline.pipeline.yaml", @@ -46,7 +46,7 @@ func TestProcessIncludeFormatPass(t *testing.T) { } { t.Run(fileName, func(t *testing.T) { b := &bundle.Bundle{ - RootPath: "testdata/format_pass", + RootPath: "testdata/format_match", Config: config.Root{ Bundle: config.Bundle{ Name: "format_test", @@ -61,7 +61,7 @@ func TestProcessIncludeFormatPass(t *testing.T) { } } -func TestProcessIncludeFormatFail(t *testing.T) { +func TestProcessIncludeFormatNotMatch(t *testing.T) { for fileName, expectedDiags := range map[string]diag.Diagnostics{ "single_job.pipeline.yaml": { { @@ -201,7 +201,7 @@ func TestProcessIncludeFormatFail(t *testing.T) { } { t.Run(fileName, func(t *testing.T) { b := &bundle.Bundle{ - RootPath: "testdata/format_fail", + RootPath: "testdata/format_not_match", Config: config.Root{ Bundle: config.Bundle{ Name: "format_test", diff --git a/bundle/config/loader/testdata/format_pass/job_and_pipeline.yml b/bundle/config/loader/testdata/format_match/job_and_pipeline.yml similarity index 100% rename from bundle/config/loader/testdata/format_pass/job_and_pipeline.yml rename to bundle/config/loader/testdata/format_match/job_and_pipeline.yml diff --git a/bundle/config/loader/testdata/format_pass/multiple_resources.yml b/bundle/config/loader/testdata/format_match/multiple_resources.yml similarity index 100% rename from bundle/config/loader/testdata/format_pass/multiple_resources.yml rename to bundle/config/loader/testdata/format_match/multiple_resources.yml diff --git a/bundle/config/loader/testdata/format_pass/one_job.job.yml b/bundle/config/loader/testdata/format_match/one_job.job.yml similarity index 100% rename from bundle/config/loader/testdata/format_pass/one_job.job.yml rename to bundle/config/loader/testdata/format_match/one_job.job.yml diff --git a/bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml b/bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml similarity index 100% rename from bundle/config/loader/testdata/format_pass/one_pipeline.pipeline.yaml rename to bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml diff --git a/bundle/config/loader/testdata/format_pass/two_job.yml b/bundle/config/loader/testdata/format_match/two_job.yml similarity index 100% rename from bundle/config/loader/testdata/format_pass/two_job.yml rename to bundle/config/loader/testdata/format_match/two_job.yml diff --git a/bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml b/bundle/config/loader/testdata/format_not_match/job_and_pipeline.experiment.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/job_and_pipeline.experiment.yml rename to bundle/config/loader/testdata/format_not_match/job_and_pipeline.experiment.yml diff --git a/bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml b/bundle/config/loader/testdata/format_not_match/job_and_pipeline.job.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/job_and_pipeline.job.yml rename to bundle/config/loader/testdata/format_not_match/job_and_pipeline.job.yml diff --git a/bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml b/bundle/config/loader/testdata/format_not_match/multiple_resources.model_serving_endpoint.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/multiple_resources.model_serving_endpoint.yml rename to bundle/config/loader/testdata/format_not_match/multiple_resources.model_serving_endpoint.yml diff --git a/bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml b/bundle/config/loader/testdata/format_not_match/second_job_in_target.job.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/second_job_in_target.job.yml rename to bundle/config/loader/testdata/format_not_match/second_job_in_target.job.yml diff --git a/bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml b/bundle/config/loader/testdata/format_not_match/single_job.pipeline.yaml similarity index 100% rename from bundle/config/loader/testdata/format_fail/single_job.pipeline.yaml rename to bundle/config/loader/testdata/format_not_match/single_job.pipeline.yaml diff --git a/bundle/config/loader/testdata/format_fail/two_jobs.job.yml b/bundle/config/loader/testdata/format_not_match/two_jobs.job.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/two_jobs.job.yml rename to bundle/config/loader/testdata/format_not_match/two_jobs.job.yml diff --git a/bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml b/bundle/config/loader/testdata/format_not_match/two_jobs_in_target.job.yml similarity index 100% rename from bundle/config/loader/testdata/format_fail/two_jobs_in_target.job.yml rename to bundle/config/loader/testdata/format_not_match/two_jobs_in_target.job.yml From dd28a56b7dd9c3659917fe86a08d24174fb1f0a0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 12:56:53 +0200 Subject: [PATCH 25/35] address comments --- bundle/render/render_text_output.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index f005e5b476..8223d387fd 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -56,7 +56,7 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} ` -const infoTemplate = `{{ "Recommendation" | blue }}: {{ .Summary }} +const recommendationTemplate = `{{ "Recommendation" | blue }}: {{ .Summary }} {{- range $index, $element := .Paths }} {{ if eq $index 0 }}at {{else}} {{ end}}{{ $element.String | green }} {{- end }} @@ -153,7 +153,7 @@ func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnosti func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { errorT := template.Must(template.New("error").Funcs(renderFuncMap).Parse(errorTemplate)) warningT := template.Must(template.New("warning").Funcs(renderFuncMap).Parse(warningTemplate)) - recommendationT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(infoTemplate)) + recommendationT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(recommendationTemplate)) // Print errors and warnings. for _, d := range diags { From 14e73acb118058d806f82652cd9bfad02c75d7ee Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 12:57:49 +0200 Subject: [PATCH 26/35] - --- bundle/config/loader/testdata/format/databricks.yml | 2 -- bundle/config/loader/testdata/format/foo.job.yml | 7 ------- 2 files changed, 9 deletions(-) delete mode 100644 bundle/config/loader/testdata/format/databricks.yml delete mode 100644 bundle/config/loader/testdata/format/foo.job.yml diff --git a/bundle/config/loader/testdata/format/databricks.yml b/bundle/config/loader/testdata/format/databricks.yml deleted file mode 100644 index a621514c88..0000000000 --- a/bundle/config/loader/testdata/format/databricks.yml +++ /dev/null @@ -1,2 +0,0 @@ -bundle: - name: format_test diff --git a/bundle/config/loader/testdata/format/foo.job.yml b/bundle/config/loader/testdata/format/foo.job.yml deleted file mode 100644 index 591aaa08c7..0000000000 --- a/bundle/config/loader/testdata/format/foo.job.yml +++ /dev/null @@ -1,7 +0,0 @@ -resources: - jobs: - foo: - name: foo - - bar: - name: bar From e22ba7506267907e698a535b71f8d42dcd5a738c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 12:58:33 +0200 Subject: [PATCH 27/35] remove todo --- .../loader/testdata/format_match/one_pipeline.pipeline.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml b/bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml index 6b16641e96..85cb0d7fc2 100644 --- a/bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml +++ b/bundle/config/loader/testdata/format_match/one_pipeline.pipeline.yaml @@ -1,4 +1,3 @@ -# TODO: Remove all the schema inlined references resources: pipelines: pipeline1: From 367048bde5c2dc76224a7272921a9751c21fb765 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 13:36:22 +0200 Subject: [PATCH 28/35] account for the oxford comma --- bundle/render/render_text_output.go | 6 ++-- bundle/render/render_text_output_test.go | 37 ++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 8223d387fd..2a7f2214f9 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -112,10 +112,12 @@ func buildTrailer(diags diag.Diagnostics) string { parts = append(parts, color.BlueString(pluralize(recommendations, "recommendation", "recommendations"))) } switch { - case len(parts) >= 2: + case len(parts) >= 3: first := strings.Join(parts[:len(parts)-1], ", ") last := parts[len(parts)-1] - return fmt.Sprintf("Found %s and %s", first, last) + return fmt.Sprintf("Found %s, and %s", first, last) + case len(parts) == 2: + return fmt.Sprintf("Found %s and %s", parts[0], parts[1]) case len(parts) == 1: return fmt.Sprintf("Found %s", parts[0]) default: diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index 8921f02f2b..1a41fa01c4 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -149,7 +149,40 @@ func TestRenderTextOutput(t *testing.T) { "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 2 errors, 1 warning and 1 recommendation\n", + "Found 2 errors, 1 warning, and 1 recommendation\n", + }, + { + name: "bundle during 'load' and 1 error and 1 warning", + bundle: loadingBundle, + diags: diag.Diagnostics{ + diag.Diagnostic{ + Severity: diag.Error, + Summary: "error (1)", + Detail: "detail (1)", + Locations: []dyn.Location{{File: "foo.py", Line: 1, Column: 1}}, + }, + diag.Diagnostic{ + Severity: diag.Warning, + Summary: "warning (2)", + Detail: "detail (2)", + Locations: []dyn.Location{{File: "foo.py", Line: 2, Column: 1}}, + }, + }, + opts: RenderOptions{RenderSummaryTable: true}, + expected: "Error: error (1)\n" + + " in foo.py:1:1\n" + + "\n" + + "detail (1)\n" + + "\n" + + "Warning: warning (2)\n" + + " in foo.py:2:1\n" + + "\n" + + "detail (2)\n" + + "\n" + + "Name: test-bundle\n" + + "Target: test-target\n" + + "\n" + + "Found 1 error and 1 warning\n", }, { name: "bundle during 'load' and 1 errors, 2 warning and 2 recommendations with details", @@ -215,7 +248,7 @@ func TestRenderTextOutput(t *testing.T) { "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 1 error, 2 warnings and 2 recommendations\n", + "Found 1 error, 2 warnings, and 2 recommendations\n", }, { name: "bundle during 'init'", From dd20a5274341cc04646bbe67832721a22dcfbbe0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 13:42:48 +0200 Subject: [PATCH 29/35] terser recommendation message --- bundle/config/loader/process_include.go | 2 +- bundle/config/loader/process_include_test.go | 60 ++++++++++---------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 060b7e331f..32fdb94657 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -122,7 +122,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D return diag.Diagnostics{ { Severity: diag.Recommendation, - Summary: fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.", typ, ext), + Summary: fmt.Sprintf("define a single %s in a file with the %s extension.", strings.ReplaceAll(typ, "_", " "), ext), Detail: detail.String(), Locations: locations, Paths: paths, diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 7f2ff2a440..16ac581617 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -66,11 +66,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "single_job.pipeline.yaml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yaml extension.", + Summary: "define a single pipeline in a file with the .pipeline.yaml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/single_job.pipeline.yaml"), Line: 11, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/single_job.pipeline.yaml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/single_job.pipeline.yaml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/single_job.pipeline.yaml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.job1"), @@ -81,11 +81,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "job_and_pipeline.job.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Summary: "define a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 11, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.job.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/job_and_pipeline.job.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/job_and_pipeline.job.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.pipelines.pipeline1"), @@ -96,11 +96,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "job_and_pipeline.experiment.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.", + Summary: "define a single experiment in a file with the .experiment.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 11, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/job_and_pipeline.experiment.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/job_and_pipeline.experiment.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/job_and_pipeline.experiment.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.pipelines.pipeline1"), @@ -111,11 +111,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "two_jobs.job.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Summary: "define a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 4, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/two_jobs.job.yml"), Line: 7, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/two_jobs.job.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/two_jobs.job.yml"), Line: 7, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.job1"), @@ -126,11 +126,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "second_job_in_target.job.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Summary: "define a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 11, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/second_job_in_target.job.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/second_job_in_target.job.yml"), Line: 11, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/second_job_in_target.job.yml"), Line: 4, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.jobs.job1"), @@ -141,11 +141,11 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "two_jobs_in_target.job.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single job in a file with the .job.yml extension.", + Summary: "define a single job in a file with the .job.yml extension.", Detail: "The following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/two_jobs_in_target.job.yml"), Line: 6, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/two_jobs_in_target.job.yml"), Line: 8, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/two_jobs_in_target.job.yml"), Line: 6, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/two_jobs_in_target.job.yml"), Line: 8, Column: 11}, }, Paths: []dyn.Path{ dyn.MustPathFromString("targets.target1.resources.jobs.job1"), @@ -156,7 +156,7 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { "multiple_resources.model_serving_endpoint.yml": { { Severity: diag.Recommendation, - Summary: "We recommend only defining a single model_serving_endpoint in a file with the .model_serving_endpoint.yml extension.", + Summary: "define a single model serving endpoint in a file with the .model_serving_endpoint.yml extension.", Detail: `The following resources are defined or configured in this file: - experiment1 (experiment) - job1 (job) @@ -171,17 +171,17 @@ func TestProcessIncludeFormatNotMatch(t *testing.T) { - schema1 (schema) `, Locations: []dyn.Location{ - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 12, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 14, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 18, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 22, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 24, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 28, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 35, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 39, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 43, Column: 11}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 4, Column: 7}, - {File: filepath.FromSlash("testdata/format_fail/multiple_resources.model_serving_endpoint.yml"), Line: 8, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 12, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 14, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 18, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 22, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 24, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 28, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 35, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 39, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 43, Column: 11}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 4, Column: 7}, + {File: filepath.FromSlash("testdata/format_not_match/multiple_resources.model_serving_endpoint.yml"), Line: 8, Column: 7}, }, Paths: []dyn.Path{ dyn.MustPathFromString("resources.experiments.experiment1"), From 90f360b08b8ddb0d4a7c926faf5d2a6eb7f61b45 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 27 Sep 2024 13:50:18 +0200 Subject: [PATCH 30/35] - --- bundle/render/render_text_output.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 23c1a32461..56387c3868 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -155,7 +155,7 @@ func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnosti func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { errorT := template.Must(template.New("error").Funcs(renderFuncMap).Parse(errorTemplate)) warningT := template.Must(template.New("warning").Funcs(renderFuncMap).Parse(warningTemplate)) - recommendationT := template.Must(template.New("info").Funcs(renderFuncMap).Parse(recommendationTemplate)) + recommendationT := template.Must(template.New("recommendation").Funcs(renderFuncMap).Parse(recommendationTemplate)) // Print errors and warnings. for _, d := range diags { From a737977d6f1b2405dbb02028626d6a4ec8e15d3f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 2 Oct 2024 20:26:42 +0200 Subject: [PATCH 31/35] address comments --- bundle/config/loader/process_include.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 32fdb94657..3c802141b2 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -16,7 +16,9 @@ import ( func validateFileFormat(configRoot dyn.Value, filePath string) diag.Diagnostics { for _, resourceDescription := range config.SupportedResources() { singularName := resourceDescription.SingularName - for _, ext := range []string{fmt.Sprintf(".%s.yml", singularName), fmt.Sprintf(".%s.yaml", singularName)} { + + for _, yamlExt := range []string{"yml", "yaml"} { + ext := fmt.Sprintf(".%s.%s", singularName, yamlExt) if strings.HasSuffix(filePath, ext) { return validateSingleResourceDefined(configRoot, ext, singularName) } From d064566fc9b617cba1fcc9b4d0f421f76a66f77c Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 2 Oct 2024 23:57:12 +0530 Subject: [PATCH 32/35] Update bundle/config/loader/process_include.go Co-authored-by: Lennart Kats (databricks) --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 3c802141b2..81b4d99cb0 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -44,7 +44,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D configRoot, dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - // The key for the resource. Eg: "my_job" for jobs.my_job. + // The key for the resource, e.g. "my_job" for jobs.my_job. k := p[2].Key() // The type of the resource. Eg: "job" for jobs.my_job. typ := supportedResources[p[1].Key()].SingularName From b288b0701df394e3f656d727dbffb63c8c02e94b Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 2 Oct 2024 23:57:20 +0530 Subject: [PATCH 33/35] Update bundle/config/loader/process_include.go Co-authored-by: Lennart Kats (databricks) --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 81b4d99cb0..f190cfa734 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -46,7 +46,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // The key for the resource, e.g. "my_job" for jobs.my_job. k := p[2].Key() - // The type of the resource. Eg: "job" for jobs.my_job. + // The type of the resource, e.g. "job" for jobs.my_job. typ := supportedResources[p[1].Key()].SingularName resources = append(resources, resource{path: p, value: v, typ: typ, key: k}) From b1d7c7bdb7840c587f53332aeda7cd8e4940f8b7 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 2 Oct 2024 23:57:26 +0530 Subject: [PATCH 34/35] Update bundle/config/loader/process_include.go Co-authored-by: Lennart Kats (databricks) --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index f190cfa734..7f4a1f4ce5 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -61,7 +61,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D configRoot, dyn.NewPattern(dyn.Key("targets"), dyn.AnyKey(), dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - // The key for the resource. Eg: "my_job" for jobs.my_job. + // The key for the resource, e.g. "my_job" for jobs.my_job. k := p[4].Key() // The type of the resource. Eg: "job" for jobs.my_job. typ := supportedResources[p[3].Key()].SingularName From 361f59087e1e4f39b37ea92fab4751231bf303aa Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 2 Oct 2024 23:58:10 +0530 Subject: [PATCH 35/35] Update bundle/config/loader/process_include.go Co-authored-by: Lennart Kats (databricks) --- bundle/config/loader/process_include.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 7f4a1f4ce5..f82f5db1ec 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -63,7 +63,7 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // The key for the resource, e.g. "my_job" for jobs.my_job. k := p[4].Key() - // The type of the resource. Eg: "job" for jobs.my_job. + // The type of the resource, e.g. "job" for jobs.my_job. typ := supportedResources[p[3].Key()].SingularName resources = append(resources, resource{path: p, value: v, typ: typ, key: k})