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 coverDbtCloudRunJobOperatortrigger reason explicitly? I think the test forDbtCloudHookcovers 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.