Skip to content

add support for full_refresh in template_fields#177

Open
mvdh98 wants to merge 1 commit intotomasfarias:masterfrom
mvdh98:feat-add-full-refresh-to-template-fields
Open

add support for full_refresh in template_fields#177
mvdh98 wants to merge 1 commit intotomasfarias:masterfrom
mvdh98:feat-add-full-refresh-to-template-fields

Conversation

@mvdh98
Copy link
Copy Markdown

@mvdh98 mvdh98 commented Mar 26, 2026

Summary

  • Adds full_refresh to template_fields on DbtRunOperator, DbtSeedOperator, DbtCompileOperator, and DbtBuildOperator, enabling Jinja templating in DAG definitions.
  • Adds string-to-bool coercion in TableMutabilityConfig.__post_init__ to handle Airflow rendering full_refresh as a string after template resolution.

Example usage

DbtRunOperator(                        
    task_id="dbt_run",                                                                                                                                                                                  
    full_refresh="{{ params.full_refresh }}",
)

Test plan

  • Added test_full_refresh_in_template_fields for each of the 4 operators
  • Added test_full_refresh_templated for each of the 4 operators
  • Added test_table_mutability_config_full_refresh_string_coercion for string-to-bool coercion
  • Verified existing tests still pass

Development Approach

  • First added the tests and verified they failed
  • Then made the code changes
  • Then verified the tests now succeed and any already existing tests still succeed

@mvdh98
Copy link
Copy Markdown
Author

mvdh98 commented Mar 26, 2026

@tomasfarias could you have a look at this one? :)

@tomasfarias
Copy link
Copy Markdown
Owner

@mvdh98 Thanks for the PR! I will do a more thorough review later, but for now I'll let CI run the tests.

Copy link
Copy Markdown
Owner

@tomasfarias tomasfarias left a comment

Choose a reason for hiding this comment

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

Pretty good work! I have a few questions, but overall great, shouldn't take too much more work to get it merged.

Comment on lines +348 to +350
context = {"params": {"full_refresh": "True"}}
op.render_template_fields(context)
assert op.full_refresh == "True"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

A bit surprising that the tests use a string here. I don't think there is necessarily any issue with it, but I'm curious why didn't you use a bool? (i.e. assert op.full_refresh == True)

Comment on lines +511 to +514
def test_table_mutability_config_full_refresh_string_coercion(
profiles_file, dbt_project_file
):
"""Test that full_refresh string values are coerced to booleans."""
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's a good test, but a tad repetitive, do you mind re-writing it using pytest.mark.parametrize? I can also push a quick commit for it.

Comment on lines +621 to +622
if isinstance(self.full_refresh, str):
self.full_refresh = self.full_refresh.lower() == "true"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I wonder if it is safer to raise an error if the value is a str different than true/false. With this implementation, any typos (e.g. "treu") will be resolved to full_refresh=False, which I guess is the safer option as full_refresh=True is more destructive. But I think we should at least raise a warning.

Any thoughts on this?

@tomasfarias
Copy link
Copy Markdown
Owner

CI seems a bit flaky today, but the failures do not seem to be related to this PR. Retrying seems to help.

@tomasfarias
Copy link
Copy Markdown
Owner

@mvdh98 One final question: May I ask for the motivation for this change? Just curious as to know in case there is more work for us to do here. Did you encounter a situation in which this feature would have been useful?

Thanks!

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.

2 participants