dbt-core icon indicating copy to clipboard operation
dbt-core copied to clipboard

Move manifest nodes to dbt/artifacts

Open gshank opened this issue 1 year ago • 3 comments

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

  • [x] I have read the contributing guide and understand what's expected of me
  • [x] I have run this code in development and it appears to resolve the stated issue
  • [x] This PR includes tests, or tests are not required/relevant for this PR
  • [x] 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
  • [x] This PR includes type annotations for new and modified functions

gshank avatar Feb 07 '24 17:02 gshank

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (1ec5e22) 88.00% compared to head (07229e2) 88.00%. Report is 2 commits behind head on main.

Files Patch % Lines
core/dbt/artifacts/resources/base.py 87.09% 4 Missing :warning:
core/dbt/artifacts/resources/v1/snapshot.py 97.56% 1 Missing :warning:
core/dbt/contracts/graph/nodes.py 96.66% 1 Missing :warning:
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     
Flag Coverage Δ
integration 85.59% <97.51%> (-0.08%) :arrow_down:
unit 62.10% <94.54%> (+0.17%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 07 '24 17:02 codecov[bot]

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.

gshank avatar Feb 07 '24 17:02 gshank

There's a couple of failing unit tests that I need to chase down, but wanted to give people on opportunity to review.

gshank avatar Feb 07 '24 17:02 gshank