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.

Sign up for GitHub

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

Jump to bottom

update adapter version messages #10919

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

update adapter version messages #10919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7d3cc4
update tests for mismatching plugins
dave-connors-3 Oct 25, 2024
1d9a0ee
changie
dave-connors-3 Oct 25, 2024
3529df4
add patch version test case
dave-connors-3 Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20241025-104339.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: update adapter version messages
time: 2024-10-25T10:43:39.274723-05:00
custom:
Author: dave-connors-3
Issue: "10230"
6 changes: 5 additions & 1 deletion core/dbt/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,11 @@ def _get_plugin_msg_info(

needs_update = False

if plugin.major != core.major or plugin.minor != core.minor:
assert plugin.major is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

asserts shouldn't be used in production, see: https://snyk.io/blog/the-dangers-of-assert-in-python/

assert plugin.minor is not None
assert core.major is not None
assert core.minor is not None
if plugin.major != core.major or plugin.minor < core.minor:
Copy link
Contributor

Choose a reason for hiding this comment

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

So I talked with @colin-rogers-dbt + @mikealfare about this. I think we can get safely rid of this check entirely. Basically, we've decoupled the plugin (adapter specific) version and the core version. The majors no longer need to be equal, and the minor can be behind the core minor. The later might not actually be that uncommon.

All of this is the case because PyPI now handles this via dbt-adapters. Essentially, dbt-core specifies what dbt-adapters versions it is compatible with, and each dbt adapter also specifies what dbt-adapters versions it is compatible with. The result being, you'd have to intentionally force install incompatible versions for this check to be relevant. Thus, this check is unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even better! you want to do the honors or should i?

compatibility_msg = red("Not compatible!")
needs_update = True
return (compatibility_msg, needs_update)
Expand Down
93 changes: 93 additions & 0 deletions tests/unit/test_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,99 @@ def test_plugin_diff_core_minor_ahead_latest(self, mocker):

assert expected == actual

def test_plugin_diff_plugin_minor_ahead_latest(self, mocker):
"""
Now that adapters are decoupled from core, a higher minor version of a plugin
is compatible with a lower minor version of core.
"""

mock_versions(
mocker,
installed="1.8.0",
latest="1.8.0",
plugins={
"foobar": ("1.9.0", "1.9.0"),
},
)

actual = dbt.version.get_version_information()
expected = "\n".join(
[
"Core:",
" - installed: 1.8.0",
f" - latest: 1.8.0 - {green('Up to date!')}",
"",
"Plugins:",
f" - foobar: 1.9.0 - {green('Up to date!')}",
"",
"",
]
)

assert expected == actual

def test_plugin_diff_plugin_patch_ahead_latest(self, mocker):
"""
Now that adapters are decoupled from core, a higher minor version of a plugin
is compatible with a lower minor version of core.
"""

mock_versions(
mocker,
installed="1.8.0",
latest="1.8.0",
plugins={
"foobar": ("1.8.4", "1.8.4"),
},
)

actual = dbt.version.get_version_information()
expected = "\n".join(
[
"Core:",
" - installed: 1.8.0",
f" - latest: 1.8.0 - {green('Up to date!')}",
"",
"Plugins:",
f" - foobar: 1.8.4 - {green('Up to date!')}",
"",
"",
]
)

assert expected == actual

def test_plugin_diff_plugin_minor_ahead_no_latest(self, mocker):
"""
Now that adapters are decoupled from core, a higher minor version of a plugin
is compatible with a lower minor version of core.
"""

mock_versions(
mocker,
installed="1.8.0",
latest="1.8.0",
plugins={
"foobar": ("1.9.0", None),
},
)

actual = dbt.version.get_version_information()
expected = "\n".join(
[
"Core:",
" - installed: 1.8.0",
f" - latest: 1.8.0 - {green('Up to date!')}",
"",
"Plugins:",
f" - foobar: 1.9.0 - {yellow('Could not determine latest version')}",
"",
"",
]
)

assert expected == actual

def test_plugin_diff_core_minor_behind_latest(self, mocker):
mock_versions(
mocker,
Expand Down
Loading