-
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
Check model configs #1027
Check model configs #1027
Conversation
…crl/check-model-configs
d305cd9
to
2cb7394
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.
I dropped a comment in here about disabled
models, but otherwise this looks pretty good to me!
Some other thoughts:
- we should get the tests passing
- I couldn't check out this branch? I'm not sure if it's because i have a local branch with the same prefix, or if it's something else, but I'm pretty confused. I ended up checking out the latest commit to get it working locally.
This should be ready to merge once we've discussed (and possibly acted upon) the enabled: false
conversation.
dbt/config.py
Outdated
return unused_resource_config_paths | ||
|
||
def warn_for_unused_resource_config_paths(self, resource_fqns): | ||
unused = self.get_unused_resource_config_paths(resource_fqns) |
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's an unfortunate effect that happens here:
Disabling models in dbt_project.yml
file will effectively "remove" the models from the manifest, therefore causing dbt to think that the config is useless.
For example:
models:
my_project:
enabled: false
If your project name is my_project
, this will (errantly) result in:
WARNING: Configuration paths exist in your dbt_project.yml file which do not apply to any resources.
There are 1 unused configuration paths:
- models.my_project
I don't really know that there's a good fix for this, as we've consciously decided to discard disabled resources in parsing. Would it be crazy to skip adding paths in get_unused_resource_config_paths
if there's an enabled: false
present? I think that would probably get pretty messy....
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.
Ok, I got this done. I had to refactor the loader code quite a bit, the ResourceLoader
abstraction ended up getting in the way quite a bit, so now the GraphLoader
just explicitly builds up a manifest as it goes. I think if we want to do more things like this (retaining extra data as we load/parse) we'll have to refactor the parser/loader code further.
3336f44
to
4be304a
Compare
4be304a
to
e07d1aa
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 mostly @clrcrl's PR #725, with additional tests and rebased into dev/guion-bluford. No real code changes from her PR.
@clrcrl I can't request a review from you, unfortunately, but please give any feedback you have as well!