-
Notifications
You must be signed in to change notification settings - Fork 139
feat: derive model/test owners from dbt group owner when no direct owner is set #1034
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
Merged
+211
−1
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0ca6bbb
feat: derive model/test owners from dbt group owner when no direct ow…
devin-ai-integration[bot] 8184234
Merge remote-tracking branch 'origin/master' into feat/derive-owner-f…
devin-ai-integration[bot] e0165c7
test: skip group-owner test for dbt fusion (graph.groups not populated)
devin-ai-integration[bot] 3c1488e
refactor: drop redundant resource_type check in get_group_owner
devin-ai-integration[bot] 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 hidden or 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 hidden or 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 hidden or 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,17 @@ | ||
| {% macro get_group_owner(group_name) %} | ||
| {# | ||
| Resolve a dbt group's owner to a single string, preferring the owner email | ||
| and falling back to the owner name. Mirrors how dbt_groups exposes | ||
| owner_email / owner_name. Returns none when the group or owner is missing. | ||
| #} | ||
| {% if not group_name %} {% do return(none) %} {% endif %} | ||
| {% for group_node in graph.groups.values() %} | ||
| {% if group_node.get("name") == group_name %} | ||
| {% set owner_dict = elementary.safe_get_with_default( | ||
| group_node, "owner", {} | ||
| ) %} | ||
| {% do return(owner_dict.get("email") or owner_dict.get("name")) %} | ||
|
elazarlachkar marked this conversation as resolved.
|
||
| {% endif %} | ||
| {% endfor %} | ||
| {% do return(none) %} | ||
| {% endmacro %} | ||
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.
🚩 Behavioral change to owner enforcement: grouped models now pass enforce_owners checks
The
enforce_project_configurationsmacro atmacros/commands/enforce_project_configurations.sql:85checksflattened_node.owner | length == 0to enforce that models have owners. Sinceflatten_modelnow populates theownerfield from the group owner when no direct owner is set, models that previously failed this check (no direct owner) will now silently pass if they belong to a group with an owner. This may be the desired behavior (groups as an ownership mechanism), but it changes the semantics ofenforce_ownersfrom 'model must have a directly-assigned owner' to 'model must have an owner from any source'. Users relying onenforce_ownersto ensure explicit per-model ownership will no longer get warnings for grouped models.Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Good catch — this is a real, intended consequence.
enforce_project_configurations(line 85) checksflattened_node.owner | length == 0, so grouped models with a group owner will now passenforce_ownerseven without a direct+owner.This is consistent with the goal of the change: a dbt
+groupowner is treated as a legitimate ownership source, soenforce_ownersnow means "model has an owner from any source (direct or group)" rather than "model has a directly-assigned owner". For projects that consolidated ownership into groups (the motivating use case here), that's the desired behavior.Flagging to the repo owners: if you'd prefer
enforce_ownersto keep requiring a direct per-model owner, we can gate the group fallback behind a config var (e.g.derive_owner_from_group, default true) and have the enforcement check the direct owner only. Happy to add that if preferred.