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.

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

Do not jinja render packages from dependencies yml #7910

Merged
merged 6 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/Under the Hood-20230620-180852.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Under the Hood
body: Don't jinja render packages from dependencies.yml
time: 2023-06-20T18:08:52.848093-04:00
custom:
Author: gshank
Issue: "7905"
13 changes: 10 additions & 3 deletions core/dbt/config/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,9 @@ def package_and_project_data_from_root(project_root):
packages_yml_dict = {}
dependencies_yml_dict = {}
if path_exists(package_filepath):
packages_yml_dict = _load_yaml(package_filepath)
packages_yml_dict = _load_yaml(package_filepath) or {}
if path_exists(dependencies_filepath):
dependencies_yml_dict = _load_yaml(dependencies_filepath)
dependencies_yml_dict = _load_yaml(dependencies_filepath) or {}

if "packages" in packages_yml_dict and "packages" in dependencies_yml_dict:
msg = "The 'packages' key cannot be specified in both packages.yml and dependencies.yml"
Expand All @@ -116,6 +116,7 @@ def package_and_project_data_from_root(project_root):
dependent_projects_dict = {}
if "packages" in dependencies_yml_dict:
packages_dict["packages"] = dependencies_yml_dict["packages"]
packages_dict["packages_from_dependencies"] = True
Copy link
Contributor

@jtcohen6 jtcohen6 Jun 21, 2023

Choose a reason for hiding this comment

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

Not related to the change in this PR, as much as the changes previously made in #7857:

This logic raises an error if any packages.yml is an empty file, whether defined in the project root or in an installed package. (I noticed this because the dbt-date package has just such a file.) An empty packages.yml is not officially documented behavior, but it didn't used to raise an error, and now it does.

packages:
  - package: calogica/dbt_date
    version: 0.7.2
$ dbt deps && dbt parse
...
  File "/Users/jerco/dev/product/dbt-core/core/dbt/config/project.py", line 108, in package_and_project_data_from_root
    if "packages" in packages_yml_dict and "packages" in dependencies_yml_dict:
TypeError: argument of type 'NoneType' is not iterable

Quick fix on lines 103-106 above:

    if path_exists(package_filepath):
        packages_yml_dict = _load_yaml(package_filepath) or {}
    if path_exists(dependencies_filepath):
        dependencies_yml_dict = _load_yaml(dependencies_filepath) or {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I've added a test and made this change.

else: # don't check for "packages" here so we capture invalid keys in packages.yml
packages_dict = packages_yml_dict
if "projects" in dependencies_yml_dict:
Expand Down Expand Up @@ -283,6 +284,7 @@ class RenderComponents:

@dataclass
class PartialProject(RenderComponents):
# This class includes the project_dict, packages_dict, selectors_dict, etc from RenderComponents
profile_name: Optional[str] = field(
metadata=dict(description="The unrendered profile name in the project, if set")
)
Expand Down Expand Up @@ -324,7 +326,7 @@ def get_rendered(
selectors_dict=rendered_selectors,
)

# Called by 'collect_parts' in RuntimeConfig
# Called by Project.from_project_root (not PartialProject.from_project_root!)
def render(self, renderer: DbtProjectYamlRenderer) -> "Project":
try:
rendered = self.get_rendered(renderer)
Expand Down Expand Up @@ -538,6 +540,7 @@ def from_dicts(
project_name = project_dict.get("name")
profile_name = project_dict.get("profile")

# Create a PartialProject
return cls(
profile_name=profile_name,
project_name=project_name,
Expand All @@ -555,6 +558,7 @@ def from_project_root(
) -> "PartialProject":
project_root = os.path.normpath(project_root)
project_dict = load_raw_project(project_root)
# Read packages.yml and dependencies.yml and pass dictionaries to "from_dicts" method
packages_dict, dependent_projects_dict = package_and_project_data_from_root(project_root)
selectors_dict = selector_data_from_root(project_root)
return cls.from_dicts(
Expand Down Expand Up @@ -715,6 +719,9 @@ def partial_load(cls, project_root: str, *, verify_version: bool = False) -> Par
verify_version=verify_version,
)

# Called by:
# RtConfig.load_dependencies => RtConfig.load_projects => RtConfig.new_project => Project.from_project_root
# RtConfig.from_args => RtConfig.collect_parts => load_project => Project.from_project_root
@classmethod
def from_project_root(
cls,
Expand Down
8 changes: 7 additions & 1 deletion core/dbt/config/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,14 @@ def render_project(

def render_packages(self, packages: Dict[str, Any]):
"""Render the given packages dict"""
packages = packages or {} # Sometimes this is none in tests
package_renderer = self.get_package_renderer()
return package_renderer.render_data(packages)
if "packages_from_dependencies" in packages:
# We don't want to render the "packages" dictionary that came from dependencies.yml
packages.pop("packages_from_dependencies")
return packages
else:
return package_renderer.render_data(packages)

def render_dependent_projects(self, dependent_projects: Dict[str, Any]):
"""This is a no-op to maintain regularity in the interfaces. We don't render dependencies.yml."""
Expand Down
4 changes: 3 additions & 1 deletion core/dbt/config/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
from .renderer import DbtProjectYamlRenderer, ProfileRenderer


# Called by RuntimeConfig.collect_parts class method
def load_project(
project_root: str,
version_check: bool,
Expand Down Expand Up @@ -237,6 +238,7 @@ def validate(self):
except ValidationError as e:
raise ConfigContractBrokenError(e) from e

# Called by RuntimeConfig.from_args
@classmethod
def collect_parts(cls: Type["RuntimeConfig"], args: Any) -> Tuple[Project, Profile]:
# profile_name from the project
Expand All @@ -251,7 +253,7 @@ def collect_parts(cls: Type["RuntimeConfig"], args: Any) -> Tuple[Project, Profi
project = load_project(project_root, bool(flags.VERSION_CHECK), profile, cli_vars)
return project, profile

# Called in main.py, lib.py, task/base.py
# Called in task/base.py, in BaseTask.from_args
@classmethod
def from_args(cls, args: Any) -> "RuntimeConfig":
"""Given arguments, read in dbt_project.yml from the current directory,
Expand Down
10 changes: 10 additions & 0 deletions tests/functional/dependencies/test_simple_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ def test_dependency_with_dependencies_file(self, run_deps, project):
assert len(results) == 4


class TestSimpleDependencyWithEmptyPackagesFile(SimpleDependencyBase):
@pytest.fixture(scope="class")
def packages(self):
return " "

def test_dependency_with_empty_packages_file(self, run_deps, project):
# Tests that an empty packages file doesn't fail with a Python error
run_dbt(["deps"])


class TestSimpleDependencyNoProfile(SimpleDependencyBase):
"""dbt deps and clean commands should not require a profile."""

Expand Down
16 changes: 16 additions & 0 deletions tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,22 @@ def test_custom_query_comment_append(self):
self.assertEqual(project.query_comment.comment, "run by user test")
self.assertEqual(project.query_comment.append, True)

def test_packages_from_dependencies(self):
packages = {
"packages_from_dependencies": True,
"packages": [
{
"git": "{{ env_var('some_package') }}",
"warn-unpinned": True,
}
],
}

project = project_from_config_rendered(self.default_project_data, packages)
git_package = project.packages.packages[0]
# packages did not render because of "packages_from_dependencies" key
assert git_package.git == "{{ env_var('some_package') }}"


class TestProjectFile(BaseFileTest):
def setUp(self):
Expand Down