-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 run operation return codes (#1377) #1406
Changes from 4 commits
13dd720
a72a4e1
5b74c58
4730789
08c5f9a
ca02a58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
from dbt.logger import GLOBAL_LOGGER as logger | ||
|
||
from dbt.task.base import BaseTask | ||
from dbt.task.base import ConfiguredTask | ||
from dbt.adapters.factory import get_adapter | ||
from dbt.loader import GraphLoader | ||
|
||
|
@@ -9,7 +8,7 @@ | |
import dbt.exceptions | ||
|
||
|
||
class RunOperationTask(BaseTask): | ||
class RunOperationTask(ConfiguredTask): | ||
def _get_macro_parts(self): | ||
macro_name = self.args.macro | ||
if '.' in macro_name: | ||
|
@@ -22,19 +21,44 @@ def _get_macro_parts(self): | |
def _get_kwargs(self): | ||
return dbt.utils.parse_cli_vars(self.args.args) | ||
|
||
def run(self): | ||
def _run_unsafe(self): | ||
manifest = GraphLoader.load_all(self.config) | ||
adapter = get_adapter(self.config) | ||
|
||
package_name, macro_name = self._get_macro_parts() | ||
macro_kwargs = self._get_kwargs() | ||
|
||
res = adapter.execute_macro( | ||
macro_name, | ||
project=package_name, | ||
kwargs=macro_kwargs, | ||
manifest=manifest, | ||
connection_name="macro_{}".format(macro_name) | ||
) | ||
with adapter.connection_named('macro_{}'.format(macro_name)): | ||
res = adapter.execute_macro( | ||
macro_name, | ||
project=package_name, | ||
kwargs=macro_kwargs, | ||
manifest=manifest | ||
) | ||
adapter.commit_if_has_connection() | ||
|
||
return res | ||
|
||
def run(self): | ||
try: | ||
result = self._run_unsafe() | ||
except dbt.exceptions.Exception as exc: | ||
logger.error( | ||
'Encountered a dbt exception while running a macro: {}' | ||
.format(exc) | ||
) | ||
logger.debug('', exc_info=True) | ||
return False, None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we conceivably return a dict that encodes this error message? That would be useful for an eventual API that wants to do something with the results when an operation fails. I think we can leave this as-is for now, but want to keep it in the back of our minds when we revisit the RunResults node contract. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, though in the RPC case I'd probably reach for overriding the |
||
except Exception as exc: | ||
logger.error( | ||
'Encountered an uncaught exception while running a macro: {}' | ||
.format(exc) | ||
) | ||
logger.debug('', exc_info=True) | ||
return False, None | ||
else: | ||
return True, result | ||
|
||
def interpret_results(self, results): | ||
success, _ = results | ||
return success |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
{% macro no_args() %} | ||
{% if execute %} | ||
{% call statement() %} | ||
create table "{{ schema }}"."no_args" (id int) | ||
{% endcall %} | ||
{% endif %} | ||
{% endmacro %} | ||
|
||
|
||
{% macro table_name_args(table_name) %} | ||
{% if execute %} | ||
{% call statement() %} | ||
create table "{{ schema }}"."{{ table_name }}" (id int) | ||
{% endcall %} | ||
{% endif %} | ||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{% macro syntax_error() %} | ||
{% if execute %} | ||
{% call statement() %} | ||
select NOPE NOT A VALID QUERY | ||
{% endcall %} | ||
{% endif %} | ||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
from test.integration.base import DBTIntegrationTest, use_profile | ||
import yaml | ||
|
||
|
||
class TestOperations(DBTIntegrationTest): | ||
@property | ||
def schema(self): | ||
return "run_operations_044" | ||
|
||
@property | ||
def models(self): | ||
return "test/integration/044_run_operations_test/models" | ||
|
||
@property | ||
def project_config(self): | ||
return { | ||
"macro-paths": ['test/integration/044_run_operations_test/macros'], | ||
} | ||
|
||
def run_operation(self, macro, expect_pass=True, extra_args=None, **kwargs): | ||
args = ['run-operation'] | ||
if macro: | ||
args.extend(('--macro', macro)) | ||
if kwargs: | ||
args.extend(('--args', yaml.safe_dump(kwargs))) | ||
if extra_args: | ||
args.extend(extra_args) | ||
return self.run_dbt(args, expect_pass=expect_pass) | ||
|
||
@use_profile('postgres') | ||
def test__postgres_macro_noargs(self): | ||
self.run_operation('no_args') | ||
self.assertTableDoesExist('no_args') | ||
|
||
@use_profile('postgres') | ||
def test__postgres_macro_args(self): | ||
self.run_operation('table_name_args', table_name='my_fancy_table') | ||
self.assertTableDoesExist('my_fancy_table') | ||
|
||
@use_profile('postgres') | ||
def test__postgres_macro_exception(self): | ||
self.run_operation('syntax_error', False) | ||
|
||
@use_profile('postgres') | ||
def test__postgres_macro_missing(self): | ||
self.run_operation('this_macro_does_not_exist', False) | ||
|
||
@use_profile('postgres') | ||
def test__postgres_cannot_connect(self): | ||
self.run_operation('no_args', | ||
extra_args=['--target', 'noaccess'], | ||
expect_pass=False) |
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 an exception that dbt is aware of, but it's not a dbt exception per se. I just don't want folks to think that dbt did something wrong if their operations fail. Can we make this say: