-
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
Configs in schema files #3616
Configs in schema files #3616
Conversation
Notes on implementation: |
424b503
to
8b089af
Compare
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.
This is great work! I had a lot of fun taking this for a spin. We'll want to significantly revise our documentation on nodes vs. properties, and that's a good thing :)
I'm going to spend some more time playing around with this. For the moment, my biggest comment is that we should merge node.config.meta
into node.meta
. It doesn't make sense to leave these as separate, unrelated values. This might have some confusing behavior, so let's find ways to do reasonable things. I'm thinking we should treat the meta
key in the models
dict as being at the "same level as" the config.meta
key, and raise an error if both are set. For example:
models:
- name: my_model
meta:
something: blabla
config:
meta:
something: lala
We should raise an error here, since we don't know whether to prefer blabla
or lala
. We should do it even if those meta
dictionaries contain different keys. Any in-file config(meta = ...)
should take precedence.
I'm also going to open some follow-on issues that are outside the scope of this specific PR, related to the larger reconciliation effort proposed in #2401 and kicked off here.
- Support
config:
on tests. That might get us "for free" the ability to set themeta
property on tests (Add support formeta
in tests #3629) - Support
config:
on sources, and then turn existing node-level properties into configs that can be set indbt_project.yml
+config
blocks. When I say this, I'm especially thinking about source properties likedatabase
,loaded_at_field
, etc. (I think it would make sense to reserve some properties—description
,columns
,tests
—rather than try to make those work as configs... though there are some valid reasons to want those, too!) - Support the same
config:
block-style syntax indbt_project.yml
, as a substitute for the+
prefix that everyone finds confusing :)
Update on my comment above, with links to the promised issues:
|
As discussed, there are three outstanding pieces for this PR:
Those are all things we want for feature completeness in v0.21. They don't have to be blockers to merging this specific PR; I'd be happy to open follow-on issue(s), if they'd require nontrivial work and we'd rather get this PR merged now. |
9938983
to
9ab919e
Compare
0969567
to
0a3eaa4
Compare
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.
Giving this a thumbs-up from an end-user functional perspective. There are likely to be a handful of small things that we want to touch up as people start using this. I'm excited to get it into beta.
🚀 🚀
0a3eaa4
to
b622dcc
Compare
b622dcc
to
d5461cc
Compare
These aren't being captured in dbt Docs. The only way that seems to work is to config in the model. It would also be great if column tags made it into Docs. |
resolves #2401
Description
This adds the ability to set configs in schema files for models, seeds, snapshots, and analyses. It does not address configs in macros, sources, or tests, which will be addressed separately.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.