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 #7046

Closed
wants to merge 1 commit into from
Closed

Conversation

stu-k
Copy link
Contributor

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

resolves #6960

Description

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

Checklist

@cla-bot cla-bot bot added the cla:yes label Feb 24, 2023
Comment on lines 402 to 407
description = (
f"The environment variable `{self.old_name}` has been renamed to `{self.new_name}`. "
f"Since `{self.old_name}` has been set, its value will be used instead of `{self.new_name}`."
)
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 message is lacking. @dbeatty10 Do you have a suggestion for a better message to communicate that

  • ENV_VAR_FOO has been deprecated in favor of ENV_VAR_BAR
  • If you set ENV_VAR_FOO its will be used instead of the value of ENV_VAR_BAR

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of this?

The environment variable `ENV_VAR_FOO` has been deprecated and renamed as `ENV_VAR_BAR`.
If `ENV_VAR_FOO` is currently set, its value will be used instead of `ENV_VAR_BAR`.
Set `ENV_VAR_BAR` and unset `ENV_VAR_FOO` to avoid this deprecation warning and ensure it works properly in a future release.

It's kinda wordy, but it communicates three key things:

  1. The name you are using is deprecated and it has a new name.
  2. Here's how it will behave in the meantime.
  3. Here's the action you should take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds great!

@stu-k stu-k requested review from MichelleArk, dbeatty10, a team and iknox-fa February 24, 2023 19:32
@stu-k stu-k closed this Feb 24, 2023
@stu-k stu-k reopened this Feb 24, 2023
core/dbt/cli/flags.py Outdated Show resolved Hide resolved
@stu-k stu-k closed this Feb 27, 2023
@stu-k stu-k reopened this Feb 27, 2023
@stu-k stu-k marked this pull request as ready for review February 27, 2023 15:59
@stu-k stu-k requested review from a team as code owners February 27, 2023 15:59
@stu-k stu-k requested review from gshank and MichelleArk February 27, 2023 15:59
@stu-k stu-k force-pushed the CT-2109/flag-param-renaming branch from bdbdea0 to 7ceb9bc Compare February 28, 2023 18:49
@stu-k stu-k force-pushed the CT-2109/flag-param-renaming branch from 7ceb9bc to 42ca93f Compare February 28, 2023 18:51
@stu-k stu-k closed this Feb 28, 2023
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
3 participants