add 1.5 features - fct_public_models_without_contracts #339

dave-connors-3 · 2023-05-03T22:25:31Z

This is a:

bug fix PR with no breaking changes
new functionality

Link to Issue

This PR is in partial fulfillment of #332

Description & motivation

this PR adds:

access
contract.enforced
latest_version
group
number of yml cols
number of yml cols with description

for models and adds:

fct_public_models_without_contracts - outputs each model with access: public and config.contract.enforced: false
fct_exposures_dependent_on_private_models - exposure parent that is not a public model
fct_undocumented_public_models - outputs each undocumented public model

Integration Test Screenshot

Checklist

I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
- BigQuery
- Postgres
- Redshift
- Snowflake
- Databricks
- DuckDB
I have updated the README.md (if applicable)
I have added tests & descriptions to my models (and macros if applicable)

dave-connors-3 · 2023-05-04T14:07:39Z

we'll have to wait for dbx to release 1.5, but otherwise ready for review @graciegoheen @b-per !

b-per · 2023-05-05T14:14:35Z

Did you check the docs that get generated? I think that you will need to update the nav config because you added a new page.

b-per

A couple of changes required for the docs but except that it's all good for me

docs/index.md

docs/rules.md

Co-authored-by: Benoit Perigaud <[email protected]>

…t-project-evaluator into public-models-have-contracts

docs/rules/governance.md

graciegoheen · 2023-08-07T22:47:38Z

docs/rules/governance.md

+    description: very important OKR reporting model
+    access: public
+    config:
+      materialized: table


Are public models required to be materialized as a table? If so, let's add a note about that. If not, let's remove this from the example because it's a difference between the public model without a contract example

I don't think they are! removed

graciegoheen · 2023-08-07T22:50:23Z

docs/rules/governance.md

+
+## Undocumented public models
+
+`fct_undocumented_public_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/governance/fct_undocumented_public_models.sql)) shows each model with `access` configured as public that is not fully documented. This check is similar to `fct_undocumented_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/documentation/fct_undocumented_models.sql)), but is a stricter check that will highlight any public model that does not have a model-level description as well descriptions on each of its columns. 


Curious the reason to have both this model and fct_undocumented_models? Seems a bit redundant to have both, though this captures documenting at the column level so maybe it's ok for the bit of overlap?

I think it's to have a stricter requirement that is called out separately (better signal to noise ratio) and also have details at the column level as you note. It's more important for these models to be documented than it is for a random staging model!

graciegoheen · 2023-08-07T22:50:37Z

docs/rules/governance.md

+
+**Example**
+
+`report_1` is defined as a public model, but does not descriptions on the model and each column 


Suggested change

`report_1` is defined as a public model, but does not descriptions on the model and each column

`report_1` is defined as a public model, but does not have descriptions on the model and each column

graciegoheen · 2023-08-07T22:51:14Z

docs/rules/governance.md

+
+**How to Remediate**
+
+Edit the yml to include a model level description,  as well as a column entry with a description for all columns output by the model. While not strictly required for public models, these should likely also have contracts added as well. (See above rule)


Can you link to the "above" rule (and name it, in case at some point it's no longer "above")

graciegoheen · 2023-08-07T22:53:14Z

docs/rules/governance.md

+    access: public
+    config:
+      materialized: table
+      contract:


This example shows the fix as also adding a contract. I wonder if we could have 2 different examples for these 2 rules so it's clear what needs to be added to remediate this specific issue

yeah i note in the description that it's not required, but encouraged -- I can remove if we think it's too confusing

graciegoheen · 2023-08-07T22:54:27Z

docs/rules/governance.md

+
+## Exposures dependent on private models
+
+`fct_exposures_dependent_on_private_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/governance/fct_exposures_dependent_on_private_models.sql)) shows each relationship between a resource and an exposure where the parent resource is not a model with `access` configured as public.


Do we still need fct_exposure_parents_materializations or can these be combined?

it's a good question! this one is a bit stricter -- do we think folks not using governance features will get value out of this rule in the absence of the other?

docs/rules/governance.md

graciegoheen · 2023-08-07T22:55:35Z

docs/rules/governance.md

+
+**How to Remediate**
+
+Edit the yml to include fully expose the models that your exposures depend on. This should include `access`, a full model contract, and descriptions at all levels of the model. 


Again, what is the actual rule here? If it's just checking for access... should we also show adding contracts, descriptions, etc.

yeah, I updated this description to be more clear. I can remove the extra details if we think they are not helpful

graciegoheen · 2023-08-07T22:56:44Z

macros/recursive_dag.sql

@@ -31,6 +33,8 @@ all_relationships (
    child_resource_type,
    child_model_type,
    child_materialized,
+    child_access,
+    child_is_public,


Why the need for both access and is_public - isn't is_public just if access == 'public'?

I think just to allow for future rules that use access! can remove for now if we don't think it's valuable

Co-authored-by: Grace Goheen <[email protected]>

b-per

🥐

add 1.5 features to stg_nodes, fct_public_models_without_contracts, docs

1826e89

dave-connors-3 requested review from graciegoheen and b-per as code owners May 3, 2023 22:25

dave-connors-3 added 2 commits May 3, 2023 17:31

rename group to model_group to avoid protected keyword

bef8f0a

disable governance for dbx

0d7b96d

dave-connors-3 changed the title ~~add 1.5 features to stg_nodes, fct_public_models_without_contracts, docs~~ add 1.5 features - fct_public_models_without_contracts May 4, 2023

re-enable

49adb78

dave-connors-3 added 10 commits May 5, 2023 10:23

update nav

4b9e488

typo

51b09b5

add documentation tests for public models

fe4498c

docs for docs

e005773

exposures on non public models

c903f46

exposures on non public models - more files

3de4e66

resolve conflicts

33e4ea8

add column alias to integration test model

9d9f8b3

syntax issue in bq recursive dag

eca5a65

update readme to include governance category

38ec043

b-per reviewed Aug 7, 2023

View reviewed changes

docs/index.md Outdated Show resolved Hide resolved

docs/rules.md Show resolved Hide resolved

dave-connors-3 and others added 4 commits August 7, 2023 14:30

Update docs/index.md

e638da8

Co-authored-by: Benoit Perigaud <[email protected]>

docs updates

67f363c

Merge branch 'public-models-have-contracts' of github.com:dbt-labs/db…

4e738e9

…t-project-evaluator into public-models-have-contracts

merge conflicts

3ec0163

graciegoheen reviewed Aug 7, 2023

View reviewed changes

dave-connors-3 and others added 3 commits August 8, 2023 08:29

docs edits

4f7149e

Apply suggestions from code review

7ef76a1

Co-authored-by: Grace Goheen <[email protected]>

slim example docs to just show rule resolution

b15ebbc

b-per self-requested a review August 9, 2023 14:03

b-per approved these changes Aug 9, 2023

View reviewed changes

dave-connors-3 merged commit 2180997 into main Aug 9, 2023

dave-connors-3 deleted the public-models-have-contracts branch August 9, 2023 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add 1.5 features - fct_public_models_without_contracts #339

add 1.5 features - fct_public_models_without_contracts #339

dave-connors-3 commented May 3, 2023 •

edited

Loading

dave-connors-3 commented May 4, 2023

b-per commented May 5, 2023

b-per left a comment

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

graciegoheen Aug 7, 2023

dave-connors-3 Aug 8, 2023

b-per left a comment


		## Undocumented public models

		`fct_undocumented_public_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/governance/fct_undocumented_public_models.sql)) shows each model with `access` configured as public that is not fully documented. This check is similar to `fct_undocumented_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/documentation/fct_undocumented_models.sql)), but is a stricter check that will highlight any public model that does not have a model-level description as well descriptions on each of its columns.


		Example

		`report_1` is defined as a public model, but does not descriptions on the model and each column


		How to Remediate

		Edit the yml to include a model level description, as well as a column entry with a description for all columns output by the model. While not strictly required for public models, these should likely also have contracts added as well. (See above rule)


		## Exposures dependent on private models

		`fct_exposures_dependent_on_private_models` ([source](https://github.com/dbt-labs/dbt-project-evaluator/blob/main/models/marts/governance/fct_exposures_dependent_on_private_models.sql)) shows each relationship between a resource and an exposure where the parent resource is not a model with `access` configured as public.


		How to Remediate

		Edit the yml to include fully expose the models that your exposures depend on. This should include `access`, a full model contract, and descriptions at all levels of the model.

add 1.5 features - fct_public_models_without_contracts #339

add 1.5 features - fct_public_models_without_contracts #339

Conversation

dave-connors-3 commented May 3, 2023 • edited Loading

Link to Issue

Description & motivation

Integration Test Screenshot

Checklist

dave-connors-3 commented May 4, 2023

b-per commented May 5, 2023

b-per left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-per left a comment

Choose a reason for hiding this comment

dave-connors-3 commented May 3, 2023 •

edited

Loading