airflow
airflow copied to clipboard
Validate dbt `trigger_reason` field to be less than 255 characters
Validate and truncate trigger_reason
field if it is longer than the limit of 255 characters.
closes: #34676
Hey @josh-fell, would greatly appreciate your thoughts and suggestions on this!
Hi @dirrao, did you have a chance to look at this PR again? Would appreciate your comments!
Nice work here!
I do think though that this logic should live in the DbtCloudHook instead. Moving the logic to the hook would cover both scenarios of folks using the operator and the
DbtCloudHook.trigger_job_run()
method explicitly (as part of a TaskFlow function for example).
Thanks for your review @josh-fell, I have updated accordingly!
I have a question about testing. I have only implemented a test for the DbtCloudHook
, do you think we need to add a test to cover DbtCloudRunJobOperator
trigger reason explicitly? I think the test for DbtCloudHook
covers all the possible cases including operator and DbtCloudHook.trigger_job_run()
.
Nice work here! I do think though that this logic should live in the DbtCloudHook instead. Moving the logic to the hook would cover both scenarios of folks using the operator and the
DbtCloudHook.trigger_job_run()
method explicitly (as part of a TaskFlow function for example).Thanks for your review @josh-fell, I have updated accordingly!
I have a question about testing. I have only implemented a test for the
DbtCloudHook
, do you think we need to add a test to coverDbtCloudRunJobOperator
trigger reason explicitly? I think the test forDbtCloudHook
covers all the possible cases including operator andDbtCloudHook.trigger_job_run()
.
Agreed. Adding the test to DbtCloudHook
is perfectly fine! There is another test which confirms passing the trigger_reason
value from the operator to the hook already which is good enough.