-
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
Move SavedQuery
data artifact to dbt/artifacts
#9460
Move SavedQuery
data artifact to dbt/artifacts
#9460
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9460 +/- ##
==========================================
+ Coverage 87.89% 87.96% +0.07%
==========================================
Files 164 165 +1
Lines 21967 21975 +8
==========================================
+ Hits 19307 19331 +24
+ Misses 2660 2644 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ts` in artifacts
This is important because we want to move the `Export` class to artifacts. However, because it had functional parts we would have split it in half, with the data definition exists in artifacts and the functional specification defined in core. At first glance thats not problematic. However, the `SavedQuery` definition in artifacts would only be able to point at the data definition of `Export`, and then the function `SavedQuery` spec in core would have to override that with the functional `Export` definition that exists in core. This would make the inheritance rather wonky and confusing. This refactor simplifies thigs greatly because now we can move the entirety of `Export` to artifacts, and the core `SavedQuery` won't have to override anything.
Specifically the components in `contracts/graph/saved_queries.py` which are `Export`, `ExportConfig`, and `QueryParams` got moved to `artifacts/resources/v1/saved_query.py`. The moving of `Export` was made possible by the refactor in the previous commit.
If we had followed the general paradigm we've set, we would have split `DependsOn` into a data half and a functional half, with the data half going in artifacts. However, doing so overly complicates the work that we're doing. Additionally looking forward, we hope to simplify the `DependsOn` (as well as `MacroDependsOn`) to use `sets` instead of `lists`, thus allowing us to get rid of the fuctional part. We haven't done that refactor here because there is a reasonable amount of risk associated with such a change such that doign so should be it's own segement of work.
I debated about making this two commits. However I only realized we needed to also move `NodeVersion` when I was most the way through moving `RefArgs`, and instead of stashing, I just decided to due both. They're kind of inseparable anyways because it only makes sense to move `NodeVersion` if you move `RefArgs`, but you can't move `RefArgs` unless you also move `NodeVersion`. The two in one commit are still small enough that I'm okay with this.
8ff0769
to
44a1f11
Compare
SavedQuery
data artifact to dbt/artifacts
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.
approved pending 🟢 CI, which should be fixed once this is rebased on main (& #9473 is merged)
resolves #9386
Problem
We are moving data artifacts to
dbt/artifacts
in a piecewise fashion. We needed to moveSavedQuery
as part of that.Solution
Moved data portion of
SavedQuery
to dbt/artifacts (and all other classes that doing so required)Checklist