You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/08 19:36:11 UTC
[GitHub] [airflow] mlgruby opened a new pull request #11359: Strict type check for Microsoft
mlgruby opened a new pull request #11359:
URL: https://github.com/apache/airflow/pull/11359
Strict type check for Microsoft provider. Part of: #9708
cc: @kaxil @mik-laj This completes provider Microsoft. Can I request for review?
96.914682 Microsoft =========> from 81 to 96
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705844048
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705858910
🤞
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501975160
##########
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] = []
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj closed pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj closed pull request #11359:
URL: https://github.com/apache/airflow/pull/11359
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r502021054
##########
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:
Oh okay, this will create a cyclic import! I was reading it as not create cyclic import so got confused.
Yup, that totally makes sense, for now, I will remove this and later I will create system test for this. Thanks for sharing the example.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-706040674
CI started ;-)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501991061
##########
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:
This will create cyclic import i.e. this module will call itself in a loop, causing errors
##########
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:
mainly because this is a log handler. I think that is what @mik-laj means
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
```suggestion
tags: Optional[Dict[str, str]] = None,
```
isn't `Dict[str, str] more appropriate here?
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
Haha man we still support Py 2.7 for Airflow 1.10.x, imagine :D
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501992071
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
```suggestion
tags: Optional[Dict[str, str]] = None,
```
isn't `Dict[str, str] more appropriate here?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #11359:
URL: https://github.com/apache/airflow/pull/11359
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705856240
@mlgruby Can you send an empty commit to your branch? For some reason the CI was not started for this change.
```
git commit --allow-empty --allow-empty-message -m ""
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501991061
##########
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:
This will create cyclic import i.e. this module will call itself in a loop, causing errors
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501989781
##########
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 sorry I didn't understand this!
##########
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:
👍🏼
##########
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:
Make sense 👍🏼
##########
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:
Oh okay, this will create a cyclic import! I was reading it as not create cyclic import so got confused.
Yup, that totally makes sense, for now, I will remove this and later I will create system test for this. Thanks for sharing the example.
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
it is, but I thought with python 3.9 we can have dict[str, str], hence kept it as dict only. Maybe I am thinking in future! :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705856240
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705909688
Retried once more, let's see
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705844048
[The Workflow run](https://github.com/apache/airflow/actions/runs/296320092) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r502021889
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
it is, but I thought with python 3.9 we can have dict[str, str], hence kept it as dict only. Maybe I am thinking in future! :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501989856
##########
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:
Make sense 👍🏼
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #11359:
URL: https://github.com/apache/airflow/pull/11359
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501994703
##########
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705872345
Nah not working!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r502027727
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -136,16 +136,16 @@ def __init__(
name: str,
image: str,
region: str,
- environment_variables: Optional[Dict[Any, Any]] = None,
+ environment_variables: Optional[dict] = None,
secured_variables: Optional[str] = None,
- volumes: Optional[List[Any]] = None,
+ volumes: Optional[list] = None,
memory_in_gb: Optional[Any] = None,
cpu: Optional[Any] = None,
gpu: Optional[Any] = None,
command: Optional[str] = None,
remove_on_error: bool = True,
fail_if_exists: bool = True,
- tags: Optional[Dict[str, str]] = None,
+ tags: Optional[dict] = None,
Review comment:
Haha man we still support Py 2.7 for Airflow 1.10.x, imagine :D
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705873592
CI for all PRs are broken.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705872345
Nah not working!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [airflow] mlgruby commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501989781
##########
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 sorry I didn't understand this!
##########
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:
👍🏼
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501991311
##########
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:
mainly because this is a log handler. I think that is what @mik-laj means
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#discussion_r501994703
##########
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:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705858910
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj closed pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
mik-laj closed pull request #11359:
URL: https://github.com/apache/airflow/pull/11359
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11359: Strict type check for Microsoft
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11359:
URL: https://github.com/apache/airflow/pull/11359#issuecomment-705844240
[The Workflow run](https://github.com/apache/airflow/actions/runs/296321619) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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:
users@infra.apache.org