Skip to content
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

[CT-1991] Parse group config on 'groupable' nodes #6823

Closed
Tracked by #6747
MichelleArk opened this issue Feb 1, 2023 · 1 comment · Fixed by #6965
Closed
Tracked by #6747

[CT-1991] Parse group config on 'groupable' nodes #6823

MichelleArk opened this issue Feb 1, 2023 · 1 comment · Fixed by #6965
Assignees
Labels
model_groups_access Issues related to groups multi_project user docs [docs.getdbt.com] Needs better documentation

Comments

@MichelleArk
Copy link
Contributor

MichelleArk commented Feb 1, 2023

Introduce and parse a new optional group config on 'groupable' nodes: models, seeds, snapshots, tests, analyses, and metrics.

# models/marts/github/github.yml

models:
  - name: int__github_issue_label_history
    config: 
      group: github
  - name: fct_github_issues
    config: 
      group: github

Raise an exception during resolution if a group is specified (by name) that doesn't exist in the project. Could potentially introduce a new type of error - DependencyError - for these types of more granular resolution errors as opposed to existing compilation/parsing errors (more discussion here)

@github-actions github-actions bot changed the title Parse group property on 'groupable' nodes [CT-1991] Parse group property on 'groupable' nodes Feb 1, 2023
@MichelleArk MichelleArk changed the title [CT-1991] Parse group property on 'groupable' nodes [CT-1991] Parse group config on 'groupable' nodes Feb 1, 2023
@MichelleArk MichelleArk self-assigned this Feb 13, 2023
@jtcohen6
Copy link
Contributor

copying from Slack

What's a config versus a top-level node property/attribute? (docs)

Generally, configs are anything the user/adapter wants, and they control runtime/materialization behavior ("the physical layer"). They're a bit wilder, since it's a dict that can contain anything. By contrast, top-level node properties are more strongly typed, more logical in nature. But, they can't be set from the in-file config or in dbt_project.yml based on directory structure for many models at once.

So: Should group be a nested config, or a top-level node attribute?

We think it should be a config specifically because we want the ability to do:

# dbt_project.yml
models:
  my_project:
    marts:
      github:
        +group: github

It feels like a top-level property. But it also feels like tags and meta, and we decided to make those both configs, for ease of setting them in all the places.

Risk: I had previously imagined that groups could be another step in the config inheritance hierarchy. If group is a config, I think it would be trickier for us to resolve that config and then also use the value of the config for any additional config inheritance/resolution. That's definitely out of scope for now, and as one-way doors go, I don't think we're losing out on all that much.

Conclusion:

  • allow users to configure group in all the places (dbt_project.yml, yaml file, in-file {{ config() }})
  • afterward, copy it out of node.config.group to be a top-level node attribute (node.group) — like we do with tags + meta

The only thing that feels bad about that is duplication, when manifests can already get so big. But I don't think we can solve for that now, and we shouldn't let it stop us from doing the right-est thing in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model_groups_access Issues related to groups multi_project user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants