-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Check dbt-core version requirements when installing Hub packages #5651
Changes from 5 commits
a5e5448
181e75d
ab42c78
8818bb9
1e9f5d3
243ce97
a94b41c
3b5d53d
5de7117
1c0e94a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kind: Features | ||
body: Check dbt-core version requirements when installing Hub packages | ||
time: 2022-08-15T13:43:12.965143+01:00 | ||
custom: | ||
Author: jtcohen6 | ||
Issue: "5648" | ||
PR: "5651" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -778,9 +778,10 @@ def package_not_found(package_name): | |
|
||
def package_version_not_found(package_name, version_range, available_versions): | ||
base_msg = ( | ||
"Could not find a matching version for package {}\n" | ||
"Could not find a matching compatible version for package {}\n" | ||
" Requested range: {}\n" | ||
" Available versions: {}" | ||
" Compatible versions: {}\n" | ||
" (Not shown: versions incompatible with installed version of dbt-core)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very nice 🤩 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we recommend using the no-version-check flag here, like we do when a dbt project fails to run due to mismatching versions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea! |
||
) | ||
raise_dependency_error(base_msg.format(package_name, version_range, available_versions)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
from dbt.deps.git import GitUnpinnedPackage | ||
from dbt.deps.local import LocalUnpinnedPackage | ||
from dbt.deps.registry import RegistryUnpinnedPackage | ||
from dbt.clients.registry import is_compatible_version | ||
from dbt.deps.resolver import resolve_packages | ||
from dbt.contracts.project import ( | ||
LocalPackage, | ||
|
@@ -15,6 +16,7 @@ | |
|
||
from dbt.contracts.project import PackageConfig | ||
from dbt.semver import VersionSpecifier | ||
from dbt.version import get_installed_version | ||
|
||
from dbt.dataclass_schema import ValidationError | ||
|
||
|
@@ -114,13 +116,13 @@ def setUp(self): | |
self.patcher = mock.patch('dbt.deps.registry.registry') | ||
self.registry = self.patcher.start() | ||
self.index_cached = self.registry.index_cached | ||
self.get_available_versions = self.registry.get_available_versions | ||
self.get_compatible_versions = self.registry.get_compatible_versions | ||
self.package_version = self.registry.package_version | ||
|
||
self.index_cached.return_value = [ | ||
'dbt-labs-test/a', | ||
] | ||
self.get_available_versions.return_value = [ | ||
self.get_compatible_versions.return_value = [ | ||
'0.1.2', '0.1.3', '0.1.4a1' | ||
] | ||
self.package_version.return_value = { | ||
|
@@ -235,9 +237,10 @@ def test_resolve_missing_version(self): | |
with self.assertRaises(dbt.exceptions.DependencyException) as exc: | ||
a.resolved() | ||
msg = ( | ||
"Could not find a matching version for package " | ||
"Could not find a matching compatible version for package " | ||
"dbt-labs-test/a\n Requested range: =0.1.4, =0.1.4\n " | ||
"Available versions: ['0.1.2', '0.1.3']" | ||
"Compatible versions: ['0.1.2', '0.1.3']\n " | ||
"(Not shown: versions incompatible with installed version of dbt-core)" | ||
) | ||
self.assertEqual(msg, str(exc.exception)) | ||
|
||
|
@@ -509,12 +512,19 @@ def __init__(self, packages): | |
def index_cached(self, registry_base_url=None): | ||
return sorted(self.packages) | ||
|
||
def get_available_versions(self, name): | ||
def package(self, package_name, registry_base_url=None): | ||
try: | ||
pkg = self.packages[name] | ||
pkg = self.packages[package_name] | ||
except KeyError: | ||
return [] | ||
return list(pkg) | ||
return pkg | ||
Comment on lines
+516
to
+521
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice add, especially |
||
|
||
def get_compatible_versions(self, package_name, dbt_version, should_version_check): | ||
packages = self.package(package_name) | ||
return [ | ||
pkg_version for pkg_version, info in packages.items() | ||
if is_compatible_version(info, dbt_version) | ||
] | ||
|
||
def package_version(self, name, version): | ||
try: | ||
|
@@ -524,7 +534,13 @@ def package_version(self, name, version): | |
|
||
|
||
class TestPackageSpec(unittest.TestCase): | ||
def setUp(self): | ||
def setUp(self): | ||
dbt_version = get_installed_version() | ||
next_version = dbt_version | ||
next_version.minor = str(int(next_version.minor) + 1) | ||
next_version.prerelease = None | ||
require_next_version = ">" + next_version.to_version_string() | ||
|
||
self.patcher = mock.patch('dbt.deps.registry.registry') | ||
self.registry = self.patcher.start() | ||
self.mock_registry = MockRegistry(packages={ | ||
|
@@ -556,6 +572,22 @@ def setUp(self): | |
'extra': 'field', | ||
}, | ||
'newfield': ['another', 'value'], | ||
}, | ||
'0.2.0': { | ||
'id': 'dbt-labs-test/a/0.2.0', | ||
'name': 'a', | ||
'version': '0.2.0', | ||
'packages': [], | ||
'_source': { | ||
'blahblah': 'asdfas', | ||
}, | ||
# this one shouldn't be picked! | ||
'require_dbt_version': require_next_version, | ||
'downloads': { | ||
'tarball': 'https://example.com/invalid-url!', | ||
'extra': 'field', | ||
}, | ||
'newfield': ['another', 'value'], | ||
} | ||
}, | ||
'dbt-labs-test/b': { | ||
|
@@ -577,7 +609,7 @@ def setUp(self): | |
}) | ||
|
||
self.registry.index_cached.side_effect = self.mock_registry.index_cached | ||
self.registry.get_available_versions.side_effect = self.mock_registry.get_available_versions | ||
self.registry.get_compatible_versions.side_effect = self.mock_registry.get_compatible_versions | ||
self.registry.package_version.side_effect = self.mock_registry.package_version | ||
|
||
def tearDown(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for
installable
to be empty?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! We should be sure to raise a good exception before we try to access an element. I'll rearrange the logic accordingly