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

Update fire_event to handle warn_or_error logic #237

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

QMalcolm
Copy link
Contributor

@QMalcolm QMalcolm commented Jan 17, 2025

resolves #236

Description

In dbt-labs/dbt-core#11116 we have resolved to allow people to opt in to having all warnings be handled by the logic of the warn_or_error function. There are a few ways we could have solved this

  1. Update every warn level event currently fired with fire_event to be conditionally either fired with warn_or_error or fire_event based on a behavior flag
  2. Have core implement its own fire_event helper which does what is described in (1) for every warn passed into it (an then updated every call to dbt-common's fire_event to instead be dbt-core's fire_event
  3. Update dbt-common's fire_event to optionally put every warn even through the warn_or_error logic depending on a behavior flag
  4. Do (2) or (3) but instead of a single behavior flag, a behavior flag per warn level event / cohorts of warn level events

This PR implements option (3), as I believe (3) to be less error-prone than (1) or (2) as both (1) and (2) allow us to accidentally forget to send warnings through the new workflow. However, as I was opening this PR I realized (3) is not perfect. For instance, consider that we introduce a new warning for an already existing feature in core. If a user has opted in via the behavior flag, and is running with --warn-error, then this new warning could break their previously working project 🫠

Checklist

…ager`

`WARN_ERROR` and `WARN_ERROR_OPTIONS` were introduced _before_ we had our
`EventManager` architecture. We never moved them, because we never had a need
to. However, we're working to wrap `warn_or_error` functionality into `fire_event`.
As such, `EventManager` will need the context of `warn_error` and `warn_errror_options`.
As a secondary effect, we're eliminating some globals / module variables, which will make
our event architecture a little less complicated.
@QMalcolm QMalcolm requested a review from a team as a code owner January 17, 2025 20:41
Now whenever `warn_or_error` is called, it actually just passes it through
to `fire_event`. That is, `warn_or_error` will now be deprecated, and shouldn't
be used. As `warn_or_error` optionally takes a "node", `fire_event` had to be
updated to handle this extra argument.
…rn_or_error`

Previously only `warn` events handed directly to `warn_or_error` got the
`warn_or_error` treatment. In the prior commit we moved the `warn_or_error`
handling logic to `fire_event` which meant _all_ warnings were suddenly being
handled by the `warn_or_error` logic. This is undesirable as it would break a
lot of dbt-core projects. This commit takes it a step back by only using the
`warn_or_error` logic in `fire_event` if `force_warn_or_error_handling=True` is
passed to `fire_event` (by default it is `False`). Calls to `warn_or_error` will
_automatically_ pass `True` for this parameter.
@cla-bot cla-bot bot added the cla:yes label Jan 17, 2025
… logic

We want to move to a world where _all_ warnings are handled by the `warn_or_error`
logic that now lives in `fire_event`. However we can't by default do that as that
would break many dbt-core projects currently. That is, not all warnings in dbt-core
are fired with `warn_or_error`. Thus if a project has `--warn-error` set, some set
of warnings which weren't previously being handled by `warn_or_error` would now
be handled by `warn_or_error` if `fire_event` handled all warnings with `warn_or_error`,
which would cause errors that were previously happening. Thus by default _we don't_
handle all warnings in `fire_event` with `warn_or_error`. Instead a warning in
`fire_event` will only be handled with `warn_or_error` logic if either
1. `force_warn_or_error_handling` is passed as True
OR
2. `require_warn_or_error_handing` is set to True

(1) only happens if the event was initially sent via the old `warn_or_error` function
(2) will only happen if a corresponding project flag set in dbt-core
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.94%. Comparing base (2253401) to head (3007a6c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   68.89%   68.94%   +0.05%     
==========================================
  Files          52       52              
  Lines        3433     3439       +6     
==========================================
+ Hits         2365     2371       +6     
  Misses       1068     1068              
Flag Coverage Δ
unit 68.94% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Allow fire_event to optionally handle warn_or_error functionality
2 participants