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

[Backport] deprecate materialization overrides from imported packages #9998

Merged
merged 9 commits into from
Apr 29, 2024

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Apr 22, 2024

Backport of #9956 to 1.7

Solution

The easiest way to explain & review this PR is commit-by-commit:

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

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.

@MichelleArk MichelleArk requested a review from a team as a code owner April 22, 2024 15:30
@cla-bot cla-bot bot added the cla:yes label Apr 22, 2024
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 98.24561% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 86.67%. Comparing base (6a54a4c) to head (2080ae7).

Files Patch % Lines
core/dbt/config/project.py 97.56% 1 Missing ⚠️
core/dbt/contracts/graph/manifest.py 94.44% 1 Missing ⚠️
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     
Flag Coverage Δ
integration 83.56% <92.98%> (+0.09%) ⬆️
unit 65.05% <88.59%> (-0.02%) ⬇️

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.

@MichelleArk MichelleArk force-pushed the backport-9971-to-1.7.latest branch from f7303c6 to 455d817 Compare April 23, 2024 20:02
@MichelleArk MichelleArk changed the title deprecate materialization overrides from imported packages [Backport] deprecate materialization overrides from imported packages Apr 23, 2024
Copy link
Member

@emmyoop emmyoop left a 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
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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

@MichelleArk
Copy link
Contributor Author

MichelleArk commented Apr 26, 2024

🎩 Did some final tophatting of this change using jaffle-shop! Here are the scenarios I tested:

  • dbt run passes as usual
  • dbt run when profiles.yml specifies config (but emits deprecation warning)
  • dbt run when dbt_project.yml specifies flags + respects their configurations as expected
  • custom materialization from root project works as expected
  • added a 'default' materialization override for view in an installed package -> saw deprecation warning
  • ^ + setting require_explicit_package_overrides_for_builtin_materializations: true -> no deprecation warning and confirmed materialization override not reached
  • require_explicit_package_overrides_for_builtin_materializations: false` -> deprecation warning, materialization override occurred

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩 lgtm!

@MichelleArk MichelleArk merged commit 87ac4de into 1.7.latest Apr 29, 2024
112 checks passed
@MichelleArk MichelleArk deleted the backport-9971-to-1.7.latest branch April 29, 2024 13:59
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.

6 participants