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

Add deprecation warnings for log-path, target-path in dbt_project.yml #7185

Merged
merged 4 commits into from
Mar 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
9 changes: 9 additions & 0 deletions .changes/unreleased/Breaking Changes-20230317-110033.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
kind: Breaking Changes
body: Specifying "log-path" and "target-path" in "dbt_project.yml" is deprecated.
This functionality will be removed in a future version of dbt-core. If you need
to specify a custom path for logs or artifacts, please set via CLI flag or env var
instead.
time: 2023-03-17T11:00:33.448472+01:00
custom:
Author: jtcohen6
Issue: "6882"
2 changes: 2 additions & 0 deletions core/dbt/cli/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ def assign_params(ctx, params_assigned_from_default, deprecated_env_vars):
self._override_if_set("LOG_FORMAT", "LOG_FORMAT_FILE", params_assigned_from_default)

# Default LOG_PATH from PROJECT_DIR, if available.
# Starting in v1.5, if `log-path` is set in `dbt_project.yml`, it will raise a deprecation warning,
# with the possibility of removing it in a future release.
if getattr(self, "LOG_PATH", None) is None:
project_dir = getattr(self, "PROJECT_DIR", default_project_dir())
version_check = getattr(self, "VERSION_CHECK", True)
Expand Down
26 changes: 16 additions & 10 deletions core/dbt/config/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,23 +298,27 @@ def render_package_metadata(self, renderer: PackageRenderer) -> ProjectPackageMe
raise DbtProjectError("Package dbt_project.yml must have a name!")
return ProjectPackageMetadata(self.project_name, packages_config.packages)

def check_config_path(self, project_dict, deprecated_path, exp_path):
def check_config_path(
self, project_dict, deprecated_path, expected_path=None, default_value=None
):
if deprecated_path in project_dict:
if exp_path in project_dict:
if expected_path in project_dict:
msg = (
"{deprecated_path} and {exp_path} cannot both be defined. The "
"`{deprecated_path}` config has been deprecated in favor of `{exp_path}`. "
"{deprecated_path} and {expected_path} cannot both be defined. The "
"`{deprecated_path}` config has been deprecated in favor of `{expected_path}`. "
"Please update your `dbt_project.yml` configuration to reflect this "
"change."
)
raise DbtProjectError(
msg.format(deprecated_path=deprecated_path, exp_path=exp_path)
msg.format(deprecated_path=deprecated_path, expected_path=expected_path)
)
# this field is no longer supported, but many projects may specify it with the default value
# if so, let's only raise this deprecation warning if they set a custom value
Comment on lines +315 to +316
Copy link
Contributor Author

@jtcohen6 jtcohen6 Mar 17, 2023

Choose a reason for hiding this comment

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

Because so many users have the configs set, with just the default values, only raise the deprecation warning if a custom (non-default) value is set in dbt_project.yml

❗disagreement welcome❗

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this. Why rouse them to action when there's really no need. It's just a syntactic artifact. At most, you could tell people "hey this doesn't even do anything anymore tbh" but also idk if it really impacts anyone.

if not default_value or project_dict[deprecated_path] != default_value:
deprecations.warn(
f"project-config-{deprecated_path}",
deprecated_path=deprecated_path,
)
deprecations.warn(
f"project-config-{deprecated_path}",
deprecated_path=deprecated_path,
exp_path=exp_path,
)

def create_project(self, rendered: RenderComponents) -> "Project":
unrendered = RenderComponents(
Expand All @@ -329,6 +333,8 @@ def create_project(self, rendered: RenderComponents) -> "Project":

self.check_config_path(rendered.project_dict, "source-paths", "model-paths")
self.check_config_path(rendered.project_dict, "data-paths", "seed-paths")
self.check_config_path(rendered.project_dict, "log-path", default_value="logs")
self.check_config_path(rendered.project_dict, "target-path", default_value="target")

try:
ProjectContract.validate(rendered.project_dict)
Expand Down
12 changes: 12 additions & 0 deletions core/dbt/deprecations.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ class ExposureNameDeprecation(DBTDeprecation):
_event = "ExposureNameDeprecation"


class ConfigLogPathDeprecation(DBTDeprecation):
_name = "project-config-log-path"
_event = "ConfigLogPathDeprecation"


class ConfigTargetPathDeprecation(DBTDeprecation):
_name = "project-config-target-path"
_event = "ConfigTargetPathDeprecation"


def renamed_env_var(old_name: str, new_name: str):
class EnvironmentVariableRenamed(DBTDeprecation):
_name = f"environment-variable-renamed:{old_name}"
Expand Down Expand Up @@ -116,6 +126,8 @@ def warn(name, *args, **kwargs):
ConfigDataPathDeprecation(),
MetricAttributesRenamed(),
ExposureNameDeprecation(),
ConfigLogPathDeprecation(),
ConfigTargetPathDeprecation(),
]

deprecations: Dict[str, DBTDeprecation] = {d.name: d for d in deprecations_list}
Expand Down
6 changes: 5 additions & 1 deletion core/dbt/events/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,8 @@ logger = AdapterLogger("<database name>")

## Compiling types.proto

After adding a new message in types.proto, in the core/dbt/events directory: ```protoc --python_betterproto_out . types.proto```
After adding a new message in types.proto:
```
cd core/dbt/events
protoc --python_betterproto_out . types.proto
```
26 changes: 26 additions & 0 deletions core/dbt/events/proto_types.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 25 additions & 5 deletions core/dbt/events/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ message ConfigDataPathDeprecationMsg {
ConfigDataPathDeprecation data = 2;
}

//D005
// D005
message AdapterDeprecationWarning {
string old_name = 1;
string new_name = 2;
Expand All @@ -316,7 +316,7 @@ message AdapterDeprecationWarningMsg {
AdapterDeprecationWarning data = 2;
}

//D006
// D006
message MetricAttributesRenamed {
string metric_name = 1;
}
Expand All @@ -326,7 +326,7 @@ message MetricAttributesRenamedMsg {
MetricAttributesRenamed data = 2;
}

//D007
// D007
message ExposureNameDeprecation {
string exposure = 1;
}
Expand All @@ -336,7 +336,7 @@ message ExposureNameDeprecationMsg {
ExposureNameDeprecation data = 2;
}

//D008
// D008
message InternalDeprecation {
string name = 1;
string reason = 2;
Expand All @@ -349,7 +349,7 @@ message InternalDeprecationMsg {
InternalDeprecation data = 2;
}

//D009
// D009
message EnvironmentVariableRenamed {
string old_name = 1;
string new_name = 2;
Expand All @@ -360,6 +360,26 @@ message EnvironmentVariableRenamedMsg {
EnvironmentVariableRenamed data = 2;
}

// D010
message ConfigLogPathDeprecation {
string deprecated_path = 1;
}

message ConfigLogPathDeprecationMsg {
EventInfo info = 1;
ConfigLogPathDeprecation data = 2;
}

// D011
message ConfigTargetPathDeprecation {
string deprecated_path = 1;
}

message ConfigTargetPathDeprecationMsg {
EventInfo info = 1;
ConfigTargetPathDeprecation data = 2;
}

// E - DB Adapter

// E001
Expand Down
36 changes: 36 additions & 0 deletions core/dbt/events/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,42 @@ def message(self):
return line_wrap_message(warning_tag(f"Deprecated functionality\n\n{description}"))


@dataclass
class ConfigLogPathDeprecation(WarnLevel, pt.ConfigSourcePathDeprecation): # noqa
def code(self):
return "D010"

def message(self):
output = "logs"
cli_flag = "--log-path"
env_var = "DBT_LOG_PATH"
description = (
f"The `{self.deprecated_path}` config in `dbt_project.yml` has been deprecated, "
f"and will no longer be supported in a future version of dbt-core. "
f"If you wish to write dbt {output} to a custom directory, please use "
f"the {cli_flag} CLI flag or {env_var} env var instead."
)
return line_wrap_message(warning_tag(f"Deprecated functionality\n\n{description}"))


@dataclass
class ConfigTargetPathDeprecation(WarnLevel, pt.ConfigSourcePathDeprecation): # noqa
def code(self):
return "D011"

def message(self):
output = "artifacts"
cli_flag = "--target-path"
env_var = "DBT_TARGET_PATH"
description = (
f"The `{self.deprecated_path}` config in `dbt_project.yml` has been deprecated, "
f"and will no longer be supported in a future version of dbt-core. "
f"If you wish to write dbt {output} to a custom directory, please use "
f"the {cli_flag} CLI flag or {env_var} env var instead."
)
return line_wrap_message(warning_tag(f"Deprecated functionality\n\n{description}"))


# =======================================================
# E - DB Adapter
# =======================================================
Expand Down
1 change: 0 additions & 1 deletion core/dbt/include/starter_project/dbt_project.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ seed-paths: ["seeds"]
macro-paths: ["macros"]
snapshot-paths: ["snapshots"]

target-path: "target" # directory which will store compiled SQL files
clean-targets: # directories to be removed by `dbt clean`
- "target"
- "dbt_packages"
Expand Down
11 changes: 5 additions & 6 deletions core/dbt/tests/fixtures/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,10 @@ def project_config_update():
# Combines the project_config_update dictionary with project_config defaults to
# produce a project_yml config and write it out as dbt_project.yml
@pytest.fixture(scope="class")
def dbt_project_yml(project_root, project_config_update, logs_dir):
def dbt_project_yml(project_root, project_config_update):
project_config = {
"name": "test",
"profile": "test",
"log-path": logs_dir,
}
if project_config_update:
if isinstance(project_config_update, dict):
Expand Down Expand Up @@ -355,7 +354,10 @@ def project_files(project_root, models, macros, snapshots, properties, seeds, te
# We have a separate logs dir for every test
@pytest.fixture(scope="class")
def logs_dir(request, prefix):
return os.path.join(request.config.rootdir, "logs", prefix)
dbt_log_dir = os.path.join(request.config.rootdir, "logs", prefix)
os.environ["DBT_LOG_PATH"] = str(dbt_log_dir)
yield dbt_log_dir
del os.environ["DBT_LOG_PATH"]


# This fixture is for customizing tests that need overrides in adapter
Expand All @@ -379,7 +381,6 @@ def __init__(
test_data_dir,
test_schema,
database,
logs_dir,
test_config,
):
self.project_root = project_root
Expand All @@ -390,7 +391,6 @@ def __init__(
self.test_data_dir = test_data_dir
self.test_schema = test_schema
self.database = database
self.logs_dir = logs_dir
self.test_config = test_config
self.created_schemas = []

Expand Down Expand Up @@ -498,7 +498,6 @@ def project(
test_data_dir=test_data_dir,
test_schema=unique_schema,
database=adapter.config.credentials.database,
logs_dir=logs_dir,
test_config=test_config,
)
project.drop_test_schema()
Expand Down
13 changes: 11 additions & 2 deletions tests/functional/deprecations/test_deprecations.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,22 @@ def models(self):

@pytest.fixture(scope="class")
def project_config_update(self):
return {"config-version": 2, "data-paths": ["data"]}
return {
"config-version": 2,
"data-paths": ["data"],
"log-path": "customlogs",
"target-path": "customtarget",
}

def test_data_path(self, project):
deprecations.reset_deprecations()
assert deprecations.active_deprecations == set()
run_dbt(["debug"])
expected = {"project-config-data-paths"}
expected = {
"project-config-data-paths",
"project-config-log-path",
"project-config-target-path",
}
assert expected == deprecations.active_deprecations

def test_data_path_fail(self, project):
Expand Down
2 changes: 0 additions & 2 deletions tests/functional/init/test_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,6 @@ def test_init_task_outside_of_project(
macro-paths: ["macros"]
snapshot-paths: ["snapshots"]

target-path: "target" # directory which will store compiled SQL files
clean-targets: # directories to be removed by `dbt clean`
- "target"
- "dbt_packages"
Expand Down Expand Up @@ -667,7 +666,6 @@ def test_init_provided_project_name_and_skip_profile_setup(
macro-paths: ["macros"]
snapshot-paths: ["snapshots"]

target-path: "target" # directory which will store compiled SQL files
clean-targets: # directories to be removed by `dbt clean`
- "target"
- "dbt_packages"
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/partial_parsing/test_pp_vars.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ def dbt_profile_target(self):
"dbname": "dbt",
}

def test_profile_env_vars(self, project):
def test_profile_env_vars(self, project, logs_dir):

# Initial run
os.environ["ENV_VAR_USER"] = "root"
Expand All @@ -334,7 +334,7 @@ def test_profile_env_vars(self, project):
with pytest.raises(FailedToConnectError):
run_dbt(["run"], expect_pass=False)

log_output = Path(project.logs_dir, "dbt.log").read_text()
log_output = Path(logs_dir, "dbt.log").read_text()
assert "env vars used in profiles.yml have changed" in log_output

manifest = get_manifest(project.project_root)
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ def test_event_codes(self):
types.ExposureNameDeprecation(exposure=""),
types.InternalDeprecation(name="", reason="", suggested_action="", version=""),
types.EnvironmentVariableRenamed(old_name="", new_name=""),
types.ConfigLogPathDeprecation(deprecated_path=""),
types.ConfigTargetPathDeprecation(deprecated_path=""),
# E - DB Adapter ======================
types.AdapterEventDebug(),
types.AdapterEventInfo(),
Expand Down