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

Use yaml renderer (with target context) for rendering selectors #5136

Merged
merged 7 commits into from
Apr 26, 2022
Merged
Show file tree
Hide file tree
Changes from 6 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
7 changes: 7 additions & 0 deletions .changes/unreleased/Fixes-20220422-135645.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Fixes
body: Use yaml renderer (with target context) for rendering selectors
time: 2022-04-22T13:56:45.147893-04:00
custom:
Author: gshank
Issue: "5131"
PR: "5136"
17 changes: 3 additions & 14 deletions core/dbt/config/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,10 @@ def __init__(
def name(self):
"Project config"

# Uses SecretRenderer
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

def get_package_renderer(self) -> BaseRenderer:
return PackageRenderer(self.ctx_obj.cli_vars)

def get_selector_renderer(self) -> BaseRenderer:
return SelectorRenderer(self.ctx_obj.cli_vars)

def render_project(
self,
project: Dict[str, Any],
Expand All @@ -136,8 +134,7 @@ def render_packages(self, packages: Dict[str, Any]):
return package_renderer.render_data(packages)

def render_selectors(self, selectors: Dict[str, Any]):
selector_renderer = self.get_selector_renderer()
return selector_renderer.render_data(selectors)
return self.render_data(selectors)

def render_entry(self, value: Any, keypath: Keypath) -> Any:
result = super().render_entry(value, keypath)
Expand Down Expand Up @@ -165,18 +162,10 @@ def should_render_keypath(self, keypath: Keypath) -> bool:
return True


class SelectorRenderer(BaseRenderer):
@property
def name(self):
return "Selector config"


class SecretRenderer(BaseRenderer):
def __init__(self, cli_vars: Optional[Dict[str, Any]] = None) -> None:
def __init__(self, cli_vars: Dict[str, Any] = {}) -> None:
# Generate contexts here because we want to save the context
# object in order to retrieve the env_vars.
if cli_vars is None:
cli_vars = {}
self.ctx_obj = SecretContext(cli_vars)
context = self.ctx_obj.to_dict()
super().__init__(context)
Expand Down
6 changes: 3 additions & 3 deletions core/dbt/config/selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from dbt.clients.yaml_helper import yaml, Loader, Dumper, load_yaml_text # noqa: F401
from dbt.dataclass_schema import ValidationError

from .renderer import SelectorRenderer
from .renderer import BaseRenderer
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to just use BaseRenderer, since the context available to selectors.yml is the same as every other yaml files


from dbt.clients.system import (
load_file_contents,
Expand Down Expand Up @@ -57,7 +57,7 @@ def selectors_from_dict(cls, data: Dict[str, Any]) -> "SelectorConfig":
def render_from_dict(
cls,
data: Dict[str, Any],
renderer: SelectorRenderer,
renderer: BaseRenderer,
) -> "SelectorConfig":
try:
rendered = renderer.render_data(data)
Expand All @@ -72,7 +72,7 @@ def render_from_dict(
def from_path(
cls,
path: Path,
renderer: SelectorRenderer,
renderer: BaseRenderer,
) -> "SelectorConfig":
try:
data = load_yaml_text(load_file_contents(str(path)))
Expand Down
2 changes: 2 additions & 0 deletions core/dbt/config/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ def get_project_config(
user_config = read_user_config(flags.PROFILES_DIR)
# Update flags
flags.set_from_args(args, user_config)
if cli_vars is None:
cli_vars = {}
Comment on lines +65 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just moving the code from SecretRenderer here?

This is the only part that scares me, insofar as it touches code that's more widely used than just selectors.yml. It's hard to imagine how this could cause issues, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to "make mypy happy", because in some places we have an Optional[Dict[str, Any]] and in other places it's not Optional. So without this mypy will complain. We do a similar thing in a number of other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Thanks for the explanation

profile = Profile.render_from_args(args, ProfileRenderer(cli_vars), profile_name)
# Generate a project
project = Project.from_project_root(
Expand Down
3 changes: 3 additions & 0 deletions core/dbt/tests/fixtures/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ def test_data_dir(request):

# This contains the profile target information, for simplicity in setting
# up different profiles, particularly in the adapter repos.
# Note: because we load the profile to create the adapter, this
# fixture can't be used to test vars and env_vars or errors. The
# profile must be written out after the test starts.
@pytest.fixture(scope="class")
def dbt_profile_target():
return {
Expand Down
8 changes: 8 additions & 0 deletions core/dbt/tests/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
# read_file
# get_artifact
# update_config_file
# write_config_file
# get_unique_ids_in_results
# check_result_nodes_by_name
# check_result_nodes_by_unique_id
Expand Down Expand Up @@ -149,6 +150,13 @@ def update_config_file(updates, *paths):
write_file(new_yaml, *paths)


# Write new config file
def write_config_file(data, *paths):
if type(data) is dict:
data = yaml.safe_dump(data)
write_file(data, *paths)


# Get the unique_ids in dbt command results
def get_unique_ids_in_results(results):
unique_ids = []
Expand Down

This file was deleted.

19 changes: 0 additions & 19 deletions test/integration/028_cli_vars_tests/models_complex/schema.yml

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

57 changes: 0 additions & 57 deletions test/integration/028_cli_vars_tests/test_cli_var_override.py

This file was deleted.

68 changes: 0 additions & 68 deletions test/integration/028_cli_vars_tests/test_cli_vars.py

This file was deleted.

68 changes: 68 additions & 0 deletions tests/functional/cli_vars/test_cli_var_override.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import pytest

from dbt.tests.util import run_dbt


models_override__schema_yml = """
version: 2
models:
- name: test_vars
columns:
- name: field
tests:
- accepted_values:
values:
- override
"""

models_override__test_vars_sql = """
select '{{ var("required") }}'::varchar as field
"""


# Tests that cli vars override vars set in the project config
class TestCLIVarOverride:
@pytest.fixture(scope="class")
def models(self):
return {
"schema.yml": models_override__schema_yml,
"test_vars.sql": models_override__test_vars_sql,
}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"vars": {
"required": "present",
},
}

def test__override_vars_global(self, project):
run_dbt(["run", "--vars", "{required: override}"])
run_dbt(["test"])


# This one switches to setting a var in 'test'
class TestCLIVarOverridePorject:
@pytest.fixture(scope="class")
def models(self):
return {
"schema.yml": models_override__schema_yml,
"test_vars.sql": models_override__test_vars_sql,
}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"vars": {
"test": {
"required": "present",
},
},
}

def test__override_vars_project_level(self, project):

# This should be "override"
run_dbt(["run", "--vars", "{required: override}"])
run_dbt(["test"])
Loading