-
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
Compile on-run-(start|end) hooks to file #412
Conversation
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 looks pretty good. there is a lot of messy conditional logic in runner.py and compilation.py but you don't need to deal with that all here.
there are a few questions in here, wondering why you did a couple specific things in here.
if you fix the circle build, this is ok from my end
dbt/compilation.py
Outdated
for name, node in flat_graph.get('nodes').items(): | ||
self.link_node(linker, node, flat_graph) | ||
linked_graph['nodes'][name] = node | ||
for node_type in ['nodes', 'operations']: |
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.
curious why you put operations in a separate subgraph
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.
yeah, i think you're right -- these can in the nodes
graph... let me investigate
@@ -331,6 +331,52 @@ def load_and_parse_sql(package_name, root_project, all_projects, root_dir, | |||
return parse_sql_nodes(result, root_project, all_projects, tags) |
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.
looks like this file was chmod +x
ed -- can you undo that
dbt/parser.py
Outdated
'raw_sql': hooks | ||
}) | ||
|
||
return parse_sql_nodes(result, root_project, all_projects, tags={hook_type}) |
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.
looks great
if is_model and is_ephemeral: | ||
res.append(ancestor) | ||
|
||
return set(res) |
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.
👍
dbt/runner.py
Outdated
] | ||
|
||
return self.run_types_from_graph(include_spec, | ||
exclude_spec, | ||
resource_types=resource_types, | ||
tags=set(), | ||
should_run_hooks=False, | ||
should_execute=False) | ||
should_execute=False, | ||
flatten_graph=True) |
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.
why flatten the graph 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.
debugging! good catch
this fixes #403 |
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.
one really dumb comment, but this lgtm
dbt/utils.py
Outdated
@@ -31,6 +31,12 @@ class NodeType(object): | |||
Operation = 'operation' | |||
|
|||
|
|||
class RunHookTypes: |
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.
RunHookType
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.
reasonable
compile on-run-(start|end) hooks to a file automatic commit by git-black, original commits: ce46052
@cmcarthur i'm not super happy with this yet -- going to try to clean up a little in the AM