eladkal commented on code in PR #39217: URL: https://github.com/apache/airflow/pull/39217#discussion_r1587844709
########## airflow/providers/teradata/provider.yaml: ########## @@ -33,6 +33,8 @@ dependencies: - apache-airflow-providers-common-sql>=1.3.1 - teradatasqlalchemy>=17.20.0.0 - teradatasql>=17.20.0.28 + - apache-airflow-providers-microsoft-azure + - apache-airflow-providers-amazon Review Comment: Setting this means that anyone who is using teradata provider will be forced to also have azure and amazon. I don't think it's right as these integrations are not part of the core functionality that this provider offer (unlike for example common-sql) I suggest to move them to optional dependencies like: https://github.com/apache/airflow/blob/1074b8eed680af9668f11c63cf28e72db5470fde/airflow/providers/google/provider.yaml#L167-L185 This will allow users of teradata to choose if they want to add the optional dependencies into their installation. You can also wrap the imports with AirflowOptionalProviderFeatureException to prevent cases where someone uses operator without having the underlying provider installed (similar to what I suggested in https://github.com/apache/airflow/pull/39366#discussion_r1587563547 ) -- 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]
