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

[CT-2169] [Bug] All global configs should also be settable in ProjectFlags #7036

Open
Tracked by #6706
jtcohen6 opened this issue Feb 23, 2023 · 7 comments
Open
Tracked by #6706
Labels
bug Something isn't working user docs [docs.getdbt.com] Needs better documentation

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 23, 2023

Opening from #6903 (comment)

There are some "global configs" that are supported as params (flag / env var), but missing in ProjectFlags.

Acceptance criteria:

Relevant, but very much out of scope for this issue:

@jtcohen6 jtcohen6 added tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality Team:Execution labels Feb 23, 2023
@github-actions github-actions bot changed the title All global configs should also be settable in UserConfig [CT-2169] All global configs should also be settable in UserConfig Feb 23, 2023
@jtcohen6
Copy link
Contributor Author

But maybe not --log-path or --target-path. Not sure any "path"-specific configs make a whole lot of sense! :)

@jtcohen6
Copy link
Contributor Author

Switching this from tech_debt to bug, because it's actually unexpected behavior, given our current documentation

@jtcohen6 jtcohen6 added bug Something isn't working and removed tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality labels Apr 18, 2023
@jtcohen6 jtcohen6 changed the title [CT-2169] All global configs should also be settable in UserConfig [CT-2169] [Bug] All global configs should also be settable in UserConfig Apr 18, 2023
@emmyoop
Copy link
Member

emmyoop commented Jul 17, 2023

Add a test to pull out everything global and ensure there are configs.

@dbeatty10
Copy link
Contributor

Caution

This issue is in the same area of code as #9183 / #9289.

Any solutions to #9183 and #7036 should be compared to each other to make sure one doesn't accidentally stomp on / overwrite the solution of the other.

@dbeatty10
Copy link
Contributor

dbeatty10 commented Dec 18, 2023

I took a swing at auditing our global configs versus UserConfig and recorded them in this internal spreadsheet.

Global configs missing from UserConfig

After filtering out flags that are hidden=True or click.Path or excluded for other reasons*, these are the global configs missing from UserConfig:

  • print
  • quiet
  • log_cache_events

Exclusions

Previously decided here to exclude profile and target from this effort.

Excluded version since it is a unique flag that merely displays the relevant versions before exiting any further execution.

Hidden flags that are missing from UserConfig
  • deprecated_print
  • enable_legacy_logger
  • log_file_max_bytes
  • macro_debugging
  • partial_parse_file_diff
  • single_threaded

UserConfig missing from global configs

There are were a handful that are missing the opposite direction (but are now fixed by #9040):

  • indirect_selection
  • log_format
  • warn_error
  • warn_error_options

For refinement: determine if these are okay and/or desired.

All of these except for indirect_selection are covered by #8715 / #9040.

@dbeatty10 dbeatty10 added the user docs [docs.getdbt.com] Needs better documentation label Jan 13, 2024
@dbeatty10 dbeatty10 changed the title [CT-2169] [Bug] All global configs should also be settable in UserConfig [CT-2169] [Bug] All global configs should also be settable in ProjectFlags Jan 29, 2024
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Feb 5, 2024

The remaining scope of this issue is to decide whether these "global" flags should be settable in dbt_project.yml:

  • print
  • quiet
  • log_cache_events

The question being: Do these feel different enough from all other global configs to justify the inconsistency? Right now, we only disallow setting file-path flags (--log-path, --target-path, --state) in the project flags.

@dbeatty10
Copy link
Contributor

print, quiet, and log_cache_events don't seem different enough to me to justify the inconsistency.

At various times, I've found myself tying to set each of these within profiles.yml at different times. Granted, print and log_cache_events would be most likely to be used while troubleshooting. And my uses of quiet were specifically to get parseable output from dbt list.

But it seems the least amount of effort and confusion within the code base and the documentation to make them global in dbt_project.yml. Do you buy it @jtcohen6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

No branches or pull requests

4 participants