Backport dbt compatibilty bugfixes from the Soda Cloud dbt integration into this codebase #2423
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR backports code fixes from the Soda Cloud dbt integration into the core package, addressing various issues and inconsistencies between the two codebases.
- Updates the
DbtCheckCfgconstructor to makecolumn_nameoptional (nullable) - Enhances
DbtCheckto properly handle check values and use expressions in definitions - Refactors the dbt cloud integration with simplified data classes, improved error handling, and better artifact processing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| soda/dbt/soda/sodacl/dbt_check_cfg.py | Makes column_name parameter optional in DbtCheckCfg constructor |
| soda/dbt/soda/execution/check/dbt_check.py | Improves cloud diagnostics and definition handling in DbtCheck |
| soda/dbt/soda/cloud/dbt.py | Major refactoring with new data classes, simplified parsing, and enhanced error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| test_nodes is not None | ||
| ), "No test nodes were retrieved from the manifest.json. This could be because no tests have been implemented in dbt yet or you never ran `dbt test`." | ||
| assert test_nodes is not None, ( | ||
| "No test nodes found in manifest.json. This could be because no test was implemented" |
There was a problem hiding this comment.
Missing space in the error message. Should be 'no test was implemented in dbt' instead of 'no test was implementedin dbt'.
| "No test nodes found in manifest.json. This could be because no test was implemented" | |
| "No test nodes found in manifest.json. This could be because no test was implemented " |
Copilot uses AI. Check for mistakes.
| run_results: list[DbtRunResult], | ||
| ) -> dict[str, set[str]]: | ||
| assert test_nodes is not None, ( | ||
| "No test nodes found in manifest.json. This could be because no test was implemented" |
There was a problem hiding this comment.
Missing space in the error message. Should be 'no test was implemented in dbt' instead of 'no test was implementedin dbt'.
| "No test nodes found in manifest.json. This could be because no test was implemented" | |
| "No test nodes found in manifest.json. This could be because no test was implemented " |
Copilot uses AI. Check for mistakes.
| "In the meantime, if your jobs do not end on the above mentioned commands, you could make sure to add at least a `dbt test` " | ||
| "step as your last step and make sure that 'generate documentation' is not turned on in your job definition." | ||
| "In the meantime, if your jobs do not end on the above mentioned commands, you could make sure to add " | ||
| "at least a `dbt test`" |
There was a problem hiding this comment.
Missing space between 'dbt test' and 'step'. The concatenated string should read 'at least a dbt teststep' instead of 'at least adbt test`step'.
| "at least a `dbt test`" | |
| "at least a `dbt test` " |
Copilot uses AI. Check for mistakes.
sdebruyn
changed the title
It seems the code has some fixes in the cloud package which have not made their way to the core package. Relates to #2422 but does not fix it yet.