-
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
Some cleanups to dbtshelltask #2980
Conversation
@@ -86,10 +86,12 @@ def __init__( | |||
self.dbt_kwargs = dbt_kwargs | |||
super().__init__( | |||
**kwargs, | |||
command=command, |
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.
FYI command
and env
weren't being forwarded here and thus were overwritten to None in the super method, so they couldn't be provided at initialization
# when set to `return_all=True`, should return a list | ||
assert isinstance(out, list) | ||
# check that the result is multiple lines | ||
assert len(out) > 1 |
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.
FYI the output of dbt --version
I am depending on looks like this:
(prefect_dev) ➜ prefect git:(cleanup-on-dbt-task-return) dbt --version
installed version: 0.17.1
latest version: 0.17.1
Up to date!
Plugins:
- bigquery: 0.17.1
- snowflake: 0.17.1
- redshift: 0.17.1
- postgres: 0.17.1
Thanks for contributing to Prefect!
Please describe your work and make sure your PR:
changes/
directory (if appropriate)docs/outline.toml
for API reference docs (if appropriate)Note that your PR will not be reviewed unless all three boxes are checked.
What does this PR change?
First off, adds
return
that I think just got missed in the original dbtshelltask. Otherwise the stdout from dbt being run in the shell is not retrieved as the return value of the task, against what is says in its docstrings and the intention of ShellTask itself and presumably its derivatives. This was brought up in this slack thread: https://prefect-community.slack.com/archives/CL09KU1K7/p1594912760250100While writing tests I realized that the super method was also failing to forward
command
andenv
kwargs which made the@default_from_attrs
functionality not work properly for those two, so I fixed that too.And while running tests that actually called dbt I realized that it wasn't being added as an
extras
option in setup.py (and thus it wasn't really possible to meaningfully run the tests on CI) so I've added that as well.Why is this PR important?
So that people can use the stdout from the DbtShellTask for anything downstream.