parse group resource #6921
parse group resource #6921
Conversation
MichelleArk
force-pushed
the
CT-1989/parse-group-resource
branch
from
b40bf3c to
6d68309
Compare
MichelleArk
requested review from
stu-k,
emmyoop,
peterallenwebb and
gshank
and removed request for
stu-k
MichelleArk
force-pushed
the
CT-1989/parse-group-resource
branch
2 times, most recently
from
3abb842 to
4911ccc
Compare
MichelleArk
force-pushed
the
CT-1989/parse-group-resource
branch
from
4911ccc to
f17b360
Compare
|
I just merged some changes to tests/functional/artifacts/data/state/v8/manifest.json, you probably need to merge main. |
MichelleArk
force-pushed
the
CT-1989/parse-group-resource
branch
from
eb41482 to
7cf69a8
Compare
|
@gshank - Thanks for letting me know! I've merged main, should be ready for review. |
| @@ -903,6 +926,19 @@ def delete_schema_exposure(self, schema_file, exposure_dict): | |||
| elif unique_id in self.saved_manifest.disabled: | |||
| self.delete_disabled(unique_id, schema_file.file_id) | |||
|
|
|||
| # groups are created only from schema files, so just delete the group | |||
There was a problem hiding this comment.
When there are references to groups in other nodes, then we'll have to go through and schedule every node with this groups for reparsing.
There was a problem hiding this comment.
Makes sense. That would also go for modifications to existing groups in addition to deletions, right?
I'm thinking it makes more sense to add that to the follow-up PR that parses group configs on group-able nodes: #6965.
There was a problem hiding this comment.
We'd only need to reparse for modifications of existing groups if information from the group is pulled into the node that needs to be rebuilt. If we only have the name in the node then probably not.
There was a problem hiding this comment.
On thinking about this... Would people be pulling group information into model jinja code? Like model.group.owner? Then we'd have to reparse.
There was a problem hiding this comment.
We're not storing any additional group information directly on the node, right? Users would need to look up .groups() in the manifest / flat_graph (a.k.a. {{ graph }} in Jinja code) to get information about the owner
There was a problem hiding this comment.
We're not storing any additional group information directly on the node, right? Users would need to look up .groups() in the manifest / flat_graph (a.k.a. {{ graph }} in Jinja code) to get information about the owner
Right - we're just storing the name. Changes to which will be captured as a deleted + new group. So {{ model.config.group }} would just be able to access the name, not owner.
We're not currently adding groups to the flat graph - should we be?
There was a problem hiding this comment.
Ooh, I'd just seen this change and thought we were.
I think we probably should. This feels like valid metadata that someone might want access to in the Jinja context (including the folks who maintain the dbt_artifacts package), and I can't see potential for anti-pattern shenanigans. (There are indeed some things we exclude from the flat_graph, such as macros. I would say that exclusion is somewhat intentional. I just remembered this issue that I tagged myself to refine forever ago: #4919)
It would just require an update to this method, and associated tests
There was a problem hiding this comment.
okay, makes sense! I had actually added it on first pass of this work but wasn't convinced it should be part of the flat graph so removed it. Will add now
resolves #6822
Description
Checklist
changie newto create a changelog entry