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

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Mar 17, 2023

resolves #6882

Description

  • We have back-compat (for now) for log-path and target-path being set in dbt_project.yml
  • Since these are now properly Flags (settable via CLI flag or env var), it is inconsistent (and added complexity in our codebase / initialization flow) that they can also be set in dbt_project.yml
  • Add a deprecation warning that we plan to remove support for these configs in a future version of dbt-core. 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

Checklist

Comment on lines +320 to +321
# 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
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.

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

Comments mostly around readability. I'm generally on board for this and how you've gone about it.

Also, just an aside, man, I used to use these flags way back when. Crazy that they're going away. tempus fugit.

@@ -298,7 +298,7 @@ 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, exp_path=None, default=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what exp_path is.....expected path? Explicit path? Something else? An explicit variable would be nice for readable/reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what's default exactly?

Copy link
Contributor Author

@jtcohen6 jtcohen6 Mar 21, 2023

Choose a reason for hiding this comment

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

I'll rename these!

  • exp_pathexpected_path
  • defaultdefault_value

Comment on lines +320 to +321
# 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
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.

@jtcohen6 jtcohen6 requested a review from VersusFacit March 21, 2023 20:42
Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

Thanks for making the variable names explicit. Last night without knowing exactly what they were, I was just having trouble parsing the code. It's much clearer now!

@jtcohen6
Copy link
Contributor Author

@VersusFacit Thank you for the review! :)

@jtcohen6 jtcohen6 merged commit 8225a00 into main Mar 21, 2023
@jtcohen6 jtcohen6 deleted the jerco/deprecation-warn-log-target-path branch March 21, 2023 21:31
dbeatty10 added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Apr 24, 2024
[Preview](https://docs-getdbt-com-git-dbeatty10-patch-6-dbt-labs.vercel.app/reference/dbt_project.yml)

## What are you changing in this pull request and why?

We deprecated `log-path` and `target-path` from `dbt_project.yml`
beginning in v1.5 via dbt-labs/dbt-core#7185.

So this PR removes them from the docs for `dbt_project.yml` for later
versions to reduce confusion.

## Checklist
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
so my content adheres to these guidelines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2054] Add deprecation warnings for LOG_PATH and TARGET_PATH in dbt_project.yml
2 participants