mik-laj commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501974718
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -17,11 +17,13 @@
# under the License.
import os
import shutil
+from typing import Optional, Tuple, Dict
from azure.common import AzureHttpError
from cached_property import cached_property
from airflow.configuration import conf
+from airflow.models import TaskInstance
Review comment:
I am afraid that this will not create cyclical imports. We've had a
issue with this module before.
##########
File path:
airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -44,7 +44,7 @@
)
-DEFAULT_ENVIRONMENT_VARIABLES = {} # type: Dict[str, str]
+DEFAULT_ENVIRONMENT_VARIABLES = {} # type: dict
DEFAULT_SECURED_VARIABLES = [] # type: Sequence[str]
DEFAULT_VOLUMES = [] # type: Sequence[Volume]
Review comment:
```suggestion
DEFAULT_ENVIRONMENT_VARIABLES: Dict = {}
DEFAULT_SECURED_VARIABLES: Sequence[str] = []
DEFAULT_VOLUMES: Sequence[Volume] = []
```
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -17,11 +17,13 @@
# under the License.
import os
import shutil
+from typing import Optional, Tuple, Dict
from azure.common import AzureHttpError
from cached_property import cached_property
from airflow.configuration import conf
+from airflow.models import TaskInstance
Review comment:
I would prefer not to add this type here if we do not have system tests
that will detect regression.
See: https://github.com/apache/airflow/pull/4601
For other task handlers, we have system tests that can detect such a
problem, but this handler is not covered by these types of tests.
See:
https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/log/test_gcs_task_handler_system.py
https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/log/test_stackdriver_task_handler_system.py
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -17,11 +17,13 @@
# under the License.
import os
import shutil
+from typing import Optional, Tuple, Dict
from azure.common import AzureHttpError
from cached_property import cached_property
from airflow.configuration import conf
+from airflow.models import TaskInstance
Review comment:
I would prefer not to add this type here if we do not have system tests
that will detect regression.
See: https://github.com/apache/airflow/pull/4601
For other task handlers, we have system tests that can detect such a
problem, but this handler is not covered by this type of test.
See:
https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/log/test_gcs_task_handler_system.py
https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/log/test_stackdriver_task_handler_system.py
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
