feat(dbt): Adding flag to raise an error when a model sync fails #266
Conversation
src/preset_cli/cli/main.py
Outdated
| ), | ||
| ) | ||
| sys.exit(1) | ||
| raise_error(1, "Couldn't read credentials") |
There was a problem hiding this comment.
A better approach here would be to define a new exception:
class CLIError(Exception):
def __init__(self, code: int, message: str):
...Then here you can do:
except Exception as ex:
raise CLIError(1, "Couldn't read credentials") from exAnd then we can either add a decorator to the command that catches CLIError and displays the message and exits, or we can catch it explicitly where we call this function.
With raise CLIError() it's clear that the code execution stops there (since it raises an exception). The problem with raise_error() is that there's no indication that the flow will stop, unless I check the function and see that it calls sys.exit() at the end.
There was a problem hiding this comment.
ohh that's a good idea! let me look into that!
| preset_cli.add_command(auth, name="auth") | ||
| preset_cli.add_command(invite_users, name="invite-users") | ||
| preset_cli.add_command(import_users, name="import-users") |
There was a problem hiding this comment.
For some reason, for the commands that involved the raise_cli_errors decorator, the tests were failing with command not found until I explicitly added the command names. 🤷
| if raise_failures: | ||
| warnings.simplefilter("always", DeprecationWarning) |
There was a problem hiding this comment.
I added this otherwise the DeprecationWarning doesn't get logged in the terminal.
| @@ -40,6 +42,7 @@ def setup_logging(loglevel: str) -> None: | |||
| handlers=[RichHandler()], | |||
| force=True, | |||
| ) | |||
| logging.captureWarnings(True) | |||
There was a problem hiding this comment.
Also add to add this to include DeprecationWarnings in the terminal
| @@ -54,7 +54,7 @@ repos: | |||
| # additional_dependencies: [flake8-bugbear] | |||
|
|
|||
| - repo: https://github.com/pre-commit/mirrors-mypy | |||
| rev: 'v0.910' # Use the sha / tag you want to point at | |||
| rev: 'v0.981' # Use the sha / tag you want to point at | |||
There was a problem hiding this comment.
Had to bump it to fix pre-commit for newer Python versions. python/mypy#13627
Currently, during the dbt sync models might fail to be synced. The operation is not atomic (a sync failure won't prevent other models from being synced) and the CLI logs the error messages.
While the error messages are helpful to debug the issue, when using the CLI in a CI/CD pipeline, the fact that the execution ends up successfully (doesn't return a failed status) results in a successful job, which could be misleading to admins since there were failures.
This PR adds a new flag
--raise-failuresthat when used would end the command execution with an error status code in case any model failed to sync, and also list failures. This can be further used in the future when failing to sync incompatible metrics.Also added a deprecation logic, so using the
--raise-failuresflag would also return an error code in case the command was triggered using deprecated features. This is going to be used in the future when deprecating--preserve-columns(replaced by--preserve-metadata).