airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Validate dbt `trigger_reason` field to be less than 255 characters

Open boraberke opened this issue 10 months ago • 2 comments

Validate and truncate trigger_reason field if it is longer than the limit of 255 characters.

closes: #34676

boraberke avatar Apr 10 '24 14:04 boraberke

Hey @josh-fell, would greatly appreciate your thoughts and suggestions on this!

boraberke avatar Apr 10 '24 14:04 boraberke

Hi @dirrao, did you have a chance to look at this PR again? Would appreciate your comments!

boraberke avatar Apr 25 '24 18:04 boraberke

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().

boraberke avatar May 05 '24 11:05 boraberke

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().

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.

josh-fell avatar May 06 '24 13:05 josh-fell