Move manifest nodes to dbt/artifacts #9538
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9538 +/- ##
==========================================
- Coverage 88.00% 88.00% -0.01%
==========================================
Files 168 176 +8
Lines 22210 22311 +101
==========================================
+ Hits 19546 19634 +88
- Misses 2664 2677 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Questions: There's some validation logic in the configs that I'm not sure should be in artifacts. Should we attempt to move that out someplace else? I put all of the "resources" into a manifest_nodes.py file, because a number of them were pretty small and it seemed like overkill to give each one separate files, but I don't have strong feelings about that. Anything in the "base" resources dictionary will not be versioned, so we should be sure that it makes sense to put things there. I did not put UnitTestNode into artifacts because right now that resource is never written out into json artifacts, it's only used internally in constructing the temporary unit testing manifest. None of the embedded objects (i.e. Configs and things like RefArgs) can use the pattern of putting the fields and methods in different classes, because then there are different types in the artifacts and the nodes and mypy complains. It works fine for top-level resources though. |
|
There's a couple of failing unit tests that I need to chase down, but wanted to give people on opportunity to review. |
| class ParsedNodeMandatory(GraphResource, HasRelationMetadata): | ||
| alias: str | ||
| checksum: FileHash | ||
| config: NodeConfig = field(default_factory=NodeConfig) | ||
|
|
||
| @property | ||
| def identifier(self): | ||
| return self.alias | ||
|
|
||
|
|
||
| @dataclass | ||
| class ParsedNode(ParsedNodeMandatory): | ||
| tags: List[str] = field(default_factory=list) | ||
| description: str = field(default="") | ||
| columns: Dict[str, ColumnInfo] = field(default_factory=dict) | ||
| meta: Dict[str, Any] = field(default_factory=dict) | ||
| group: Optional[str] = None | ||
| docs: Docs = field(default_factory=Docs) | ||
| patch_path: Optional[str] = None | ||
| build_path: Optional[str] = None | ||
| deferred: bool = False | ||
| unrendered_config: Dict[str, Any] = field(default_factory=dict) | ||
| created_at: float = field(default_factory=lambda: time.time()) | ||
| config_call_dict: Dict[str, Any] = field(default_factory=dict) | ||
| relation_name: Optional[str] = None | ||
| raw_code: str = "" | ||
|
|
||
|
|
||
| @dataclass | ||
| class CompiledNode(ParsedNode): |
There was a problem hiding this comment.
We could rename these to ParsedResource (& ParsedResourceMandatory) + CompiledResource and define them in https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/artifacts/resources/base.py to be more consistent with the base + graph resource refactors.
There was a problem hiding this comment.
We can't add anything to classes in base.py because they aren't versioned. Are we sure that this is what we want?
There was a problem hiding this comment.
Discussed offline: there isn't really a clear policy shaking out to discern whether these type of base resource classes should be defined in v1/ or one directory above in base.py. Let's revisit this in later on once we have backward/forward compatibility considered and tested (issue: #9615) , potentially as part of a more holistic 'final' reorganization push.
MichelleArk
added
the
artifact_minor_upgrade
resolves #9388
Problem
As part of the initiative to make a package to handle artifact generation, serializing and validation we are moving things to a separate dbt/artifacts directory. This PR is to move the nodes/resources in the ManifestNodes union.
Solution
Move the nodes in the ManifestNodes union to the artifacts package.
Checklist