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

Warn on unpinned git packages (#1446) #1453

Merged
merged 2 commits into from
May 10, 2019
Merged

Warn on unpinned git packages (#1446) #1453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70206b1
warn on unpinned dependencies, unless warn-unpinned: False is set in …
May 9, 2019
ea88259
PR feedback
May 10, 2019
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
3 changes: 3 additions & 0 deletions core/dbt/contracts/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ class Project(APIObject):
'items': {'type': 'string'},
'description': 'The git revision to use, if it is not tip',
},
'warn-unpinned': {
'type': 'boolean',
}
},
'required': ['git'],
}
Expand Down
20 changes: 19 additions & 1 deletion core/dbt/task/deps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

DOWNLOADS_PATH = None
REMOVE_DOWNLOADS = False
PIN_PACKAGE_URL = 'https://docs.getdbt.com/docs/package-management#section-specifying-package-versions' # noqa


def _initialize_downloads():
Expand Down Expand Up @@ -215,6 +216,8 @@ class GitPackage(Package):
SCHEMA = GIT_PACKAGE_CONTRACT

def __init__(self, *args, **kwargs):
if 'warn_unpinned' in kwargs:
kwargs['warn-unpinned'] = kwargs.pop('warn_unpinned')
super(GitPackage, self).__init__(*args, **kwargs)
self._checkout_name = hashlib.md5(six.b(self.git)).hexdigest()
self.version = self._contents.get('revision')
Expand Down Expand Up @@ -252,8 +255,12 @@ def nice_version_name(self):
return "revision {}".format(self.version_name())

def incorporate(self, other):
# if one is True, make both be True.
warn_unpinned = self.warn_unpinned and other.warn_unpinned
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intended logic here? This comment sounds to me like self.warn_unpinned or other.warn_unpinned, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! My thought was that it should be False if either was False since it means that at least someone knew what they were doing and didn't want to be bothered. I'm not sure why I wrote True twice there in that comment.


return GitPackage(git=self.git,
revision=(self.version + other.version))
revision=(self.version + other.version),
warn_unpinned=warn_unpinned)

def _resolve_version(self):
requested = set(self.version)
Expand All @@ -263,6 +270,10 @@ def _resolve_version(self):
'{} contains: {}'.format(self.git, requested))
self.version = requested.pop()

@property
def warn_unpinned(self):
return self.get('warn-unpinned', True)

def _checkout(self, project):
"""Performs a shallow clone of the repository into the downloads
directory. This function can be called repeatedly. If the project has
Expand All @@ -287,6 +298,13 @@ def _checkout(self, project):

def _fetch_metadata(self, project):
path = self._checkout(project)
if self.version[0] == 'master' and self.warn_unpinned:
dbt.exceptions.warn_or_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is in the class of warnings that's worth breaking out log_fmt=printer.yellow('{}') for :)

Copy link
Contributor

Choose a reason for hiding this comment

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

log_fmt=printer.red_bold_blinking('{}')

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can we format this as:

WARNING: The git package "https://github.com/fishtown-analytics/dbt-utils" is not pinned.
         This can introduce breaking changes into your project without warning!

See https://docs.getdbt.com/docs/package-management#section-specifying-package-versions

We show this warning if a revision is unspecified too, so calling out master here might be confusing. I think we're best off pointing people to the docs, then doing a really good job of explaining what's wrong and how to proceed in there.

As a part of this, I'll clean up the whole /package-management docs page for sure

'Version for {} not pinned - "master" may introduce breaking '
'changes without warning! See {}'
.format(self.git, PIN_PACKAGE_URL),
log_fmt='WARNING: {}'
)
loaded = project.from_project_root(path, {})
return ProjectPackageMetadata(loaded)

Expand Down
50 changes: 43 additions & 7 deletions test/integration/006_simple_dependency_test/test_simple_dependency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
from test.integration.base import DBTIntegrationTest, use_profile
from dbt.exceptions import CompilationException


class TestSimpleDependency(DBTIntegrationTest):
Expand All @@ -21,13 +22,14 @@ def packages_config(self):
return {
"packages": [
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project'
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'warn-unpinned': False,
}
]
}

@use_profile('postgres')
def test_simple_dependency(self):
def test_postgres_simple_dependency(self):
self.run_dbt(["deps"])
results = self.run_dbt(["run"])
self.assertEqual(len(results), 4)
Expand All @@ -49,7 +51,7 @@ def test_simple_dependency(self):
self.assertTablesEqual("seed","incremental")

@use_profile('postgres')
def test_simple_dependency_with_models(self):
def test_postgres_simple_dependency_with_models(self):
self.run_dbt(["deps"])
results = self.run_dbt(["run", '--models', 'view_model+'])
self.assertEqual(len(results), 2)
Expand All @@ -66,6 +68,37 @@ def test_simple_dependency_with_models(self):
self.assertEqual(created_models['view_summary'], 'view')


class TestSimpleDependencyUnpinned(DBTIntegrationTest):
def setUp(self):
DBTIntegrationTest.setUp(self)
self.run_sql_file("test/integration/006_simple_dependency_test/seed.sql")

@property
def schema(self):
return "simple_dependency_006"

@property
def models(self):
return "test/integration/006_simple_dependency_test/models"

@property
def packages_config(self):
return {
"packages": [
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'warn-unpinned': True,
}
]
}

@use_profile('postgres')
def test_postgres_simple_dependency(self):
self.run_dbt(['deps'], strict=False)
with self.assertRaises(CompilationException):
self.run_dbt(["deps"])


class TestSimpleDependencyWithDuplicates(DBTIntegrationTest):
@property
def schema(self):
Expand All @@ -81,10 +114,12 @@ def packages_config(self):
return {
"packages": [
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project'
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'warn-unpinned': False,
},
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project.git'
'git': 'https://github.com/fishtown-analytics/dbt-integration-project.git',
'warn-unpinned': False,
}
]
}
Expand Down Expand Up @@ -147,6 +182,7 @@ def packages_config(self):
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'revision': 'master',
'warn-unpinned': False,
},
]
}
Expand All @@ -168,7 +204,7 @@ def deps_run_assert_equality(self):
self.assertEqual(created_models['incremental'], 'table')

@use_profile('postgres')
def test_simple_dependency(self):
def test_postgres_simple_dependency(self):
self.deps_run_assert_equality()

self.assertTablesEqual("seed_summary","view_summary")
Expand All @@ -178,7 +214,7 @@ def test_simple_dependency(self):
self.deps_run_assert_equality()

@use_profile('postgres')
def test_empty_models_not_compiled_in_dependencies(self):
def test_postgres_empty_models_not_compiled_in_dependencies(self):
self.deps_run_assert_equality()

models = self.get_models_in_schema()
Expand Down
2 changes: 1 addition & 1 deletion test/integration/007_graph_selection_tests/test_schema_test_graph_selection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def models(self):
def packages_config(self):
return {
"packages": [
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project'}
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project', 'warn-unpinned': False}
]
}

Expand Down
1 change: 1 addition & 0 deletions test/integration/008_schema_tests_test/test_schema_v2_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def packages_config(self):
},
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'warn-unpinned': False,
},
]
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/016_macro_tests/test_macros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def models(self):
def packages_config(self):
return {
'packages': [
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project'},
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project', 'warn-unpinned': False}
]
}

Expand Down Expand Up @@ -92,7 +92,7 @@ def models(self):
def packages_config(self):
return {
'packages': [
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project'}
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project', 'warn-unpinned': False}
]
}

Expand Down
2 changes: 1 addition & 1 deletion test/integration/023_exit_codes_test/test_exit_codes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def models(self):
def packages_config(self):
return {
"packages": [
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project'}
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project', 'warn-unpinned': False}
]
}

Expand Down
2 changes: 2 additions & 0 deletions test/integration/025_duplicate_model_test/test_duplicate_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def packages_config(self):
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'revision': 'master',
'warn-unpinned': False,
},
],
}
Expand Down Expand Up @@ -139,6 +140,7 @@ def packages_config(self):
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'revision': 'master',
'warn-unpinned': False,
},
],
}
Expand Down
1 change: 1 addition & 0 deletions test/integration/029_docs_generate_tests/test_docs_generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def packages_config(self):
'packages': [
{
'git': 'https://github.com/fishtown-analytics/dbt-integration-project',
'warn-unpinned': False,
},
],
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/033_event_tracking_test/test_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class TestEventTrackingSuccess(TestEventTracking):
def packages_config(self):
return {
'packages': [
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project'},
{'git': 'https://github.com/fishtown-analytics/dbt-integration-project', 'warn-unpinned': False},
],
}

Expand Down