feat: add fail_project_under and fail_any_model_under options #58

WobblyRobbly · 2024-06-16T11:02:41Z

This pull request is for Issue #55

Decisions

fail_project_under and fail_any_model_under can be specified in the pyproject.toml or via command line
to avoid making this a breaking change a default value of 0.0 for fail_project_under and fail_any_model_under is set
the exit point for failing is in cli.py and that required the Evaluation instance to be returned from lint_dbt_project

Please let me know your thoughts of potential changes

Closes #55

druzhinin-kirill

Thanks for your contribution!
Left some tiny comments, already looks good 🚀

tests/test_cli.py

src/dbt_score/evaluation.py

src/dbt_score/cli.py

src/dbt_score/evaluation.py

src/dbt_score/config.py

jochemvandooren

Super nice feature 🚀, works very well. I've added some nitpicky comments, just to align with the style we have 😅

Looks good to me! Just missing an entry in the CHANGELOG, could you add it? 🙇‍♂️

docs/configuration.md

src/dbt_score/cli.py

druzhinin-kirill

pre-approve from my side 🚀

WobblyRobbly · 2024-06-19T11:27:05Z

@jochemvandooren as we've added a Breaking Change i.e. will fail if the default model score isn't met (5.0) this should bump the version to 1.0.0 based on ?

## [1.0.0] - 2024-06-19

- Add `project_fail_under` configuration 
- Add `fail_any_model_under` configuration
- **breaking:** - default values of `5.0` for `project_fail_under` and `fail_any_model_under` will cause command to exit return code 1

matthieucan · 2024-06-19T11:41:35Z

@jochemvandooren as we've added a Breaking Change i.e. will fail if the default model score isn't met (5.0) this should bump the version to 1.0.0 based on ?
## [1.0.0] - 2024-06-19

- Add `project_fail_under` configuration 
- Add `fail_any_model_under` configuration
- **breaking:** - default values of `5.0` for `project_fail_under` and `fail_any_model_under` will cause command to exit return code 1

We're following semantic versioning, meaning if the major version is 0, a breaking change will bump the minor version.
However no need for this to happen in the changelog file for this PR, this is done when releasing :)

matthieucan · 2024-06-19T11:45:23Z

src/dbt_score/cli.py

+        failed_models = {
+            model.unique_id: score.value
+            for model, score in evaluation.scores.items()
+            if (score.value < config.fail_any_model_under)
+        }
+
+        if failed_models:
+            print(f"Error: fail_any_model_under = {config.fail_any_model_under}")
+            for model, value in failed_models.items():
+                print(f"Model {model} scored {round(value,1)}")
+            ctx.exit(1)
+
+        if evaluation.project_score.value < config.fail_project_under:
+            print(f"Error: fail_project_under = {evaluation.project_score.value}")
+            ctx.exit(1)


I think it'd make sense to have this output done by src/dbt_score/formatters/human_readable_formatter.py. The Formatter class doesn't know about the config, but it could easily receive it upon instantiation in __init__. I'm happy to take care of this if you prefer - let me know.

@matthieucan I've pushed the other PR fixes but happy for you to fix this. I'm struggling for time this week

@WobblyRobbly Unfortunately it seems I can't push to this PR, despite the "Maintainers are allowed to edit this pull request" option. Is there some branch protection on your side?

@matthieucan - I've invited you as a collaborator ... let me know how that goes

Thanks, that worked!

jochemvandooren

Very nice, approved! 🙌 Two small comments, which I will leave up to one of you!

jochemvandooren · 2024-06-20T11:05:13Z

src/dbt_score/cli.py

    except FileNotFoundError:
        logger.error(
            "dbt's manifest.json could not be found. If you're in a dbt project, be "
            "sure to run 'dbt parse' first, or use the option '--run-dbt-parse'."
        )
        ctx.exit(2)

+    if (
+        any(x.value < config.fail_any_model_under for x in evaluation.scores.values())


Not a big fan of using x personally, score would be more readable IMO

That makes the line too long, and then spread over multiple lines I think we lose in readability

docs/configuration.md

matthieucan · 2024-06-20T12:42:14Z

@WobblyRobbly Thank you so much for your contribution! First external PR that we merged for this project 🚀 🥇
Looking forward to more of the same!

WobblyRobbly · 2024-06-20T12:43:13Z

Thanks for all your help on this one !!

feat: add fail_project_under and fail_any_model_under options

2ba139a

druzhinin-kirill reviewed Jun 17, 2024

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

src/dbt_score/evaluation.py Outdated Show resolved Hide resolved

src/dbt_score/cli.py Outdated Show resolved Hide resolved

Merge branch 'PicnicSupermarket:master' into master

66c9237

matthieucan reviewed Jun 17, 2024

View reviewed changes

src/dbt_score/evaluation.py Outdated Show resolved Hide resolved

src/dbt_score/config.py Outdated Show resolved Hide resolved

WobblyRobbly and others added 3 commits June 17, 2024 10:48

chore: address pull request comments

79026e6

chore: amend error messaging and display failed model list

f1631c0

Merge branch 'PicnicSupermarket:master' into master

80c8868

jochemvandooren reviewed Jun 19, 2024

View reviewed changes

docs/configuration.md Outdated Show resolved Hide resolved

docs/configuration.md Outdated Show resolved Hide resolved

src/dbt_score/cli.py Outdated Show resolved Hide resolved

src/dbt_score/cli.py Outdated Show resolved Hide resolved

druzhinin-kirill approved these changes Jun 19, 2024

View reviewed changes

matthieucan reviewed Jun 19, 2024

View reviewed changes

WobblyRobbly and others added 2 commits June 19, 2024 13:00

chore: addressed minor pr comments

5ee7797

Move model/project failure output to formatter

62234b6

matthieucan requested a review from jochemvandooren June 20, 2024 10:52

jochemvandooren approved these changes Jun 20, 2024

View reviewed changes

matthieucan added 2 commits June 20, 2024 14:35

Update docs

63a928c

typo

1d603a2

matthieucan enabled auto-merge (squash) June 20, 2024 12:41

matthieucan merged commit b95d691 into PicnicSupermarket:master Jun 20, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add fail_project_under and fail_any_model_under options #58

feat: add fail_project_under and fail_any_model_under options #58

WobblyRobbly commented Jun 16, 2024 •

edited

Loading

druzhinin-kirill left a comment

jochemvandooren left a comment

druzhinin-kirill left a comment

WobblyRobbly commented Jun 19, 2024

matthieucan commented Jun 19, 2024

matthieucan Jun 19, 2024

WobblyRobbly Jun 19, 2024

matthieucan Jun 20, 2024

WobblyRobbly Jun 20, 2024

matthieucan Jun 20, 2024

jochemvandooren left a comment •

edited

Loading

jochemvandooren Jun 20, 2024

matthieucan Jun 20, 2024

matthieucan commented Jun 20, 2024

WobblyRobbly commented Jun 20, 2024

feat: add fail_project_under and fail_any_model_under options #58

feat: add fail_project_under and fail_any_model_under options #58

Conversation

WobblyRobbly commented Jun 16, 2024 • edited Loading

druzhinin-kirill left a comment

Choose a reason for hiding this comment

jochemvandooren left a comment

Choose a reason for hiding this comment

druzhinin-kirill left a comment

Choose a reason for hiding this comment

WobblyRobbly commented Jun 19, 2024

matthieucan commented Jun 19, 2024

matthieucan Jun 19, 2024

Choose a reason for hiding this comment

WobblyRobbly Jun 19, 2024

Choose a reason for hiding this comment

matthieucan Jun 20, 2024

Choose a reason for hiding this comment

WobblyRobbly Jun 20, 2024

Choose a reason for hiding this comment

matthieucan Jun 20, 2024

Choose a reason for hiding this comment

jochemvandooren left a comment • edited Loading

Choose a reason for hiding this comment

jochemvandooren Jun 20, 2024

Choose a reason for hiding this comment

matthieucan Jun 20, 2024

Choose a reason for hiding this comment

matthieucan commented Jun 20, 2024

WobblyRobbly commented Jun 20, 2024

WobblyRobbly commented Jun 16, 2024 •

edited

Loading

jochemvandooren left a comment •

edited

Loading