-
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
Fix bug accessing target in deps and clean commands #4758
Conversation
return { | ||
'config-version': 2, | ||
'models': { | ||
'+any_config': "{{ target.name }}", |
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 works for the most common cases that we should expect to see. It still doesn't work for one of the configs that we saw in the wild:
# dbt_project.yml
models:
+enabled: "{{ target.name in ['redshift', 'postgres'] | as_bool }}"
That still raises the error:
17:08:37 Encountered an error:
'in <string>' requires string as left operand, not NoneType
Is there any way we could hope to handle that? I guess by returning ""
instead of None
for all values? Not sure if that would cause more trouble than it solves...
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.
Seems like it should be okay... I've pushed a change to return an empty string
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.
@gshank I see you made the change to use ""
instead of None
. I think it's the right call. The only way I could think of this breaking is if:
There's a specific node config that acceptsConfirmed that config-level validation doesn't happen inNone
, but notstr
deps
/clean
, it's when we're parsing + validating actual nodes- A user has defined an expression like:
models:
# target.port is numeric when set, but it will be "" in deps/clean
+query_tag: "{{ target.port + 1234 }}"
That doesn't work with NoneType
either, so I think we're either at parity or strictly better off with a string:
can only concatenate str (not "int") to str
unsupported operand type(s) for +: 'NoneType' and 'int'
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.
Here's where I miss Perl... :). It would have treated the empty string as 0 here.
3c7ac51
to
1addf77
Compare
resolves #4752
Description
After changes in #4610, direct access to target. fails with cryptic message "cannot pickle undefined object", because an empty dictionary was returned for the target. This changes the behavior of deps and clean commands to return a dict subclass that returns None for attempts to access a dictionary key.
Checklist
CHANGELOG.md
and added information about my change