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.

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

Patch references to UC schemas to capture dependencies automatically #1989

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

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Dec 9, 2024

Changes

FIxes #1977.

Tests

Unit tests and manually.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/12239986056

@shreyas-goenka shreyas-goenka changed the title Warn user to use ${resources.schemas...} syntax Warn user to use ${resources.schemas...} syntax Dec 9, 2024
bundle/config/validate/schema_references.go Outdated Show resolved Hide resolved
diags := diag.Diagnostics{}
for k, p := range rb.Config().Resources.Pipelines {
// Skip if the pipeline uses hive metastore.
if p.Catalog == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API allows creating a pipeline without a schema/target specified as long as you are not using UC. It's not a forward-looking use case anyway, so I did not look deeper into this.

Severity: diag.Warning,
Summary: `Use ${resources.schemas.s1.name} syntax to refer to the UC schema instead of directly using its name "schema1"`,
Detail: `Using ${resources.schemas.s1.name} will allow DABs to capture the deploy time dependency this DLT pipeline
has on the schema "schema1" and deploy changes to the schema before deploying the pipeline.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be painful to update this test if the message changes, but it's not nice to assert on the message.

I have added a "golden files" test utilities here #2025 , it's a good fit for this cases.

Full output is stored in the file and can be updated by running tests with TESTS_OUTPUT=OVERWRITE

Copy link
Contributor

Choose a reason for hiding this comment

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

Note, that PR only targets integration tests for now, so not ready for your use case yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine in this case since its unit tests that are colocated with the mutator that implements the functionality. It'll be a simple search and replace when updating.

Are you proposing to extend the golden file approach to mutators or unit tests? Possibly by serializing the diagnostics and persisting them in a file?

@shreyas-goenka shreyas-goenka requested a review from denik December 20, 2024 15:44
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

I understand that we can ask users to capture this dependency through variable interpolation, but could we do this ourselves instead? I.e. we know if someone wants to use a schema for a volume, so we can inject a "depends on" relationship transparently.

bundle/config/validate/schema_references.go Outdated Show resolved Hide resolved
Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1989
  • Commit SHA: aa52b1dab3354c56886cae7740b2cb73fb5ade69

Checks will be approved automatically on success.

@shreyas-goenka shreyas-goenka changed the title Warn user to use ${resources.schemas...} syntax Patch references to UC schemas to capture dependencies automatically Dec 27, 2024
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Nice. Few suggestions.

bundle/config/mutator/resolve_schema_dependency.go Outdated Show resolved Hide resolved
bundle/config/mutator/resolve_schema_dependency.go Outdated Show resolved Hide resolved
bundle/config/mutator/resolve_schema_dependency.go Outdated Show resolved Hide resolved
bundle/config/mutator/resolve_schema_dependency.go Outdated Show resolved Hide resolved
bundle/config/mutator/resolve_schema_dependency_test.go Outdated Show resolved Hide resolved
bundle/config/mutator/resolve_schema_dependency_test.go Outdated Show resolved Hide resolved
"nilschema": {},
"emptyschema": {
CreateSchema: &catalog.CreateSchema{},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The nilschema should be nil as in actual nil and empty as in what's now the nilschema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting up dependency is not working between schema and volume
4 participants