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

Create method for env var deprecation #7086

Merged
merged 5 commits into from
Mar 8, 2023
Merged

Conversation

stu-k
Copy link
Contributor

@stu-k stu-k commented Feb 28, 2023

resolves #6960

Description

AI PR summary: This pull request includes a wide range of changes to improve the functionality, maintainability, and documentation of the codebase. The most important changes include handling deprecated environment variables, adding a new Notification::Cache class, and updating the version numbers and scripts in the documentation files.

Added a method of warning about deprecated environment variables and implemented it for DBT_NO_PRINT -> DBT_PRINT.

Checklist

@stu-k stu-k requested review from a team as code owners February 28, 2023 18:59
@stu-k stu-k requested review from gshank and emmyoop February 28, 2023 18:59
@cla-bot cla-bot bot added the cla:yes label Feb 28, 2023
@stu-k stu-k requested review from MichelleArk, ChenyuLInx and a team February 28, 2023 19:14
@stu-k stu-k closed this Feb 28, 2023
@stu-k stu-k reopened this Feb 28, 2023
Comment on lines +80 to +82
DEPRECATED_PARAMS = {
"deprecated_print": "print",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the approach here be extensible to other deprecated/renamed flags & env vars? (#6903)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@@ -97,6 +103,10 @@ def assign_params(ctx, params_assigned_from_default):
# when using frozen dataclasses.
# https://docs.python.org/3/library/dataclasses.html#frozen-instances
if hasattr(self, param_name.upper()):
if param_name in deprecated_env_vars:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to consider a case where people set both deprecated and and the new env var?

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 that case. The warning message states that the deprecated env var will always be respected over the flag and if you want to respect the flag you should unset it.

param_source = ctx.get_parameter_source(param_name)
if param_source == ParameterSource.DEFAULT:
continue
elif param_source != ParameterSource.ENVIRONMENT:
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 actually possible since those params are already hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be but I'd rather be explicit about what should happen here.

@stu-k stu-k requested a review from ChenyuLInx March 3, 2023 18:20
@stu-k stu-k force-pushed the CT-2109/flag-param-renaming branch from 387b10d to 663d8b8 Compare March 6, 2023 15:39
@stu-k stu-k requested a review from aranke March 6, 2023 16:20
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Hmm, should we implement DeprecatedOption as outlined in this Stack Overflow post: https://stackoverflow.com/a/50402799?

The only reservation I have with the current approach is that it unbundles deprecated options from non-deprecated ones, and we'd want to keep the two close?

@stu-k
Copy link
Contributor Author

stu-k commented Mar 7, 2023

@aranke That's a good idea, let me play around with that.

@stu-k
Copy link
Contributor Author

stu-k commented Mar 8, 2023

@aranke The DeprecatedOption class doesn't account for environment variables, which is what this ticket is really about, not the flag params. I'm sure the DeprecatedOption class could be refactored to account for the environment variables, but I'm not sure I want to undertake that right now.

@stu-k stu-k requested a review from aranke March 8, 2023 16:59
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

LGTM!

@stu-k stu-k merged commit 64b8a12 into main Mar 8, 2023
@stu-k stu-k deleted the CT-2109/flag-param-renaming branch March 8, 2023 17:07
peterallenwebb added a commit that referenced this pull request Mar 22, 2023
* ct-2198: clean up some type names and uses

* CT-2198: Unify constraints and constraints_check properties on columns

* Make mypy version consistently 0.981 (#7134)

* CT 1808 diff based partial parsing (#6873)

* model contracts on models materialized as views (#7120)

* first pass

* rename tests

* fix failing test

* changelog

* fix functional test

* Update core/dbt/parser/base.py

* Update core/dbt/parser/schemas.py

* Create method for env var deprecation (#7086)

* update to allow adapters to change model name resolution in py models (#7115)

* update to allow adapters to change model name resolution in py models

* add changie

* fix newline adds

* move quoting into macro

* use single quotes

* add env DBT_PROJECT_DIR support #6078 (#6659)

Co-authored-by: Jeremy Cohen <[email protected]>

* Add new index.html and changelog yaml files from dbt-docs (#7141)

* Make version configs optional (#7060)

* [CT-1584] New top level commands: interactive compile (#7008)

Co-authored-by: Github Build Bot <[email protected]>

* CT-2198: Add changelog entry

* CT-2198: Fix tests which broke after merge

* CT-2198: Add explicit validation of constraint types w/ unit test

* CT-2198: Move access property, per code review

* CT-2198: Remove a redundant macro

* CT-1298: Rework constraints to be adapter-generated in Python code

* CT-2198: Clarify function name per review

---------

Co-authored-by: Gerda Shank <[email protected]>
Co-authored-by: Emily Rockman <[email protected]>
Co-authored-by: Stu Kilgore <[email protected]>
Co-authored-by: colin-rogers-dbt <[email protected]>
Co-authored-by: Leo Schick <[email protected]>
Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: FishtownBuildBot <[email protected]>
Co-authored-by: dave-connors-3 <[email protected]>
Co-authored-by: Kshitij Aranke <[email protected]>
Co-authored-by: Github Build Bot <[email protected]>
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-2109] Re-name print flag to meet naming convention
5 participants