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

Add PrefectDbtSettings to prefect-dbt #16834

Merged
merged 8 commits into from
Jan 24, 2025
Merged

Add PrefectDbtSettings to prefect-dbt #16834

merged 8 commits into from
Jan 24, 2025

Conversation

kevingrismore
Copy link
Contributor

This introduces PrefectDbtSettings, which automatically detects a common set of DBT_-prefixed env vars upon instantiation. Intended for use with PrefectDbtRunner.

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Copy link

codspeed-hq bot commented Jan 24, 2025

CodSpeed Performance Report

Merging #16834 will not alter performance

Comparing add-dbt-settings (58c426e) with main (ccaa3dc)

Summary

✅ 2 untouched benchmarks

@kevingrismore kevingrismore marked this pull request as ready for review January 24, 2025 19:53
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new settings class and discriminator look good! I have one question/comment about the new before validator.

Comment on lines 171 to 182
@field_validator("target_configs", mode="before")
@classmethod
def handle_target_configs(cls, v: Any) -> Any:
"""Handle target configs field aliasing during validation"""
if isinstance(v, dict):
if "schema_" in v:
v["schema"] = v.pop("schema_")
# Handle nested blocks
for value in v.values():
if isinstance(value, dict) and "schema_" in value:
value["schema"] = value.pop("schema_")
return v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validator feels like it should be on BaseTargetConfigs instead. I think you wouldn't have to worry about nested blocks in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@desertaxle i mentioned this here, maybe it should be a before model_validator instead of field_validator? bc the alias schema_ presents an issue with the field_validator I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, @zzstoatzz and I looked at this quite thoroughly, and it seems like we are inconsistent in our use of aliases when dumping Blocks. We agreed that the current validator approach is a good short term option and we'll work on adding additional validation aliases to schema_ fields to ensure we handle the inconsistency in Block dumping.

@kevingrismore kevingrismore merged commit d5279c5 into main Jan 24, 2025
50 checks passed
@kevingrismore kevingrismore deleted the add-dbt-settings branch January 24, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants