-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add builder config to test node config #10767
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10767 +/- ##
==========================================
- Coverage 89.20% 89.13% -0.08%
==========================================
Files 183 183
Lines 23419 23425 +6
==========================================
- Hits 20892 20879 -13
- Misses 2527 2546 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Failing dbt-snowflake test against main: https://github.com/dbt-labs/dbt-snowflake/actions/runs/11042144022/job/30673904052#step:8:575 Succesful tests running against this PR branch: https://github.com/dbt-labs/dbt-snowflake/actions/runs/11041928244/job/30673264899#step:8:572 |
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.
Prior to this "add_config_call" was generally used for SQL nodes to store the extracted config for re-application after applying schema changes. But generic nodes are a fundamentally different thing making it a bit hard to reason about. Since they are constructed only from schema file definitions, there is no "base" sql file to apply later. The config_call_dict on a node is also stored in the internal manifest (for a partial parsing reason that I remember knowing at one point but am not sure really applies anymore...). But generic tests are very leaf-y things that will always be re-generated when anything above them changes, so having it stored on the generic test node probably isn't an issue.
I think this is fine, but please add some comments to the place where the "add_config_call" is done about why (and how it's different than other uses of that call), and add some tests.
I have added a short code comment but can elaborate further, I have tried to add a test that validates this but all I have achieved is narrowing down the scope of impact: it appears this only ensure the config is available when the pre_model_hook is run. Since dbt-postgres doesn't definine a generic pre_model_hook (like dbt-snowflake) we can't test this behavior here |
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.
There are some failing tests, but I'm approving so you don't have to come back again.
Resolves #10484
Problem
Solution
Checklist