-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Call adapter.pre_model_hook + adapter.post_model_hook within tests #10258
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10258 +/- ##
==========================================
- Coverage 88.74% 88.72% -0.02%
==========================================
Files 180 180
Lines 22483 22488 +5
==========================================
Hits 19953 19953
- Misses 2530 2535 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d0cb89b
to
ef67cff
Compare
# execute materialization macro | ||
macro_func() | ||
finally: | ||
self.adapter.post_model_hook(context, hook_ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, this is not a behaviour change because:
- pre/post_model_hook are no-ops by default: https://github.com/dbt-labs/dbt-adapters/blob/8ff69435a753f0a94dd1274d0e8dad199805cd13/dbt/adapters/base/impl.py#L1415-L1438
- only dbt-snowflake has custom implementation, which is based on the presence of a config and effectively a no-op otherwise (uses warehouse from profile). Is the assumption that users with a
snowflake_warehouse
would always want to route tests to that warehouse?
cc @jtcohen6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i can help supply a answer here. with this pr and
- [Feature] add generic config for generic tests #10245 will allow us to configure the snowflake_warehosue even for tests as its a config that can be changed like we do for models, sources, etc.
as tested in dbt-labs/dbt-snowflake#1070
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! gotcha -- so configs were not possible to provide for generic tests in the past, which means there should not be any currently set on them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my understanding yes, we could use a old pattern to set a set number of accepted configs linked in the generic_config issue, making this update to the newer syntax while maintaining the old will allow us to do this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MichelleArk @McKnight-42 for thinking of this!
As I see it, the only way in which this could be a "breaking" change is if someone had configured a singular test, or all tests (from dbt_project.yml
), with a snowflake_warehouse
that they didn't actually want to be used.
Something like:
# dbt_project.yml
tests:
+snowflake_warehouse: does_not_exist
-- tests/my_singular_test.sql
{{ config(snowflake_warehouse = 'wrong_warehouse') }}
select ...
I do not see that as a breaking change; I see it as this feature in dbt (specifically dbt-snowflake
) finally working as intended. As such, I do not believe we need to put it behind a legacy behavior migration flag.
resolves #10198
Problem
Allow tests to take in pre and post hooks similar to our
pre/post_model_hook
to take in configs likesnowflake_warehouse
and other arbitrary configs.Solution
Potential Solutions:
pre/post_model_hoook
and add logging toadapter definitions as described or in the area right after we add the hooks in via this pr.
pre/post_test_hook
in BaseAdapter indbt-adapters
then have the adapters create their own version of the method as we do forpre_model_hook
(logging could possibly happen in method more easily this way) but would also mean many more pr's across adapters to add the new methods.Checklist