Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add acceptance tests for schema dependency auto-detection #2169

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Add acceptance tests for schema dependency auto-detection #2169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8a7d5fd
Warn user to use ${resources.schemas...} syntax
shreyas-goenka Dec 9, 2024
332a6b0
add comments'
shreyas-goenka Dec 20, 2024
32ebc5c
Merge remote-tracking branch 'origin' into detect/schema-dep
shreyas-goenka Dec 27, 2024
893118e
fix silently
shreyas-goenka Dec 27, 2024
1d337c8
-
shreyas-goenka Dec 27, 2024
aa52b1d
-
shreyas-goenka Dec 27, 2024
f7d926e
Merge remote-tracking branch 'origin' into detect/schema-dep
shreyas-goenka Jan 7, 2025
fc8b5e7
address comments
shreyas-goenka Jan 7, 2025
ed59500
address comments
shreyas-goenka Jan 7, 2025
fcc47de
address comments
shreyas-goenka Jan 7, 2025
4d09201
two functions
shreyas-goenka Jan 7, 2025
69d477f
-
shreyas-goenka Jan 7, 2025
a5243d6
merge
shreyas-goenka Jan 16, 2025
58aca2a
address comments
shreyas-goenka Jan 16, 2025
ba3a36e
fix test
shreyas-goenka Jan 16, 2025
5eb7f4a
address comments
shreyas-goenka Jan 16, 2025
fa43a4d
Merge remote-tracking branch 'origin' into acc-test-schema-dep
shreyas-goenka Jan 16, 2025
c0d3c4f
Add acceptance tests for Schema depenendency auto-detection
shreyas-goenka Jan 16, 2025
ce4580e
-
shreyas-goenka Jan 16, 2025
d12852c
reduce test size
shreyas-goenka Jan 16, 2025
a35ab69
fix test
shreyas-goenka Jan 16, 2025
7dadf84
address comments
shreyas-goenka Jan 20, 2025
622a59d
remove unit tests
shreyas-goenka Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_schema/databricks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
bundle:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli/bundle/config/mutator/capture_schema_dependency_test.go
Line 1 in 2e70558
package mutator

Do we need the unit tests? Is there something they cover that this acceptance tests won't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the unit tests? Is there something they cover that these acceptance tests won't?

No, mostly. They're local to the mutator itself, which implements the functionality, and that's why I did not remove them. Unit tests next to the code are nice to have and can live independently of the acceptance tests.

There's some additional coverage for what happens when the resources pointers are nil, but nothing that absolutely needs to be covered, IMO.

name: dlt-schema-field-dep-on-uc-schema

resources:
schemas:
schema1:
catalog_name: catalog1
name: foobar

schema2:
catalog_name: catalog2
name: foobar

schema3:
catalog_name: catalog1
name: barfoo

pipelines:
pipeline1:
catalog: catalog1
schema: foobar

pipeline2:
catalog: catalog2
schema: foobar

pipeline3:
catalog: catalog1
schema: barfoo

pipeline4:
catalog: catalogX
schema: foobar

pipeline5:
catalog: catalog1
schema: schemaX

pipeline6:
schema: foobar

pipeline7:
name: whatever
28 changes: 28 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_schema/out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"pipeline1": {
"catalog": "catalog1",
"schema": "${resources.schemas.schema1.name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that this is not resolved to the name? We have the name, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that this is not resolved to the name? We have the name, right?

The terraform layer in the stack resolves any values with the resources prefix. bundle/deploy/terraform/interpolate.go is the translation layer between these references and syntax that terraform understands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work for 'name' as well? I thought it's for looking up ids.

In any case, could you add this context as a comment on script?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add 'bundle summary' to see the resolution in action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work for 'name' as well? I thought it's for looking up ids

Yeah, terraform resolves any fields that are keys in it.

Should we also add 'bundle summary' to see the resolution in action?

Bundle summary does not resolve these references. It's resolved at runtime when terraform apply happens during bundle deploy.

    "volumes": {
      "abc": {
        "catalog_name": "main",
        "modified_status": "created",
        "name": "abc",
        "schema_name": "${resources.schemas.xyz.name}",
        "volume_type": "MANAGED"
      }
    }
  },

},
"pipeline2": {
"catalog": "catalog2",
"schema": "${resources.schemas.schema2.name}"
},
"pipeline3": {
"catalog": "catalog1",
"schema": "${resources.schemas.schema3.name}"
},
"pipeline4": {
"catalog": "catalogX",
"schema": "foobar"
},
"pipeline5": {
"catalog": "catalog1",
"schema": "schemaX"
},
"pipeline6": {
"schema": "foobar"
},
"pipeline7": {
"name": "whatever"
}
}
3 changes: 3 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_schema/script
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# We expect DLT pipelines that use a schema defined in the bundle to have the
# schema names replaced with a reference like `${resource.schemas.abc.name}`.
$CLI bundle validate -o json | jq .resources.pipelines | jq 'map_values(del(.deployment, .permissions))' > out.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a small comment there what do you expect to see in the output.

Also it should be clear -- is the output correct or is there something we want to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing to fix. I'd assume that the tests here are for correct behaviour by default. Should we explicitly call that out in all acceptance tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Normally, it's not needed, but ${resources.*} behaviour is a bit surprising, so worth calling out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment for the expectation with the test itself.

43 changes: 43 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_target/databricks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
bundle:
name: dlt-target-field-dep-on-uc-schema

resources:
schemas:
schema1:
catalog_name: catalog1
name: foobar

schema2:
catalog_name: catalog2
name: foobar

schema3:
catalog_name: catalog1
name: barfoo

pipelines:
pipeline1:
catalog: catalog1
target: foobar

pipeline2:
catalog: catalog2
target: foobar

pipeline3:
catalog: catalog1
target: barfoo

pipeline4:
catalog: catalogX
target: foobar

pipeline5:
catalog: catalog1
target: schemaX

pipeline6:
target: foobar

pipeline7:
name: whatever
28 changes: 28 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_target/out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"pipeline1": {
"catalog": "catalog1",
"target": "${resources.schemas.schema1.name}"
},
"pipeline2": {
"catalog": "catalog2",
"target": "${resources.schemas.schema2.name}"
},
"pipeline3": {
"catalog": "catalog1",
"target": "${resources.schemas.schema3.name}"
},
"pipeline4": {
"catalog": "catalogX",
"target": "foobar"
},
"pipeline5": {
"catalog": "catalog1",
"target": "schemaX"
},
"pipeline6": {
"target": "foobar"
},
"pipeline7": {
"name": "whatever"
}
}
3 changes: 3 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/dlt_target/script
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# We expect DLT pipelines that use a schema defined in the bundle to have the
# schema names replaced with a reference like `${resource.schemas.abc.name}`.
$CLI bundle validate -o json | jq .resources.pipelines | jq 'map_values(del(.deployment, .permissions))' > out.json
37 changes: 37 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/volume/databricks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
bundle:
name: volume-deps-on-schema

resources:
schemas:
schema1:
catalog_name: catalog1
name: foobar

schema2:
catalog_name: catalog2
name: foobar

schema3:
catalog_name: catalog1
name: barfoo

volumes:
volume1:
catalog_name: catalog1
schema_name: foobar

volume2:
catalog_name: catalog2
schema_name: foobar

volume3:
catalog_name: catalog1
schema_name: barfoo

volume4:
catalog_name: catalogX
schema_name: foobar

volume5:
catalog_name: catalog1
schema_name: schemaX
27 changes: 27 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/volume/out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"volume1": {
"catalog_name": "catalog1",
"schema_name": "${resources.schemas.schema1.name}",
"volume_type": "MANAGED"
},
"volume2": {
"catalog_name": "catalog2",
"schema_name": "${resources.schemas.schema2.name}",
"volume_type": "MANAGED"
},
"volume3": {
"catalog_name": "catalog1",
"schema_name": "${resources.schemas.schema3.name}",
"volume_type": "MANAGED"
},
"volume4": {
"catalog_name": "catalogX",
"schema_name": "foobar",
"volume_type": "MANAGED"
},
"volume5": {
"catalog_name": "catalog1",
"schema_name": "schemaX",
"volume_type": "MANAGED"
}
}
3 changes: 3 additions & 0 deletions acceptance/bundle/unitycatalog/capture_schema_dependency/volume/script
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# We expect UC Volumes that use a schema defined in the bundle to have the
# schema names replaced with a reference like `${resource.schemas.abc.name}`.
$CLI bundle validate -o json | jq .resources.volumes > out.json
Loading
Loading