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

Add dbt plugin version #2279

Merged

Conversation

sumanau7
Copy link
Contributor

@sumanau7 sumanau7 commented Mar 31, 2020

resolves #2272

Description

Add DBT plugins version information in get_version_information

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot
Copy link

cla-bot bot commented Mar 31, 2020

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sumanau Sareen.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@sumanau7
Copy link
Contributor Author

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Mar 31, 2020

The cla-bot has been summoned, and re-checked this pull request!

@beckjake
Copy link
Contributor

beckjake commented Mar 31, 2020

Cool! One thing is that I think I'd rather avoid using pkg_resources! My thought was to use importlib.util.find_spec, I had something like this:

for path in importlib.util.find_spec('dbt').submodule_search_locations:
    plugin_root, _ = os.path.split(path)
    _, plugin_name = os.path.split(plugin_root)
    if plugin_name == 'core':
        continue
    try:
        mod = importlib.import_module(f'dbt.adapters.{plugin_name}.__version__')
    except ImportError:
        # not an adapter
        continue
    print(mod.version)

Then we'd add a __version__.py in each plugin alongside its impl.py, with version = 0.17.0a1 (or whatever), and add those paths to .bumpversion.cfg.

My code is just demo code - obviously you'd need to handle the case where mod.version doesn't exist. And I have not tested it in any meaningful way on an installed dbt pacakge.

A key element of my thought process here is that I don't want dbt to have to know the names when when it starts looking for versions! That's the nice feature of this model - if a user writes a third-party plugin, dbt will still report its version in dbt --version.

@sumanau7
Copy link
Contributor Author

Cool! One thing is that I think I'd rather avoid using pkg_resources! My thought was to use importlib.util.find_spec, I had something like this:

Agree with your approach of using importlib.

A key element of my thought process here is that I don't want dbt to have to know the names when when it starts looking for versions!
This will help in removing the hard coded the plugin names in the code.

Then we'd add a __version__.py in each plugin alongside its impl.py

Now as per Pep 396, this is the advisable approach to define version and also setup.py should also read version from the same file, but as the pep specification are voluntarily, so its not widely adopted.

What if we can use importlib to find dbt plugins and then instead of reading version.py we instead read the version from pkg_resource, OR we can increase the scope of this PR to add documentation explaining its recommended that dbt plugins always define a version.py file, which is what you are recommending as well. thoughts ?

@beckjake
Copy link
Contributor

beckjake commented Mar 31, 2020

What if we can use importlib to find dbt plugins and then instead of reading version.py we instead read the version from pkg_resource, OR we can increase the scope of this PR to add documentation explaining its recommended that dbt plugins always define a version.py file, which is what you are recommending as well. thoughts ?

At least in the short term, I think we should just require ignore plugins that don't have it. That way this isn't a breaking change (always nice!). In the future we might want to make it a hard requirement (perhaps we'll want to inject the adapter plugin version into the context or something), but I don't see any reason to do so right now.

One thing we can do is make sure the changelog update is explicit, something like:

Added dbt plugins to version output. Plugins now require a `__version__.py` in their `adapters/` directory

We can expand on it in the "upgrading to dbt 0.17.0" page in the docs if we find that compelling.
We should also add it to #2145 - that will help.

I think in general those will suffice.

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

This isn't me prodding you or adding to the previous feedback - I'm just setting this so our github reviews plugin leaves me alone 😄

@sumanau7
Copy link
Contributor Author

sumanau7 commented Apr 2, 2020

No issues, i plan to finish pending PR over this weekend, got quite busy from last few days.

@sumanau7
Copy link
Contributor Author

sumanau7 commented Apr 3, 2020

A key element of my thought process here is that I don't want dbt to have to know the names when when it starts looking for versions!

@beckjake I couldn't get importlib to work as expected, hence taking your idea of not hardcoding the plugin names i have used pkg_resources to implement the same, this time only hard-coding the keyword core and naming convention of dbt plugins, let me know what do you think ?

core/dbt/version.py Outdated Show resolved Hide resolved
@beckjake
Copy link
Contributor

beckjake commented Apr 3, 2020

I couldn't get importlib to work as expected, hence taking your idea of not hardcoding the plugin names i have used pkg_resources to implement the same, this time only hard-coding the keyword core and naming convention of dbt plugins, let me know what do you think ?

The problem with that approach is that not all packages that start with dbt- are plugins, and we don't want to display those, either!

I'm pretty confident importlib will work fine, here are my suggestions for getting it working:

  • rebase off dev/octavius-catto. Currently your branch is branched off 0.14.1, which is quite far behind at this point. I don't recall how that impacts plugin loading, but it potentially doesn't behave like I'd expect! We'll need this PR to be based on dev/octavius-catto regardless, so you might as well do it now.
  • ensure that you're installing dbt-core and the plugins in "editable" mode. The easiest way to do this is set up a fresh virtualenv and run pip install -r editable_requirements.txt. Then when you edit dbt locally, the changed behavior will show up in that virtualenv. Without this, it will look like the following step doesn't do anything.
  • add a __version__.py file containing version = '0.17.0a1' to each of the plugins' adapters subdirectory. That will match the __version__ variable in dbt/core/version.py once you've rebased.

I'm think that once you do that, you can just take the code I suggested in a previous comment, paste it in place of _get_dbt_plugins_info, and change print(mod.name) to yield plugin_name, mod.version.

@cla-bot
Copy link

cla-bot bot commented Apr 4, 2020

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sumanau Sareen.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla:yes label Apr 4, 2020
@sumanau7 sumanau7 requested a review from beckjake April 4, 2020 19:34
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

This looks great! Can you update the CHANGELOG.md file to reflect your additions?
The CHANGLEOG update should be like the rest of the changelog entries: A quick summary - Add dbt plugin versions to --version should be fine, a link to this PR+issue, and an addtion to the Contributors section with your github username + a link to this PR.

Once you do that, I'll kick off the rest of the tests and then this will be good to go into 0.17.0!

Thanks for contributing to dbt 😄

@cla-bot
Copy link

cla-bot bot commented Apr 6, 2020

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sumanau Sareen.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla:yes label Apr 6, 2020
@cla-bot cla-bot bot added the cla:yes label Apr 6, 2020
@sumanau7
Copy link
Contributor Author

sumanau7 commented Apr 6, 2020

@beckjake updated.

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

Almost there! This is annoying, but can you remove the spaces between these? Otherwise github won't render them.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Jacob Beck <beckjake@users.noreply.github.com>
@beckjake
Copy link
Contributor

beckjake commented Apr 6, 2020

I've kicked off tests, they should start showing up on github as passing in a minute or two

@beckjake beckjake merged commit 2c24102 into dbt-labs:dev/octavius-catto Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"dbt --version" should also emit plugin versions
2 participants