add support for full_refresh in template_fields#177
add support for full_refresh in template_fields#177mvdh98 wants to merge 1 commit intotomasfarias:masterfrom
Conversation
|
@tomasfarias could you have a look at this one? :) |
|
@mvdh98 Thanks for the PR! I will do a more thorough review later, but for now I'll let CI run the tests. |
tomasfarias
left a comment
There was a problem hiding this comment.
Pretty good work! I have a few questions, but overall great, shouldn't take too much more work to get it merged.
| context = {"params": {"full_refresh": "True"}} | ||
| op.render_template_fields(context) | ||
| assert op.full_refresh == "True" |
There was a problem hiding this comment.
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)
| def test_table_mutability_config_full_refresh_string_coercion( | ||
| profiles_file, dbt_project_file | ||
| ): | ||
| """Test that full_refresh string values are coerced to booleans.""" |
There was a problem hiding this comment.
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.
| if isinstance(self.full_refresh, str): | ||
| self.full_refresh = self.full_refresh.lower() == "true" |
There was a problem hiding this comment.
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?
|
CI seems a bit flaky today, but the failures do not seem to be related to this PR. Retrying seems to help. |
|
@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! |
Summary
full_refreshtotemplate_fieldsonDbtRunOperator,DbtSeedOperator,DbtCompileOperator, andDbtBuildOperator, enabling Jinja templating in DAG definitions.TableMutabilityConfig.__post_init__to handle Airflow renderingfull_refreshas a string after template resolution.Example usage
Test plan
test_full_refresh_in_template_fieldsfor each of the 4 operatorstest_full_refresh_templatedfor each of the 4 operatorstest_table_mutability_config_full_refresh_string_coercionfor string-to-bool coercionDevelopment Approach