dbt-core: [CT-1436] Treat warnings as errors for specific events, based on user configuration

A functionally similar configuration syntax was recently proposed in the dbt should know more about semantic information issue. It turns out that entity definitions also need to provide an overall true/false option, plus some additional include/exclude modifications.

Implementing the same configuration syntax for warn_error would need some custom validation (expecting valid exception class names, not column names), but would have the benefit of a consistent experience with what’s being proposed for entities. Below are a few examples of the syntax applied to the warn_error config:

# all warnings are treated as exceptions, same as warn_error: true
warn_error: 
  include: *  

# all warnings are treated as exceptions except NoNodesForSelectionCriteria, UnusedResourceConfigPath
warn_error: 
  include: * 
  exclude: 
    - NoNodesForSelectionCriteria
    - UnusedResourceConfigPath 

# only NoNodesForSelectionCriteria, UnusedResourceConfigPath are treated as errors
warn_error: 
  include: 
    - NoNodesForSelectionCriteria
    - UnusedResourceConfigPath 

# invalid - exclude only supported when include is '*'
warn_error: 
  include:
    - ThisEvent
    - ThatEvent 
  exclude: 
    - NoNodesForSelectionCriteria
    - UnusedResourceConfigPath 

# invalid
warn_error: 
  exclude: * 

c’est comme ça

We could support “all” as an alias:

warn_error:
  include: all # same as "*"

Looking into the code I would say we actually have three types of warnings: deprecations, bad configs and tests + source freshness checks. Both deprecations and configs are already implemented differently since they are both utilizing warn_or_error where tests and source configs are setting the result status based on if the warn-error flag is set but do not use warn_or_error.

1. Deprecations - it seems safe to never allow these to be treated as errors.
2. Bad Configs - We have had community members asking to be able to turn specific warnings of this class into errors in the past(linked in original issue body). The big question is if this is something we want to support (I think yes) and to what extent/customizability?
3. tests + source freshness checks - To allow this to be triggered to fail, it would just require using a separate flag since it’s really one type of warning, just a different type. The warn-error flag could just get deprecated at some point. The problem here would be clearly defining what wins (or throwing an error when old and new flags are used within the same project)

Looking at other compilers: C# covers its bases with

TreatWarningsAsErrors / -warnaserror: Treat all warnings as errors
WarningsAsErrors / -warnaserror: Treat one or more warnings as errors
WarningsNotAsErrors / -warnnotaserror: Treat one or more warnings not as errors
NoWarn / -nowarn: Set a list of disabled warnings.

GCC actually does something similar where you can

1. Make all warnings errors
2. Make a specified warning an error
3. Specify a warning to not become an error even when you’ve already indicated all warnings should be errors

Making a note to not lose it: mypy does something similar with error codes (disable_error_codes and enable_error_codes) but is not exactly the same as what we’re trying to do. The interesting thing is that it’s two separate configs and they also make the executive decision that (enable_error_codes)... will override disabled error codes from the disable_error_code option. So if both are filled it’s well documented what wins while also having explicit fields.

(summary from follow-up convo w @MichelleArk)

We can’t have one CLI flag that’s both a boolean and also a dict. argparse isn’t happy with that, click isn’t happy with that, and if we’re being honest with ourselves, we shouldn’t be too happy about doing it either.

Let’s make a new config (+ flag/env var), call it warn_error_options, data structure is always a dictionary that looks like {"include": str | list, "exclude": list}. This will be distinct from and mutually exclusive with the existing boolean config/flag/var named warn_error. Note: This means it would not be possible to supply warn_error_options in UserConfig and override with plain old --warn-error in env var / CLI — you have to override like with like.

$ DBT_WARN_ERROR_OPTIONS='{"include": "*", "exclude": ["NoNodesForSelectionCriteria", "UnusedResourceConfigPath"}' dbt run
$ dbt --warn-error-options '{"include": "*", "exclude": ["NoNodesForSelectionCriteria", "UnusedResourceConfigPath"}' run

Is it a bit ugly? Yes. But it’s a single variable, you only need to set it once, you can pop it in a yaml/json validator first, and ultimately, this is a capability intended for fairly advanced users. More important that it’s possible on the CLI, than it being ergonomic & delightful. Ultimately, we should also move UserConfig somewhere else (#6266) such that dbt Cloud users can actually make use of it, without needing to modify profiles.yml directly.

These two are still identical:

$ dbt --warn-error run
$ dbt --warn-error-options '{"include": "all"}' run

Theoretically these should also be equivalent:

$ dbt --no-warn-error run
$ dbt --warn-error-options '{"include": []}' run

Up to us if we eventually (dbt v2) deprecate the existing --warn-error config. It’s a lot sugarier for sure. No deprecation warning for now.

& the good old fashioned --warn-error bool will also want to keep working, with the same effect as “include all”:

$ dbt --warn-error run
$ dbt --warn-error '{"include": "all"}' run

That could work nicely! One edge case I can think of would be for entity definitions where a column name is actually called ‘all’ - how would dbt disambiguate between a column named ‘all’ vs ‘include all columns’? I don’t think this is an issue for the warn_error configuration since dbt-core provides the exception names.

One way to disambiguate between a column named ‘all’ vs the directive to include all column names without complex validation would be based on the presence of ‘all’ as a string vs a list item. For example:

# include all columns
incude: all 

vs

# column named 'all'
include:
  - all 

One thing to check during implementation: The spec looks great as yaml, but dbt Cloud users don’t have the ability to write/edit profiles.yml, which is where UserConfig lives right now.

It will need to be possible to pass these data structures in as an env var (DBT_WARN_ERROR) or CLI flag (--warn-error) as well (even if it’s super ugly to look at).

Something like:

$ DBT_WARN_ERROR='{"include": "*", "exclude": ["NoNodesForSelectionCriteria", "UnusedResourceConfigPath"}' dbt run
$ dbt --warn-error '{"include": "*", "exclude": ["NoNodesForSelectionCriteria", "UnusedResourceConfigPath"}' run

An observation - it appears yml doesn’t support non-quoted special characters such as *. See link here and explanation from Stack Overflow.

YAML doesn’t require quoting most strings but you’ll want to quote special characters if they fall within a certain list of characters.

This implies that the syntax will be the slightly less clean:

warn_error:
  include: '*'

My understanding of Nate’s proposal:

• Flag specific warnings to be ignored
• warn_error is still a simple on/off, for all non-ignored warnings
• We’d want to disambiguate between warnings in the sense of “bad configuration,” and warnings in the sense of tests + source freshness checks with a certain status result

The appeal of that approach is it’s more in keeping with the standard practice of other compilers. The downside is losing some of the flexibility that the more-complex configuration (proposed in this issue) would enable.

As part of this work, we should remove warn_or_error entirely to make the warn_error config work for all warnings, not just a subset.

This will involve changing all callsites for warn_or_error to fire_event. dbt-snowflake uses warn_or_error in connections.py.

Note: This could be a breaking change for community adapters if they use warn_or_error.