adapter compability messaging added. #4565

nkyuray · 2022-01-11T17:50:10Z

Description

adapter compatibility messaging added.
#4438 (comment)

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

ChenyuLInx · 2022-01-12T14:40:41Z

Hey @nkyuray, thanks for contributing this! I will have more bandwidth to test this next Tuesday

joellabes · 2022-01-18T10:07:42Z

core/dbt/version.py

+            plugin_update_msg = ("  To update dbt-{} please run:\n"
+                                 "    pip install dbt-{} --upgrade\n\n".format(plugin_name, plugin_name))


Love the really actionable feedback here! My only comment is that not all users install dbt via pip - I really like the idea of making the instructions easy, but it'd be confusing to give pip instructions to someone who installed dbt via Homebrew etc.

Not sure how to square that circle; perhaps we have to fall back to linking them to the docs?

Looks like the pip message is provided as an example in the original feature request. But I agree with @joellabes 's fallback to docs idea here. It would make later updates easier also.

Hi @ChenyuLInx @joellabes

Ok, then simply we need to put a link of related docs page, right?
Besides the yellow one will be added also.

Thanks.

That would be perfect! Thanks @nkyuray

ChenyuLInx · 2022-01-21T22:26:21Z

@nkyuray One more thing, don't forget to add yourself to the changelog. So we can give you credit on the release note(we are looking for ways to automate this, but right now you have to do it manually). Here's a example.

ChenyuLInx · 2022-01-26T22:39:49Z

@nkyuray, we are thinking of getting this into the 1.0.2 release that aims to have an rc version later next week. We think more folks would appreciate and benefit from what you are adding here! Do you think you get time to work on it before that? If not we can either merge this in and leave the rest as an improvement or I am happy to update it a bit if you are okay with it!

nkyuray · 2022-01-26T22:54:43Z

Hi @ChenyuLInx,

Sorry I couldn't take time to work on this, but I will probably finalize it on Friday.
Thanks.

ChenyuLInx · 2022-01-27T16:07:12Z

Hi @ChenyuLInx,

Sorry I couldn't take time to work on this, but I will probably finalize it on Friday. Thanks.

Hi @nkyuray, please don't say sorry. Thank you so much for contributing this!!

ChenyuLInx

Looks great except for one minor part that needs to be adjusted. Thanks for pushing this on the weekend!!!

ChenyuLInx · 2022-01-31T14:56:23Z

core/dbt/version.py

+            compatibility_msg = green('Up to date!')
+        else:
+            if installed.major == plugin_version.major:
+                compatibility_msg = yellow('Update available!')


Maybe we also need to check whether a new version of the package is actually available? I just tested locally, and because my dbt-core is 1.0.1, but the latest dbt-snowflake is only 1.0.0, I will always get this warning even though there's no newer dbt-snowflake package. I would just make the check above to be checking major and minor versions only instead of the whole version since we don't always release the patch version of the adaptors when we release dbt-core.

Agreed! I think we could take one of two approaches here:

Compare {major}.{minor} only, to ensure that the plugin version is compatible with the dbt-core version

Run PyPi requests for each installed plugin, to see what its latest available version is

In the future we'd want a combination of both, right? Goal being to say whether installed plugins are Latest compatible with the installed dbt-core version

core/dbt/version.py

ChenyuLInx · 2022-01-31T15:05:36Z

I just run the tests and noticed a few minor flake8 checks failed. Currently, you have to run it by yourself. We are going to have some tutorial on getting this into your pre-commit hooks soon.

jtcohen6 · 2022-02-03T10:48:23Z

I just run the tests and noticed a few minor flake8 checks failed. Currently, you have to run it by yourself. We are going to have some tutorial on getting this into your pre-commit hooks soon.

Per CI run, it looks like the flake8 failures are just:

core/dbt/version.py:62:100: E501 line too long (110 > 99 characters)
core/dbt/version.py:65:100: E501 line too long (103 > 99 characters)

ChenyuLInx

updated compare and added pulling from PYPI. @nkyuray Thank you so much for doing this!!! Sorry we really want this change in 1.0.2 so I just changed it a bit myself.

* adapter compability messaging added. * edited plugin version compatibility message * edited test version for plugin compability * compare using only major and minor * Add checking PYPI and update changelog Co-authored-by: Chenyu Li <[email protected]> Co-authored-by: ChenyuLi <[email protected]>

jtcohen6

This is a great change! Thank you @nkyuray for the contribution, and @ChenyuLInx for seeing this through.

I think there are a few small intricacies that may require us to revisit this logic in the future. I left two comments with edge cases that come to mind. If those seem like quick fixes, all the better. In either case, I'm happy to move forward with the current logic for v1.0.2rc1.

jtcohen6 · 2022-02-04T08:44:40Z

core/dbt/version.py

+        if installed == plugin_version or (
+            latest_plugin_version and plugin_version == latest_plugin_version
+        ):


It's possible that plugins will put out patch releases before dbt-core puts out a patch release. Imagine:

dbt-core==1.0.1 and dbt-synapse==1.0.2 are both available

I have dbt-synapse==1.0.1 installed locally

installed == plugin_version will return True, at the same time, there's a compatible plugin update available

I didn't realize this case, and the rule in the next comment, I will open a new issue and get these changes in

jtcohen6 · 2022-02-04T08:46:24Z

core/dbt/version.py

+            compatibility_msg = green('Up to date!')
+        else:
+            if latest_plugin_version:
+                if installed.major == plugin_version.major:


dbt-core + plugins are compatible at the minor-version level. So I think this might want to be:

Suggested change

if installed.major == plugin_version.major:

if installed.minor == plugin_version.minor:

If installed.minor != plugin_version.minor, we'd want to raise red('Not compatible!')

* adapter compability messaging added. (#4565) * adapter compability messaging added. * edited plugin version compatibility message * edited test version for plugin compability * compare using only major and minor * Add checking PYPI and update changelog Co-authored-by: Chenyu Li <[email protected]> Co-authored-by: ChenyuLi <[email protected]> * fix changelog * fix changelog Co-authored-by: nkyuray <[email protected]>

* adapter compability messaging added. * edited plugin version compatibility message * edited test version for plugin compability * compare using only major and minor * Add checking PYPI and update changelog Co-authored-by: Chenyu Li <[email protected]> Co-authored-by: ChenyuLi <[email protected]> automatic commit by git-black, original commits: f3c7b6b

adapter compability messaging added.

4689318

leahwicz requested a review from ChenyuLInx January 11, 2022 20:38

jtcohen6 mentioned this pull request Jan 18, 2022

[CT-54] [Bug] dbt --version doesn’t identify patch releases as candidates for upgrade #4582

Closed

1 task

joellabes reviewed Jan 18, 2022

View reviewed changes

jtcohen6 added the Team:Execution label Jan 21, 2022

edited plugin version compatibility message

e4e8e97

nkyuray requested a review from a team as a code owner January 30, 2022 17:45

nkyuray requested review from leahwicz and removed request for a team January 30, 2022 17:45

edited test version for plugin compability

ff8ca6b

cla-bot bot added the cla:yes label Jan 30, 2022

ChenyuLInx requested changes Jan 31, 2022

View reviewed changes

ChenyuLInx and others added 2 commits February 3, 2022 10:15

compare using only major and minor

433f9fa

Add checking PYPI and update changelog

260e2bb

ChenyuLInx approved these changes Feb 3, 2022

View reviewed changes

leahwicz approved these changes Feb 3, 2022

View reviewed changes

ChenyuLInx merged commit f3c7b6b into dbt-labs:main Feb 3, 2022

jtcohen6 reviewed Feb 4, 2022

View reviewed changes

jtcohen6 mentioned this pull request Feb 9, 2022

[CT-18] Add adapter compatibility messaging to dbt --version #4438

Closed

ChenyuLInx mentioned this pull request Feb 14, 2022

[CT-229] Adapter compatible message rule update #4724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter compability messaging added. #4565

adapter compability messaging added. #4565

nkyuray commented Jan 11, 2022 •

edited by ChenyuLInx

Loading

ChenyuLInx commented Jan 12, 2022

joellabes Jan 18, 2022

ChenyuLInx Jan 18, 2022

nkyuray Jan 18, 2022

ChenyuLInx Jan 18, 2022

ChenyuLInx commented Jan 21, 2022

ChenyuLInx commented Jan 26, 2022

nkyuray commented Jan 26, 2022

ChenyuLInx commented Jan 27, 2022

ChenyuLInx left a comment

ChenyuLInx Jan 31, 2022

jtcohen6 Feb 3, 2022

ChenyuLInx commented Jan 31, 2022

jtcohen6 commented Feb 3, 2022

ChenyuLInx left a comment

jtcohen6 left a comment

jtcohen6 Feb 4, 2022

ChenyuLInx Feb 9, 2022

jtcohen6 Feb 4, 2022

		plugin_update_msg = (" To update dbt-{} please run:\n"
		" pip install dbt-{} --upgrade\n\n".format(plugin_name, plugin_name))

	if installed.major == plugin_version.major:
	if installed.minor == plugin_version.minor:

adapter compability messaging added. #4565

adapter compability messaging added. #4565

Conversation

nkyuray commented Jan 11, 2022 • edited by ChenyuLInx Loading

Description

Checklist

ChenyuLInx commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx commented Jan 21, 2022

ChenyuLInx commented Jan 26, 2022

nkyuray commented Jan 26, 2022

ChenyuLInx commented Jan 27, 2022

ChenyuLInx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx commented Jan 31, 2022

jtcohen6 commented Feb 3, 2022

ChenyuLInx left a comment

Choose a reason for hiding this comment

jtcohen6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkyuray commented Jan 11, 2022 •

edited by ChenyuLInx

Loading