-
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
Warn on unpinned git packages (#1446) #1453
Warn on unpinned git packages (#1446) #1453
Conversation
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.
One question and some copy changes, but otherwise this is ready to merge. There are countless people out there who won't have headaches because of this PR, and that is a really good thing!
@@ -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 |
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.
What's the intended logic here? This comment sounds to me like self.warn_unpinned or other.warn_unpinned
, right?
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.
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.
@@ -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( |
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.
this is in the class of warnings that's worth breaking out log_fmt=printer.yellow('{}')
for :)
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.
log_fmt=printer.red_bold_blinking('{}')
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.
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
Fixes #1446
This fails the run in strict mode and logs a warning otherwise.
With no repository set and no value for
warn-unpinned
:With a version set:
With warn-unpinned set to False: