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

Reorganize nodes, node.config in manifest #3557

Closed
jtcohen6 opened this issue Jul 11, 2021 · 3 comments
Closed

Reorganize nodes, node.config in manifest #3557

jtcohen6 opened this issue Jul 11, 2021 · 3 comments
Labels
artifacts enhancement New feature or request stale Issues that have gone stale

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 11, 2021

Describe the feature

Our work on #2401 promises to rationalize the ways that node configs interact with node-level properties. In the general case, a config should be something set by the user within a file's Jinja config() block or in a yaml file (including dbt_project.yml). Some of those configs sit inside the node.config object, waiting to be accessed within the Jinja context via config.get(); other configs will be copied over to node-level properties (tags today, description and meta coming soon).

This also gives us the opportunity to rationalize the ways that nodes and their configs are represented today in manifest.json:

  • There should be a separate ModelConfig that inherits from base NodeConfig.
  • Configs should only be set on the resource types for which they are relevant. E.g. full_refresh on models + seeds only.
  • If a config is empty/null, it should not be included in node.config. (This could trim down manifest.json.)
  • Adapter-specific configs with not-empty default values should be included in node.config. For simplicity, adapter configs apply to all nodes. (This would add some bulk to manifest.json.)
  • All official, documented configs (e.g. incremental_strategy, unique_key for models) should be specified in a python dataclass, and their default values stored in manifest.json accordingly. (Today, incremental_strategy default values are set in Jinja macros. It would be amazing, though perhaps more tricky than it's worth, to include the default incremental_strategy config on models that are materialized: incremental, and only those models.)
  • Resources that aren't persisted in the database (i.e. analyses) shouldn't have node configs like materialized/database/schema. (As of v0.20, tests now use the 'test' materialization, and --store-failures means they can have an associated database representation, so those configs can be relevant. Again, it would be amazing to set those configs if and only if store_failures is enabled.)
  • Big question: Right now, the top-level keys in the manifest for project resources are nodes, sources, macros, docs, exposures (honorable mentions: selectors, disabled). Should nodes be split up into models, tests, seeds, snapshots, analyses?

Describe alternatives you've considered

Leaving this in place, with its various inconsistencies and potential for confusion

@jtcohen6 jtcohen6 added enhancement New feature or request artifacts 1.0.0 Issues related to the 1.0.0 release of dbt labels Jul 11, 2021
@jtcohen6 jtcohen6 added this to the Oh-Twenty-One milestone Jul 11, 2021
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Jul 22, 2021

Add to list above:

  • To keep artifacts more stable in the long run, define artifact schemas explicitly and mapping to our internal objects, rather than forcing serialization on our internal objects

Next steps:

  • Talk to Metadata team
  • Split up laundry list above into discrete issues, then estimate

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label Jun 8, 2022
@github-actions
Copy link
Contributor

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest; add a comment to notify the maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
artifacts enhancement New feature or request stale Issues that have gone stale
Projects
None yet
Development

No branches or pull requests

1 participant