-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature/source freshness hooks #9366
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9366 +/- ##
==========================================
+ Coverage 86.98% 87.03% +0.04%
==========================================
Files 187 187
Lines 24971 25026 +55
==========================================
+ Hits 21722 21782 +60
+ Misses 3249 3244 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Initial review covering first 2 commits of this change: #9322 (comment) |
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.
This looks fine. As things for possible future improvements, that whole flags processing thing is getting complicated. We should look for an opportunity to refactor it. Also mixing in different Status enums is something that's not wonderful. We need to do a rethink of all of those status codes sometime. It might be better to have a single enum instead of all of these different ones.
Yeah, I agree. It's definitely got a lot of responsibilities and is quite aware of dbt-specific CLI flags even though it's meant to be more generalized. In the meantime, I've added an additional unit test for the project-only flag behaviour so it'll be safer to refactor in the future.
We may be able to take a stab at this with some of the ongoing dbt/artifacts refactoring, but agree it shouldn't block this work. |
Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4787 |
resolves #5609
Problem
Currently,
on-run-*
hooks are not supported in thedbt source freshness
command, this PR makes it so it does.Solution
Changed the
FreshnessTask
class to inherit fromRunTask
(as all other task types that support hooks do).Alternative solutions:
GraphRunnableTask
and disabling inCompileTask
- feels a little messier (disabling parent functionality in inheriter).HookTask
in the middle of the inheritance tree - seems like a lot more work when every other hook-able task just inherits fromRunTask
.Additionally, this change has been made backwards-compatible with previous versions of dbt which did not include project hooks by default for
dbt source freshness
commands via a project flag + tests added across the different scenarios (default configuration, flag explicitly disabled, flag explicitly enabled)Checklist