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.

Sign up for GitHub

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: exit with success boolean #34

Closed
wants to merge 1 commit into from
Closed

fix: exit with success boolean #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion dbt_py/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,5 @@ def main() -> None:
- https://docs.getdbt.com/reference/programmatic-invocations
"""
dbt.context.base.get_context_modules = new_get_context_modules
dbt.cli.main.dbtRunner().invoke(sys.argv[1:])
result = dbt.cli.main.dbtRunner().invoke(sys.argv[1:])
return not result.success
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for catching this and raising the PR!

I'm just trying to understand the change -- have you confirmed that adding the return would push the correct exit code?

Looking at the dbt docs for the programmatic invocation, I was expecting something like:

Suggested change
return not result.success
if result.success:
sys.exit(0)
elif result.exception is None:
sys.exit(1)
else:
sys.exit(2)

Given that people are actually using this repo, I'll also get the integration tests fixed this afternoon

Copy link
Author

@alexmaras alexmaras Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks - I was mainly looking for the failure case in general. The setup you've got is more correct, my fix was just a quick one I did for a CI pipeline. Glad to see there's proper guidance on how to deal with it. For reference, return does indeed produce correct statuses, though sys.exit is likely more correct.