-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Backport] deprecate materialization overrides from imported packages #9998
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.7.latest #9998 +/- ##
==============================================
+ Coverage 86.64% 86.67% +0.02%
==============================================
Files 179 179
Lines 26662 26722 +60
==============================================
+ Hits 23102 23161 +59
- Misses 3560 3561 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f7303c6
to
455d817
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
@@ -0,0 +1,6 @@ | |||
kind: Features | |||
body: Move flags from UserConfig in profiles.yml to flags in dbt_project.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: do we still support flags in profiles.yml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no would this mean a potential breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We fire a deprecation warning if flags are read from profiles.yml, but we still support it: https://github.com/dbt-labs/dbt-core/pull/9998/files#diff-8dc87da2257e24092820e0b29ea50eb16f002dd2c30b5879602b9311941858c0R820-R824
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all abstracted under read_project_flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally cool with starting to fire this this deprecation warning in v1.7.x
As a general rule, I like the idea of backporting + patching deprecation warnings to actively supported versions, as soon as we decide behavior might change / be officially deprecated (with opt-out) in the next minor release, which could otherwise be multiple months away
🎩 Did some final tophatting of this change using jaffle-shop! Here are the scenarios I tested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩 lgtm!
Backport of #9956 to 1.7
Solution
The easiest way to explain & review this PR is commit-by-commit:
require_explicit_package_overrides_for_builtin_materializations
flag to ProjectFlagsChecklist
No breaking interface changes. However, a deprecation warning will now be emitted for projects with flags defined in profiles.yml instead of dbt_project.yml. This PR introduces the capability to define flags in dbt_project.yml so I don't see an issue there.