Support node_color and docs.show under the config object

[b-per]

relates to #279
works in conjunction with dbt-labs/dbt-core#5397

I leveraged @sungchun12's logic and the modifications to dbt Core of dbt-labs/dbt-core#5397 to support defining docs.show and docs.node_color as node configs and have them update the docs accordingly.

With this approach docs.show can be defined in 2 different places. As docs.show, as it has been the case in the past versions (and kept like this to avoid breaking changes), and as config.docs.show when we define the docs as a config item.

The big advantage of the config item is that we can configure models at the folder level in dbt_project.yml

For now, there is no validation of node_color on the dbt-docs side but there is some validation on the dbt-core side.

The following config under dbt_project.yml generates the DAG below:

models:
  jaffle_shop:
      materialized: table
      +docs:
        node_color: criMsON
      staging:
        materialized: view
        +docs:
          node_color: "#e76f51"

[sungchun12]

I know we talked about +docs not being intuitive on a surface level for this config, but that's okay given current UX expects a person to do the same with the config: persist_docs

[sungchun12]

I like the if else default here

[sungchun12]

@b-per can you take a first draft of adding the defensive code: https://gist.github.com/stu-k/aac9d0fb7bf5e325f4fa8d260bd28fc8

based on the comment: dbt-labs/dbt-core#5397 (comment)

[b-per]

Yes, I will give it a shot 👍

[b-per]

I have added Stu's code on color validation and tested it.

With this new code, even thought the node_color is set as "node_color": "invalid_color", the docs do not crash anymore and the color is set to the default one. (example below with the model customers having an invalid code)

The question now is whether we would like to keep the validation on the dbt-docs side only or on both dbt-docs and dbt-core

[sungchun12]

@b-per thanks for much for progressing on this! I'll take a deeper look today/tomorrow. Per Stu's recommendation(dbt-labs/dbt-core#5397 (comment)) , we will keep the defensive code in one place for maintenance consistency. If this code is in two places, contributing to the dbt docs further will become more of a frankenstein than evolution.

NOT APPLICABLE ANYMORE

[jtcohen6]

Very supportive of the "defensive" color validation logic living in this codebase.

I have a very slight preference for accessing this metadata from node's top-level property always (node.docs), rather than reaching into node.config.docs — and that we should perform the coordination between those two fields within dbt-core (dbt-labs/dbt-core#5397 (comment)). I appreciate that's a bigger lift.

@dbt-labs/core-language very open to your thoughts here, as the team continues to develop its thinking around what should be a config and what should be a top-level property.

[sungchun12]

Doing some manual testing to check for edge cases. No red flags!

I noticed if you import a dbt package that people will have to color code that via code as well. You'll notice dbt_artifacts is left alone as expected.

What's working well?

• folder level color configs
• node-specific overrides folder level

[sungchun12]

TODO: Also, with docs behaving the way that jerco mentioned, we will need to update the dbt-docs code to look under model.docs instead of model.config.docs (not a huge change but just keeping it in mind)

[sungchun12]

@jtcohen6 any updates on whether we have the node_color under property vs. config hierarchy?

I have a draft in #288 separated out until we answer the above to then officially merge those changes into this PR.

This will require changes to dbt-labs/dbt-core#5397 based on the threads there.

I want to make sure we're aligned on approach before we commit code in the wrong design pattern!

[sungchun12]

I will own this to ensure the property is at the top-level vs. config level

[sungchun12]

@b-per Is this still expected? If I remove the 'config' portion in the list, the custom colors do NOT work.

[sungchun12]

Looks like this is expected based on this this line: here

This is ready for review!

[b-per]

I just did a test with the latest dbt-core and it works without looking at config. I think that we said we'd want to read from docs directly so I will push a small change.

[stu-k]

Looks good to me!

[emmyoop]

Looks good. Can you please add a changelog entry indicating the change and then it should be ready to merge!

[b-per]

I just modified the CHANGELOG 😄