From 12ad79c7169efbb7616e0da70d7f1a395f6c63f3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 02:47:40 +0200 Subject: [PATCH 1/9] Detect duplicate identifiers in bundle config --- bundle/config/root.go | 49 ++++++++++++++++++++++++++++++++++++++ bundle/config/root_test.go | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/bundle/config/root.go b/bundle/config/root.go index 50e223128b..b9375c9339 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "path/filepath" @@ -79,6 +80,32 @@ func (r *Root) SetConfigFilePath(path string) { } } +func (r *Root) listIds() (map[string]struct{}, error) { + result := map[string]struct{}{} + for k, _ := range r.Resources.Jobs { + result[k] = struct{}{} + } + for k, _ := range r.Resources.Pipelines { + if _, ok := result[k]; ok { + return nil, fmt.Errorf("duplicate identifier %s", k) + } + result[k] = struct{}{} + } + for k, _ := range r.Resources.Models { + if _, ok := result[k]; ok { + return nil, fmt.Errorf("duplicate identifier %s", k) + } + result[k] = struct{}{} + } + for k, _ := range r.Resources.Experiments { + if _, ok := result[k]; ok { + return nil, fmt.Errorf("duplicate identifier %s", k) + } + result[k] = struct{}{} + } + return result, nil +} + func (r *Root) Load(path string) error { raw, err := os.ReadFile(path) if err != nil { @@ -88,6 +115,13 @@ func (r *Root) Load(path string) error { if err != nil { return err } + + // check there are no duplicate identifiers in the config instance + _, err = r.listIds() + if err != nil { + return err + } + r.Path = filepath.Dir(path) r.SetConfigFilePath(path) return nil @@ -97,6 +131,21 @@ func (r *Root) Merge(other *Root) error { // TODO: when hooking into merge semantics, disallow setting path on the target instance. other.Path = "" + // Check for duplicate resource identifiers + rootIds, err := r.listIds() + if err != nil { + return err + } + otherIds, err := other.listIds() + if err != nil { + return err + } + for k := range rootIds { + if _, ok := otherIds[k]; ok { + return fmt.Errorf("duplicate identifier %s", k) + } + } + // TODO: define and test semantics for merging. return mergo.MergeWithOverwrite(r, other) } diff --git a/bundle/config/root_test.go b/bundle/config/root_test.go index 70a1e13565..d412999c53 100644 --- a/bundle/config/root_test.go +++ b/bundle/config/root_test.go @@ -2,9 +2,12 @@ package config import ( "encoding/json" + "os" + "path/filepath" "reflect" "testing" + "github.com/databricks/bricks/bundle/config/resources" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -74,3 +77,48 @@ func TestRootMergeMap(t *testing.T) { assert.NoError(t, root.Merge(other)) assert.Equal(t, &Workspace{Host: "bar", Profile: "profile"}, root.Environments["development"].Workspace) } + +func TestDuplicateIdOnLoadReturnsError(t *testing.T) { + dir := t.TempDir() + root := &Root{ + Resources: Resources{ + Jobs: map[string]*resources.Job{ + "foo": {}, + "bar": {}, + }, + Pipelines: map[string]*resources.Pipeline{ + "foo": {}, + }, + }, + } + b, err := json.Marshal(root) + require.NoError(t, err) + + err = os.WriteFile(filepath.Join(dir, "bundle.yml"), b, os.ModePerm) + require.NoError(t, err) + + err = root.Load(filepath.Join(dir, "bundle.yml")) + assert.ErrorContains(t, err, "duplicate identifier foo") +} + +func TestRootMergeDetectsDuplicateIds(t *testing.T) { + root := &Root{ + Resources: Resources{ + Jobs: map[string]*resources.Job{ + "foo": {}, + }, + Pipelines: map[string]*resources.Pipeline{ + "bar": {}, + }, + }, + } + other := &Root{ + Resources: Resources{ + Models: map[string]*resources.MlflowModel{ + "bar": {}, + }, + }, + } + err := root.Merge(other) + assert.ErrorContains(t, err, "duplicate identifier bar") +} From 4f44a5da7743a0b4a317a6618a26e017084eaaf5 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 02:52:16 +0200 Subject: [PATCH 2/9] lint --- bundle/config/root.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index b9375c9339..2c0549305a 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -82,22 +82,22 @@ func (r *Root) SetConfigFilePath(path string) { func (r *Root) listIds() (map[string]struct{}, error) { result := map[string]struct{}{} - for k, _ := range r.Resources.Jobs { + for k := range r.Resources.Jobs { result[k] = struct{}{} } - for k, _ := range r.Resources.Pipelines { + for k := range r.Resources.Pipelines { if _, ok := result[k]; ok { return nil, fmt.Errorf("duplicate identifier %s", k) } result[k] = struct{}{} } - for k, _ := range r.Resources.Models { + for k := range r.Resources.Models { if _, ok := result[k]; ok { return nil, fmt.Errorf("duplicate identifier %s", k) } result[k] = struct{}{} } - for k, _ := range r.Resources.Experiments { + for k := range r.Resources.Experiments { if _, ok := result[k]; ok { return nil, fmt.Errorf("duplicate identifier %s", k) } From 82cbdbb0134f3da8d9fd41ab18775d422a7cb7a0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 13:57:26 +0200 Subject: [PATCH 3/9] address comments --- bundle/config/resources.go | 84 ++++++++++++++++ bundle/config/resources_test.go | 96 +++++++++++++++++++ bundle/config/root.go | 23 +---- bundle/config/root_test.go | 30 +++--- .../bundle.yml | 10 ++ .../resources.yml | 4 + .../bundle.yml | 13 +++ 7 files changed, 224 insertions(+), 36 deletions(-) create mode 100644 bundle/config/resources_test.go create mode 100644 bundle/config/testdata/duplicate_resource_name_in_subconfiguration/bundle.yml create mode 100644 bundle/config/testdata/duplicate_resource_name_in_subconfiguration/resources.yml create mode 100644 bundle/config/testdata/duplicate_resource_names_in_root/bundle.yml diff --git a/bundle/config/resources.go b/bundle/config/resources.go index d11610cfc4..1bbbeedb02 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -1,6 +1,8 @@ package config import ( + "fmt" + "github.com/databricks/bricks/bundle/config/resources" ) @@ -13,6 +15,88 @@ type Resources struct { Experiments map[string]*resources.MlflowExperiment `json:"experiments,omitempty"` } +type UniqueResourceIdTracker struct { + Type map[string]string + ConfigPath map[string]string +} + +// verifies merging is safe by checking no duplicate identifiers exist +func (r *Resources) VerifySafeMerge(other *Resources) error { + rootTracker, err := r.VerifyUniqueResourceIdentifiers() + if err != nil { + return err + } + otherTracker, err := other.VerifyUniqueResourceIdentifiers() + if err != nil { + return err + } + for k := range otherTracker.Type { + if _, ok := rootTracker.Type[k]; ok { + return fmt.Errorf("multiple resources named %s (%s at %s, %s at %s)", + k, + rootTracker.Type[k], + rootTracker.ConfigPath[k], + otherTracker.Type[k], + otherTracker.ConfigPath[k], + ) + } + } + return nil +} + +// This function verifies there are no duplicate names used for the resource definations +func (r *Resources) VerifyUniqueResourceIdentifiers() (*UniqueResourceIdTracker, error) { + tracker := &UniqueResourceIdTracker{ + Type: make(map[string]string), + ConfigPath: make(map[string]string), + } + for k := range r.Jobs { + tracker.Type[k] = "job" + tracker.ConfigPath[k] = r.Jobs[k].ConfigFilePath + } + for k := range r.Pipelines { + if _, ok := tracker.Type[k]; ok { + + return tracker, fmt.Errorf("multiple resources named %s (%s at %s, %s at %s)", + k, + tracker.Type[k], + tracker.ConfigPath[k], + "pipeline", + r.Pipelines[k].ConfigFilePath, + ) + } + tracker.Type[k] = "pipeline" + tracker.ConfigPath[k] = r.Pipelines[k].ConfigFilePath + } + for k := range r.Models { + if _, ok := tracker.Type[k]; ok { + return tracker, fmt.Errorf("multiple resources named %s (%s at %s, %s at %s)", + k, + tracker.Type[k], + tracker.ConfigPath[k], + "mlflow_model", + r.Models[k].ConfigFilePath, + ) + } + tracker.Type[k] = "mlflow_model" + tracker.ConfigPath[k] = r.Models[k].ConfigFilePath + } + for k := range r.Experiments { + if _, ok := tracker.Type[k]; ok { + return tracker, fmt.Errorf("multiple resources named %s (%s at %s, %s at %s)", + k, + tracker.Type[k], + tracker.ConfigPath[k], + "mlflow_experiment", + r.Experiments[k].ConfigFilePath, + ) + } + tracker.Type[k] = "mlflow_experiment" + tracker.ConfigPath[k] = r.Experiments[k].ConfigFilePath + } + return tracker, nil +} + // SetConfigFilePath sets the specified path for all resources contained in this instance. // This property is used to correctly resolve paths relative to the path // of the configuration file they were defined in. diff --git a/bundle/config/resources_test.go b/bundle/config/resources_test.go new file mode 100644 index 0000000000..cf07e54378 --- /dev/null +++ b/bundle/config/resources_test.go @@ -0,0 +1,96 @@ +package config + +import ( + "testing" + + "github.com/databricks/bricks/bundle/config/resources" + "github.com/stretchr/testify/assert" +) + +func TestVerifyUniqueResourceIdentifiers(t *testing.T) { + r := Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo.yml", + }, + }, + }, + Models: map[string]*resources.MlflowModel{ + "bar": { + Paths: resources.Paths{ + ConfigFilePath: "bar.yml", + }, + }, + }, + Experiments: map[string]*resources.MlflowExperiment{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo2.yml", + }, + }, + }, + } + _, err := r.VerifyUniqueResourceIdentifiers() + assert.ErrorContains(t, err, "multiple resources named foo (job at foo.yml, mlflow_experiment at foo2.yml)") +} + +func TestVerifySafeMerge(t *testing.T) { + r := Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo.yml", + }, + }, + }, + Models: map[string]*resources.MlflowModel{ + "bar": { + Paths: resources.Paths{ + ConfigFilePath: "bar.yml", + }, + }, + }, + } + other := Resources{ + Pipelines: map[string]*resources.Pipeline{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo2.yml", + }, + }, + }, + } + err := r.VerifySafeMerge(&other) + assert.ErrorContains(t, err, "multiple resources named foo (job at foo.yml, pipeline at foo2.yml)") +} + +func TestVerifySafeMergeForSameResourceType(t *testing.T) { + r := Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo.yml", + }, + }, + }, + Models: map[string]*resources.MlflowModel{ + "bar": { + Paths: resources.Paths{ + ConfigFilePath: "bar.yml", + }, + }, + }, + } + other := Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + Paths: resources.Paths{ + ConfigFilePath: "foo2.yml", + }, + }, + }, + } + err := r.VerifySafeMerge(&other) + assert.ErrorContains(t, err, "multiple resources named foo (job at foo.yml, job at foo2.yml)") +} diff --git a/bundle/config/root.go b/bundle/config/root.go index 2c0549305a..2d6da46654 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -116,35 +116,22 @@ func (r *Root) Load(path string) error { return err } - // check there are no duplicate identifiers in the config instance - _, err = r.listIds() - if err != nil { - return err - } - r.Path = filepath.Dir(path) r.SetConfigFilePath(path) - return nil + + _, err = r.Resources.VerifyUniqueResourceIdentifiers() + return err } func (r *Root) Merge(other *Root) error { // TODO: when hooking into merge semantics, disallow setting path on the target instance. other.Path = "" - // Check for duplicate resource identifiers - rootIds, err := r.listIds() + // Check for safe merge, protecting against duplicate resource identifiers + err := r.Resources.VerifySafeMerge(&other.Resources) if err != nil { return err } - otherIds, err := other.listIds() - if err != nil { - return err - } - for k := range rootIds { - if _, ok := otherIds[k]; ok { - return fmt.Errorf("duplicate identifier %s", k) - } - } // TODO: define and test semantics for merging. return mergo.MergeWithOverwrite(r, other) diff --git a/bundle/config/root_test.go b/bundle/config/root_test.go index d412999c53..1febd44eed 100644 --- a/bundle/config/root_test.go +++ b/bundle/config/root_test.go @@ -2,8 +2,6 @@ package config import ( "encoding/json" - "os" - "path/filepath" "reflect" "testing" @@ -79,26 +77,22 @@ func TestRootMergeMap(t *testing.T) { } func TestDuplicateIdOnLoadReturnsError(t *testing.T) { - dir := t.TempDir() - root := &Root{ - Resources: Resources{ - Jobs: map[string]*resources.Job{ - "foo": {}, - "bar": {}, - }, - Pipelines: map[string]*resources.Pipeline{ - "foo": {}, - }, - }, - } - b, err := json.Marshal(root) + root := &Root{} + err := root.Load("./testdata/duplicate_resource_names_in_root/bundle.yml") + assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_names_in_root/bundle.yml, pipeline at ./testdata/duplicate_resource_names_in_root/bundle.yml)") +} + +func TestDuplicateIdOnMergeReturnsError(t *testing.T) { + root := &Root{} + err := root.Load("./testdata/duplicate_resource_name_in_subconfiguration/bundle.yml") require.NoError(t, err) - err = os.WriteFile(filepath.Join(dir, "bundle.yml"), b, os.ModePerm) + other := &Root{} + err = other.Load("./testdata/duplicate_resource_name_in_subconfiguration/resources.yml") require.NoError(t, err) - err = root.Load(filepath.Join(dir, "bundle.yml")) - assert.ErrorContains(t, err, "duplicate identifier foo") + err = root.Merge(other) + assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_name_in_subconfiguration/bundle.yml, pipeline at ./testdata/duplicate_resource_name_in_subconfiguration/resources.yml)") } func TestRootMergeDetectsDuplicateIds(t *testing.T) { diff --git a/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/bundle.yml b/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/bundle.yml new file mode 100644 index 0000000000..a816029204 --- /dev/null +++ b/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/bundle.yml @@ -0,0 +1,10 @@ +bundle: + name: test + +workspace: + profile: test + +resources: + jobs: + foo: + name: job foo diff --git a/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/resources.yml b/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/resources.yml new file mode 100644 index 0000000000..c3dcb6e2fe --- /dev/null +++ b/bundle/config/testdata/duplicate_resource_name_in_subconfiguration/resources.yml @@ -0,0 +1,4 @@ +resources: + pipelines: + foo: + name: pipeline foo diff --git a/bundle/config/testdata/duplicate_resource_names_in_root/bundle.yml b/bundle/config/testdata/duplicate_resource_names_in_root/bundle.yml new file mode 100644 index 0000000000..1e9aa10b1f --- /dev/null +++ b/bundle/config/testdata/duplicate_resource_names_in_root/bundle.yml @@ -0,0 +1,13 @@ +bundle: + name: test + +workspace: + profile: test + +resources: + jobs: + foo: + name: job foo + pipelines: + foo: + name: pipeline foo From 8aee7844d8f5f2782d795ab93de48524abb75571 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 13:58:23 +0200 Subject: [PATCH 4/9] removed unused function --- bundle/config/root.go | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index 2d6da46654..0e7b89e14f 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -1,7 +1,6 @@ package config import ( - "fmt" "os" "path/filepath" @@ -80,32 +79,6 @@ func (r *Root) SetConfigFilePath(path string) { } } -func (r *Root) listIds() (map[string]struct{}, error) { - result := map[string]struct{}{} - for k := range r.Resources.Jobs { - result[k] = struct{}{} - } - for k := range r.Resources.Pipelines { - if _, ok := result[k]; ok { - return nil, fmt.Errorf("duplicate identifier %s", k) - } - result[k] = struct{}{} - } - for k := range r.Resources.Models { - if _, ok := result[k]; ok { - return nil, fmt.Errorf("duplicate identifier %s", k) - } - result[k] = struct{}{} - } - for k := range r.Resources.Experiments { - if _, ok := result[k]; ok { - return nil, fmt.Errorf("duplicate identifier %s", k) - } - result[k] = struct{}{} - } - return result, nil -} - func (r *Root) Load(path string) error { raw, err := os.ReadFile(path) if err != nil { From 9d79a8a08b8310707ee7c68ab7a87ba7c3c274ea Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 14:00:05 +0200 Subject: [PATCH 5/9] - --- bundle/config/resources.go | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 1bbbeedb02..6aed895f6c 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -56,7 +56,6 @@ func (r *Resources) VerifyUniqueResourceIdentifiers() (*UniqueResourceIdTracker, } for k := range r.Pipelines { if _, ok := tracker.Type[k]; ok { - return tracker, fmt.Errorf("multiple resources named %s (%s at %s, %s at %s)", k, tracker.Type[k], From afa5db8ff78e4bdc56a6de9aab0a8eac056e4079 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 14 Apr 2023 14:01:01 +0200 Subject: [PATCH 6/9] remove old test --- bundle/config/root_test.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/bundle/config/root_test.go b/bundle/config/root_test.go index 1febd44eed..7dd1318175 100644 --- a/bundle/config/root_test.go +++ b/bundle/config/root_test.go @@ -5,7 +5,6 @@ import ( "reflect" "testing" - "github.com/databricks/bricks/bundle/config/resources" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -94,25 +93,3 @@ func TestDuplicateIdOnMergeReturnsError(t *testing.T) { err = root.Merge(other) assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_name_in_subconfiguration/bundle.yml, pipeline at ./testdata/duplicate_resource_name_in_subconfiguration/resources.yml)") } - -func TestRootMergeDetectsDuplicateIds(t *testing.T) { - root := &Root{ - Resources: Resources{ - Jobs: map[string]*resources.Job{ - "foo": {}, - }, - Pipelines: map[string]*resources.Pipeline{ - "bar": {}, - }, - }, - } - other := &Root{ - Resources: Resources{ - Models: map[string]*resources.MlflowModel{ - "bar": {}, - }, - }, - } - err := root.Merge(other) - assert.ErrorContains(t, err, "duplicate identifier bar") -} From f1cacedff1fa4d6446e5d03319a1cbee9ad284d2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sun, 16 Apr 2023 23:38:41 +0200 Subject: [PATCH 7/9] Add black box tests --- .../no_subconfigurations/bundle.yml | 13 ++++++++ .../one_subconfiguration/bundle.yml | 10 +++++++ .../one_subconfiguration/resources.yml | 4 +++ .../two_subconfigurations/bundle.yml | 5 ++++ .../two_subconfigurations/resources1.yml | 4 +++ .../two_subconfigurations/resources2.yml | 4 +++ bundle/tests/conflicting_resource_ids_test.go | 30 +++++++++++++++++++ 7 files changed, 70 insertions(+) create mode 100644 bundle/tests/conflicting_resource_ids/no_subconfigurations/bundle.yml create mode 100644 bundle/tests/conflicting_resource_ids/one_subconfiguration/bundle.yml create mode 100644 bundle/tests/conflicting_resource_ids/one_subconfiguration/resources.yml create mode 100644 bundle/tests/conflicting_resource_ids/two_subconfigurations/bundle.yml create mode 100644 bundle/tests/conflicting_resource_ids/two_subconfigurations/resources1.yml create mode 100644 bundle/tests/conflicting_resource_ids/two_subconfigurations/resources2.yml create mode 100644 bundle/tests/conflicting_resource_ids_test.go diff --git a/bundle/tests/conflicting_resource_ids/no_subconfigurations/bundle.yml b/bundle/tests/conflicting_resource_ids/no_subconfigurations/bundle.yml new file mode 100644 index 0000000000..1e9aa10b1f --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/no_subconfigurations/bundle.yml @@ -0,0 +1,13 @@ +bundle: + name: test + +workspace: + profile: test + +resources: + jobs: + foo: + name: job foo + pipelines: + foo: + name: pipeline foo diff --git a/bundle/tests/conflicting_resource_ids/one_subconfiguration/bundle.yml b/bundle/tests/conflicting_resource_ids/one_subconfiguration/bundle.yml new file mode 100644 index 0000000000..a816029204 --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/one_subconfiguration/bundle.yml @@ -0,0 +1,10 @@ +bundle: + name: test + +workspace: + profile: test + +resources: + jobs: + foo: + name: job foo diff --git a/bundle/tests/conflicting_resource_ids/one_subconfiguration/resources.yml b/bundle/tests/conflicting_resource_ids/one_subconfiguration/resources.yml new file mode 100644 index 0000000000..c3dcb6e2fe --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/one_subconfiguration/resources.yml @@ -0,0 +1,4 @@ +resources: + pipelines: + foo: + name: pipeline foo diff --git a/bundle/tests/conflicting_resource_ids/two_subconfigurations/bundle.yml b/bundle/tests/conflicting_resource_ids/two_subconfigurations/bundle.yml new file mode 100644 index 0000000000..f8fe99ebc4 --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/two_subconfigurations/bundle.yml @@ -0,0 +1,5 @@ +bundle: + name: test + +workspace: + profile: test diff --git a/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources1.yml b/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources1.yml new file mode 100644 index 0000000000..39a24ef59d --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources1.yml @@ -0,0 +1,4 @@ +resources: + jobs: + foo: + name: job foo diff --git a/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources2.yml b/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources2.yml new file mode 100644 index 0000000000..c3dcb6e2fe --- /dev/null +++ b/bundle/tests/conflicting_resource_ids/two_subconfigurations/resources2.yml @@ -0,0 +1,4 @@ +resources: + pipelines: + foo: + name: pipeline foo diff --git a/bundle/tests/conflicting_resource_ids_test.go b/bundle/tests/conflicting_resource_ids_test.go new file mode 100644 index 0000000000..d38a1d4f13 --- /dev/null +++ b/bundle/tests/conflicting_resource_ids_test.go @@ -0,0 +1,30 @@ +package config_tests + +import ( + "context" + "testing" + + "github.com/databricks/bricks/bundle" + "github.com/databricks/bricks/bundle/config/mutator" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConflictingResourceIdsNoSubconfig(t *testing.T) { + _, err := bundle.Load("./conflicting_resource_ids/no_subconfigurations") + assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/no_subconfigurations/bundle.yml, pipeline at conflicting_resource_ids/no_subconfigurations/bundle.yml)") +} + +func TestConflictingResourceIdsOneSubconfig(t *testing.T) { + b, err := bundle.Load("./conflicting_resource_ids/one_subconfiguration") + require.NoError(t, err) + err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) + assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/one_subconfiguration/bundle.yml, pipeline at conflicting_resource_ids/one_subconfiguration/resources.yml)") +} + +func TestConflictingResourceIdsTwoSubconfigs(t *testing.T) { + b, err := bundle.Load("./conflicting_resource_ids/two_subconfigurations") + require.NoError(t, err) + err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) + assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/two_subconfigurations/resources1.yml, pipeline at conflicting_resource_ids/two_subconfigurations/resources2.yml)") +} From 2f91d7858493bab7b0619f17826162c00e7dfcd6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Apr 2023 10:56:58 +0200 Subject: [PATCH 8/9] make tests ok for windows --- bundle/tests/conflicting_resource_ids_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/bundle/tests/conflicting_resource_ids_test.go b/bundle/tests/conflicting_resource_ids_test.go index d38a1d4f13..96df3066f8 100644 --- a/bundle/tests/conflicting_resource_ids_test.go +++ b/bundle/tests/conflicting_resource_ids_test.go @@ -2,6 +2,8 @@ package config_tests import ( "context" + "fmt" + "path/filepath" "testing" "github.com/databricks/bricks/bundle" @@ -12,19 +14,24 @@ import ( func TestConflictingResourceIdsNoSubconfig(t *testing.T) { _, err := bundle.Load("./conflicting_resource_ids/no_subconfigurations") - assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/no_subconfigurations/bundle.yml, pipeline at conflicting_resource_ids/no_subconfigurations/bundle.yml)") + bundleConfigPath := filepath.ToSlash("conflicting_resource_ids/no_subconfigurations/bundle.yml") + assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", bundleConfigPath, bundleConfigPath)) } func TestConflictingResourceIdsOneSubconfig(t *testing.T) { b, err := bundle.Load("./conflicting_resource_ids/one_subconfiguration") require.NoError(t, err) err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) - assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/one_subconfiguration/bundle.yml, pipeline at conflicting_resource_ids/one_subconfiguration/resources.yml)") + bundleConfigPath := filepath.ToSlash("conflicting_resource_ids/one_subconfiguration/bundle.yml") + resourcesConfigPath := filepath.ToSlash("conflicting_resource_ids/one_subconfiguration/resources.yml") + assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", bundleConfigPath, resourcesConfigPath)) } func TestConflictingResourceIdsTwoSubconfigs(t *testing.T) { b, err := bundle.Load("./conflicting_resource_ids/two_subconfigurations") require.NoError(t, err) err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) - assert.ErrorContains(t, err, "multiple resources named foo (job at conflicting_resource_ids/two_subconfigurations/resources1.yml, pipeline at conflicting_resource_ids/two_subconfigurations/resources2.yml)") + resources1ConfigPath := filepath.ToSlash("conflicting_resource_ids/two_subconfigurations/resources1.yml") + resources2ConfigPath := filepath.ToSlash("conflicting_resource_ids/two_subconfigurations/resources2.yml") + assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", resources1ConfigPath, resources2ConfigPath)) } From 222d4437c86a2ca6a6a4d4d27e51bed0793e4604 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Apr 2023 11:21:54 +0200 Subject: [PATCH 9/9] toSlash -> fromSlash --- bundle/tests/conflicting_resource_ids_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bundle/tests/conflicting_resource_ids_test.go b/bundle/tests/conflicting_resource_ids_test.go index 96df3066f8..500a9d8e22 100644 --- a/bundle/tests/conflicting_resource_ids_test.go +++ b/bundle/tests/conflicting_resource_ids_test.go @@ -14,7 +14,7 @@ import ( func TestConflictingResourceIdsNoSubconfig(t *testing.T) { _, err := bundle.Load("./conflicting_resource_ids/no_subconfigurations") - bundleConfigPath := filepath.ToSlash("conflicting_resource_ids/no_subconfigurations/bundle.yml") + bundleConfigPath := filepath.FromSlash("conflicting_resource_ids/no_subconfigurations/bundle.yml") assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", bundleConfigPath, bundleConfigPath)) } @@ -22,8 +22,8 @@ func TestConflictingResourceIdsOneSubconfig(t *testing.T) { b, err := bundle.Load("./conflicting_resource_ids/one_subconfiguration") require.NoError(t, err) err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) - bundleConfigPath := filepath.ToSlash("conflicting_resource_ids/one_subconfiguration/bundle.yml") - resourcesConfigPath := filepath.ToSlash("conflicting_resource_ids/one_subconfiguration/resources.yml") + bundleConfigPath := filepath.FromSlash("conflicting_resource_ids/one_subconfiguration/bundle.yml") + resourcesConfigPath := filepath.FromSlash("conflicting_resource_ids/one_subconfiguration/resources.yml") assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", bundleConfigPath, resourcesConfigPath)) } @@ -31,7 +31,7 @@ func TestConflictingResourceIdsTwoSubconfigs(t *testing.T) { b, err := bundle.Load("./conflicting_resource_ids/two_subconfigurations") require.NoError(t, err) err = bundle.Apply(context.Background(), b, mutator.DefaultMutators()) - resources1ConfigPath := filepath.ToSlash("conflicting_resource_ids/two_subconfigurations/resources1.yml") - resources2ConfigPath := filepath.ToSlash("conflicting_resource_ids/two_subconfigurations/resources2.yml") + resources1ConfigPath := filepath.FromSlash("conflicting_resource_ids/two_subconfigurations/resources1.yml") + resources2ConfigPath := filepath.FromSlash("conflicting_resource_ids/two_subconfigurations/resources2.yml") assert.ErrorContains(t, err, fmt.Sprintf("multiple resources named foo (job at %s, pipeline at %s)", resources1ConfigPath, resources2ConfigPath)) }