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

Fix: missing vars in disabled models fail compilation (#434) #1429

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

beckjake
Copy link
Contributor

Fixes #434

When a var is missing during parsing, just always return None. Wait until runtime (compilation, etc) to actually fail about it.

This avoids requiring a var to be set even when a model is disabled, but still fails as appropriate when a var isn't set for an enabled model at compile time.

I've based this on PR #1426 as they touch basically the same stuff.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

I feel some resistance to changing Undefined Var errors from compile-time errors to runtime errors. I'm trying to discern if it's some moral objection that I have, or if I'm just very accustomed to how it's historically worked.

The tricky part here is knowing if a model is disabled while we're parsing it, right? I imagine we'd need to look at the model after it was parsed to understand if:

  1. it has a local var with a value of _VAR_NOTSET and
  2. it is disabled

Can you think of any sane way for us to accomplish something like that? Separately, I'll have a think about whether or not we can swing runtime Undefined Var errors

@beckjake
Copy link
Contributor Author

Well, first: I think if anything it's actually more correct to let the errors ride at parse time and catch them at runtime. The only goal of parse-time should be to find refs, sources, and docs and to evaluate config() calls. Anything else that we can catch that early is nice, but we should throw away any of those niceties if it means we can do a better job at having understandable behavior.

That said I think we can figure that problem regarding the variable being set (I don't think _VAR_NOTSET comes into this!) and the model being disabled out, but it hinges on how we handle #1149. Currently that stuff is not going to work like it should.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

I think if anything it's actually more correct to let the errors ride at parse time and catch them at runtime

Yeah, you're right. It's not just disabled models -- if model_a is missing a var but you're running model_b only, dbt shouldn't brick your whole run. I'm convinced!

Ship it!

@beckjake beckjake changed the base branch from feature/allow-null-vars to dev/wilt-chamberlain April 30, 2019 01:22
@beckjake beckjake merged commit ad2f228 into dev/wilt-chamberlain Apr 30, 2019
@beckjake beckjake deleted the fix/vars-in-disabled-models branch April 30, 2019 14:34
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.

Models that are disabled still fail compilation if they are missing vars
2 participants