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-909] Adapter Management works with Click #5533

Closed
Tracked by #5527
iknox-fa opened this issue Jul 25, 2022 · 3 comments · Fixed by #5892
Closed
Tracked by #5527

[CT-909] Adapter Management works with Click #5533

iknox-fa opened this issue Jul 25, 2022 · 3 comments · Fixed by #5892

Comments

@iknox-fa
Copy link
Contributor

iknox-fa commented Jul 25, 2022

User Story:
As a user of dbt, adapters continue to work as they do currently

Acceptance Criteria:

  • The logic found in the adapter_management context manager is moved to the root click group and propagates down to all subcommands
@github-actions github-actions bot changed the title Adapter Management works with Click [CT-909] Adapter Management works with Click Jul 25, 2022
@jtcohen6
Copy link
Contributor

I agree with the scope of this ticket, which is that adapters should continue to work exactly as they do today, with the new CLI + top-level API.

One thing I wanted to share, in case it's interesting, is that I've been experimenting with making adapters less "global": https://github.com/dbt-labs/dbt-core/compare/jerco/experiment-deglobalize-adapters

The idea in that branch:

  • Store adapter object on RuntimeConfig, instead of registering / loading from the global AdapterFactory
  • The AdapterFactory is still the global registry for adapter plugins, which are library code (classes) used to create specific adapter objects. I think this is a safe assumption unless we’re going to be installing / adding new plugin code midway through dbt-core applications.

I think that's a path we should probably continue pursuing. It means that we might not need the same kind of adapter_management context management that we have currently, which requires us to reset adapters as the very first step. (We should still clean up connections at the end of every task/command.)

@iknox-fa
Copy link
Contributor Author

iknox-fa commented Sep 20, 2022

@jtcohen6
I love the idea, but it ended up being only one line of code to move this to the Click world as-is so the global state persists for now.

@jtcohen6
Copy link
Contributor

@iknox-fa great solution to the scope of this initial ticket!

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 a pull request may close this issue.

2 participants