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

Deprecate user provided custom top level attributes on nodes #11336

Open
Tracked by #11335
QMalcolm opened this issue Feb 26, 2025 · 0 comments
Open
Tracked by #11335

Deprecate user provided custom top level attributes on nodes #11336

QMalcolm opened this issue Feb 26, 2025 · 0 comments

Comments

@QMalcolm
Copy link
Contributor

QMalcolm commented Feb 26, 2025

Problem

In a YAML file you can specify top level properties for a node that the node itself does not have. Consider the following

# my_model.yaml

models:
  - name: my_model
    descripiton: "This model does a thing"
    config:
      ...
    my_custom_attribute: "My custom value"

Believe it or not there are three issues with allowing for this.

Firstly, it means anytime a new attribute is defined on the model for a feature in dbt-core, it is potentially breaking for a slew projects. For instance, recently we added the freshness property to models (#11170). This broke people's projects en-mass because people had added freshness to their models, even though models previously didn't support freshness, because their freshness value did not match the structure of model freshness.

This leads into the second problem in that people might add properties to nodes thinking it will do something when it does nothing. That is, before we added freshness to models it was possible to define freshness on model anyway and then expect it to do something, and it didn't.

Finally, the third issue is that it means it's nearly impossible to detect typos. You may not have noticed in the example YAML above, but I misspelled the "description" property as "descripiton". When this happens, again we think we're doing something (describing our model), but that information doesn't get propagated.

Solution

Deprecate the custom top level properties on nodes. We want to make it an error, and at some time it will become an error, but we can't move straight to it being an error. As it being an error would be breaking to projects everywhere, we need to first deprecate it to give people time to fix the problem.

Acceptance criteria

  • One deprecation warning is fired if any custom top level properties on nodes are defined
  • Deprecation warning is fired if a custom top level property on a node is present
  • A deprecation warning is not fired if a custom top level property on a node is not present
  • In non-verbose mode a count of the number of occurences is included in the deprecation warning
  • In verbose mode all the instances are listed in the deprecation warning

Suggested Tests

  • Deprecation warning is fired if a custom top level property on a node is present
  • A deprecation warning is not fired if a custom top level property on a node is not present
  • In non-verbose mode a count of the number of occurences is included in the deprecation warning
  • In verbose mode all the instances are listed in the deprecation warning

Backport?

N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant