Skip to content
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

dbt cli get_error() #21999

Merged
merged 1 commit into from
May 22, 2024
Merged

dbt cli get_error() #21999

merged 1 commit into from
May 22, 2024

Conversation

johannkm
Copy link
Member

@johannkm johannkm commented May 21, 2024

When using dbt.cli(..., raise_on_error=False), users may still want to retrieve the error that would have been raised. _errors is private, so they couldn't put this together themselves either.

@johannkm johannkm marked this pull request as ready for review May 21, 2024 17:08
@johannkm
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @johannkm and the rest of your teammates on Graphite Graphite

@@ -656,6 +656,45 @@ def is_successful(self) -> bool:

return self.process.wait() == 0

@public
def get_error(self) -> Optional[Exception]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

Suggested change
def get_error(self) -> Optional[Exception]:
def raise_on_error(self) -> NoReturn:
  • Make it raise a DagsterInvariantException or something if a user tries to retrieve an error from a successful invocation
  • Otherwise raise instead of return
  • Since this function is guaranteed to NoReturn, then we should force it consume the entire event stream before return so we give the most verbose error, with error logs parsed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this suggestion except for raising the DagsterInvariantException. NoReturn is cool but the name to implies a no-op if there's no error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name would also conflict with the current raise_on_error: bool property

@johannkm johannkm force-pushed the johann/05-21-dbt_cli_get_error_ branch 2 times, most recently from feef73a to 4c15b0f Compare May 21, 2024 19:08
@johannkm johannkm dismissed rexledesma’s stale review May 21, 2024 19:09

updated to always wait() so we have more context in all error messages

@johannkm johannkm requested a review from rexledesma May 21, 2024 19:09
@johannkm johannkm force-pushed the johann/05-21-dbt_cli_get_error_ branch from 4c15b0f to ed0a029 Compare May 22, 2024 18:03
@johannkm johannkm force-pushed the johann/05-21-dbt_cli_get_error_ branch from ed0a029 to c626576 Compare May 22, 2024 18:21
@johannkm johannkm merged commit f64647f into master May 22, 2024
1 check passed
@johannkm johannkm deleted the johann/05-21-dbt_cli_get_error_ branch May 22, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants