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

fix: exit with success boolean #34

Closed
wants to merge 1 commit into from
Closed

Conversation

alexmaras
Copy link

@alexmaras alexmaras commented Oct 10, 2024

This makes the exit code 1 when an error occurred in the underlying DBT invocation, and zero otherwise. It's particularly necessary for CI, where the current code will not surface errors

Summary by Sourcery

Fix the exit code behavior to return a non-zero code on DBT invocation errors, improving error detection in CI environments.

Bug Fixes:

  • Ensure the program exits with a non-zero code when an error occurs during the DBT invocation, allowing CI to correctly detect failures.

@alexmaras alexmaras requested a review from Bilbottom as a code owner October 10, 2024 23:44

This comment was marked as resolved.

sourcery-ai[bot]

This comment was marked as resolved.

@@ -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

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.

@Bilbottom
Copy link
Owner

@alexmaras I've raised #37 with this change, as I've also added some integration tests and updated the project version (I don't have automated releases yet)

Let me know if you're happy with this alternative, and I'll get it released (I'll merge it in if I don't hear anything, probs tomorrow)

Thanks for the contribution 🎉

@alexmaras
Copy link
Author

Thanks @Bilbottom - that looks great. I was just getting a quick fix in place for CI pipelines, your solutions is much more solid. I was going to ask about test processes and stuff, but it looks like you've covered that off in there. Thanks for that! I'll close this one.

@alexmaras alexmaras closed this Oct 12, 2024
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.

2 participants