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

[CT-999] [Bug] In 1.2.0 implictly imported exit being used instead of sys.exit #5621

Closed
2 tasks done
varun-dc opened this issue Aug 5, 2022 · 2 comments · Fixed by #5627
Closed
2 tasks done

[CT-999] [Bug] In 1.2.0 implictly imported exit being used instead of sys.exit #5621

varun-dc opened this issue Aug 5, 2022 · 2 comments · Fixed by #5627
Labels
bug Something isn't working init Issues related to initializing the dbt starter project
Milestone

Comments

@varun-dc
Copy link
Contributor

varun-dc commented Aug 5, 2022

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

In d257d0b, this new bit of code was added,

+        available_adapters = list(_get_adapter_plugin_names())
+        if not len(available_adapters):
+            print("No adapters available. Go to https://docs.getdbt.com/docs/available-adapters")
+            exit(1)

The exit(1) in its current form seems like the wrong thing to do, as per the Python docs,

The site module (which is imported automatically during startup, except if the -S command-line option is given) adds several constants to the built-in namespace. They are useful for the interactive interpreter shell and should not be used in programs.

Since exit is provided by the implicitly imported site, and docs suggest,

should not be used in programs

Expected Behavior

The better thing to do is, import sys and sys.exit(1)

Steps To Reproduce

The easiest way to see this fail is with python -S and then >>> exit(1), this will fail with,

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'exit' is not defined

Relevant log output

No response

Environment

- OS: any
- Python: 3+
- dbt: 1.2.0

Which database adapter are you using with dbt?

No response

Additional Context

I'm a bit unsure about how the branching and release process work for dbt-core.
The change is only in the 1.2.latest branch, not in main.

Will this change get cherry picked onto main? Right now 1.2.0 suffers from this issue only

@varun-dc varun-dc added bug Something isn't working triage labels Aug 5, 2022
@github-actions github-actions bot changed the title [Bug] In 1.2.0 implictly imported exit being used instead of sys.exit [CT-999] [Bug] In 1.2.0 implictly imported exit being used instead of sys.exit Aug 5, 2022
@jtcohen6 jtcohen6 added Team:Execution init Issues related to initializing the dbt starter project and removed triage labels Aug 7, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 8, 2022

@varun-dc Thanks for opening!

If I follow you, it sounds like we should simply be swapping out exit(1) for:

import sys
sys.exit(1)

Which makes sense to me!

To be clear, this code does also live on the main branch of dbt-core as well:

available_adapters = list(_get_adapter_plugin_names())
if not len(available_adapters):
print("No adapters available. Go to https://docs.getdbt.com/docs/available-adapters")
exit(1)

We'd want to merge the fix into main first, and then backport it to 1.2.latest for inclusion in a v1.2.x patch release.

Is this a change you'd be interested in contributing this week? Otherwise, someone from our side can swing around to it, to incorporate in a patch sooner rather than later.

@jtcohen6 jtcohen6 added this to the v1.2.x milestone Aug 8, 2022
@varun-dc
Copy link
Contributor Author

varun-dc commented Aug 8, 2022

I'll push up a PR for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working init Issues related to initializing the dbt starter project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants