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

feat(dbt): enforce all-or-nothing opt-in for asset checks #16852

Closed
wants to merge 1 commit into from

Conversation

rexledesma
Copy link
Contributor

Summary & Motivation

As the title.

How I Tested These Changes

pytest

@rexledesma
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Contributor

@sryza sryza left a comment

Choose a reason for hiding this comment

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

If this is a global setting, might it make sense as an argument to dbt_assets or somehow on the DagsterDbtTranslator, instead as test-level config?

@rexledesma
Copy link
Contributor Author

If this is a global setting, might it make sense as an argument to dbt_assets or somehow on the DagsterDbtTranslator, instead as test-level config?

We could probably consider adding it to DagsterDbtTranslator, since that's already threaded through the system. That should probably go into its own change though.

@rexledesma rexledesma force-pushed the rl/assert-any-all-asset-check-metadata branch from 625e26d to 0e0819d Compare September 27, 2023 20:17
@rexledesma rexledesma force-pushed the rl/assert-any-all-asset-check-metadata branch from 0e0819d to d13e120 Compare September 27, 2023 20:30
@sryza
Copy link
Contributor

sryza commented Sep 27, 2023

We could probably consider adding it to DagsterDbtTranslator, since that's already threaded through the system. That should probably go into its own change though.

With that change, would we also need this change?

@rexledesma
Copy link
Contributor Author

Unless we want to break users who have been enabling asset checks using their dbt project's metadata, we will still need this change.

@sryza
Copy link
Contributor

sryza commented Sep 27, 2023

Unless we want to break users who have been enabling asset checks using their dbt project's metadata, we will still need this change.

I think we should be comfortable breaking them if we think it's the right direction. We expect users who sign up to be design partners for features before their public release to be able to sustain breaking changes to those features.

rexledesma added a commit that referenced this pull request Sep 29, 2023
## Summary & Motivation
As discussed in #16852,
enabling asset checks is an all-or-nothing operation. In order to not
confuse users about this behavior, we will only allow users to enable
this feature through a binary toggle, rather than setting the metadata
on individual dbt tests.

Here, we use `DagsterDbtTranslator` as the object that contains the
settings that a user can configure to enable features on their dbt
project when consuming it with Dagster.

One thing to note here is that custom subclasses that implement their
own `__init__` will need to also pass down the settings object if they
are opting into asset checks.

## How I Tested These Changes
pytest
@rexledesma rexledesma closed this Oct 3, 2023
@rexledesma rexledesma deleted the rl/assert-any-all-asset-check-metadata branch February 21, 2024 01:40
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.

2 participants