-
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
parse group resource #6921
Merged
Merged
parse group resource #6921
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
7d5f81c
parse group resource
MichelleArk 50d9471
changelog entry
MichelleArk e8424c7
update parser README
MichelleArk f17b360
partial parsing for groups
MichelleArk 7cf69a8
partial parsing functional tests for groups
MichelleArk 9ed4264
Merge branch 'main' into CT-1989/parse-group-resource
MichelleArk f2f08fc
Add generated CLI API docs
FishtownBuildBot 083b1b8
add groups to flat graph
MichelleArk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Features | ||
body: Parse 'group' resource | ||
time: 2023-02-09T09:34:09.547006-05:00 | ||
custom: | ||
Author: michelleark | ||
Issue: "6921" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theowner
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theflat_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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