ramitkataria commented on PR #72098: URL: https://github.com/apache/airflow/pull/72098#issuecomment-5432783455
Thanks for continuing this cleanup series. The fix itself looks correct. However, two bigger-picture thoughts, neither blocking for this PR. Since this is the 6th PR of this shape, it might be worth stepping back: 1. By my count, 43 defer sites in the provider still pass none of `region_name`/`verify`/`botocore_config` (bedrock, emr, dms, comprehend, glue, opensearch_serverless, several sensors) and 18 more pass only `region_name` (eks, rds, ecs, glue, batch). If the plan is to keep going service by service, a tracking issue with that list would help coordinate and avoid duplicate PRs. 2. Many of the remaining sites can't actually be fixed at the call site: their trigger subclasses have closed `__init__` signatures without `**kwargs`, and `hook()` implementations that pass only `aws_conn_id`. A default `hook()` on `AwsBaseWaiterTrigger` driven by a `hook_class` attribute (similar to `AwsBaseHookMixin._hook_parameters`), plus one parametrized invariant test over the deferrable operators, would fix this class of bug in one place and catch new operators that forget the kwargs Related small thing: the `REGION_NAME`/`VERIFY`/`BOTOCORE_CONFIG` test constant trio is now copy-pasted into 12 test files. A shared helper under `tests/unit/amazon/aws/utils/` (following the `validate_template_fields` precedent) would stop that growth for the remaining PRs in the series. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
