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 2021/12/01 05:35:19 UTC
[GitHub] [airflow] josh-fell opened a new pull request #19923: Fix mypy errors in Microsoft Azure provider
josh-fell opened a new pull request #19923:
URL: https://github.com/apache/airflow/pull/19923
Part of #19891
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760460955
##########
File path: tests/providers/microsoft/azure/hooks/test_azure_data_factory.py
##########
@@ -177,7 +177,7 @@ def test_create_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
implicit_factory=((MODEL,), (DEFAULT_RESOURCE_GROUP, DEFAULT_FACTORY, MODEL)),
)
def test_update_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
- hook._factory_exists = Mock(return_value=True)
+ hook._factory_exists = Mock(return_value=True) # type: ignore
Review comment:
A `patch.object(...)` context manager does the trick. WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760536517
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -134,7 +134,7 @@ def get_conn(self) -> BlobServiceClient:
app_id = conn.login
app_secret = conn.password
tenant = extra.get('tenant_id') or extra.get('extra__wasb__tenant_id')
- token_credential = ClientSecretCredential(tenant, app_id, app_secret)
+ token_credential = ClientSecretCredential(tenant, app_id, app_secret) # type: ignore
Review comment:
The error here was:
```
airflow/providers/microsoft/azure/hooks/wasb.py:137: error: Argument 1 to "ClientSecretCredential" has incompatible type "Optional[Any]"; expected "str"
token_credential = ClientSecretCredential(tenant, app_id, app_secret)
```
I'll address this properly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#issuecomment-983305438
CC @potiuk
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760460955
##########
File path: tests/providers/microsoft/azure/hooks/test_azure_data_factory.py
##########
@@ -177,7 +177,7 @@ def test_create_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
implicit_factory=((MODEL,), (DEFAULT_RESOURCE_GROUP, DEFAULT_FACTORY, MODEL)),
)
def test_update_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
- hook._factory_exists = Mock(return_value=True)
+ hook._factory_exists = Mock(return_value=True) # type: ignore
Review comment:
A `patch.object(...)` context manager does the trick. WDYT? Mypy still complained adding a `spec` unfortunately.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760514217
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -643,7 +642,7 @@ def wait_for_pipeline_run_status(
"factory_name": factory_name,
"resource_group_name": resource_group_name,
}
- pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
+ pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore
Review comment:
`pipeline_run_info: Dict[str, Any] = ` resolves the check as well but it's not _entirely_ a correct typing.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760450576
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -643,7 +642,7 @@ def wait_for_pipeline_run_status(
"factory_name": factory_name,
"resource_group_name": resource_group_name,
}
- pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
+ pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore
Review comment:
Oh I guess I could use a `TypedDict` 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760603706
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -142,10 +154,17 @@ def get_conn(self) -> DataFactoryManagementClient:
conn = self.get_connection(self.conn_id)
tenant = conn.extra_dejson.get('extra__azure_data_factory__tenantId')
- subscription_id = conn.extra_dejson.get('extra__azure_data_factory__subscriptionId')
- credential = None
+ try:
Review comment:
This is precisely why I think MyPy is useful! Getting " A Subscription ID is required to connect to Azure Data Factory." rather than "None error xxxx" is the whole world of difference for the users.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760139898
##########
File path: tests/providers/microsoft/azure/hooks/test_azure_data_factory.py
##########
@@ -177,7 +177,7 @@ def test_create_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
implicit_factory=((MODEL,), (DEFAULT_RESOURCE_GROUP, DEFAULT_FACTORY, MODEL)),
)
def test_update_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
- hook._factory_exists = Mock(return_value=True)
+ hook._factory_exists = Mock(return_value=True) # type: ignore
Review comment:
I think we should find a much better way to have mypy satisfied here.
I think we shoudl (if that works) add spec='boolean' to mock's constructor?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760606208
##########
File path: tests/providers/microsoft/azure/hooks/test_azure_data_factory.py
##########
@@ -177,8 +177,9 @@ def test_create_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
implicit_factory=((MODEL,), (DEFAULT_RESOURCE_GROUP, DEFAULT_FACTORY, MODEL)),
)
def test_update_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
- hook._factory_exists = Mock(return_value=True)
- hook.update_factory(*user_args)
+ with patch.object(hook, "_factory_exists") as mock_factory_exists:
Review comment:
Hmmm. I really like this change. Somehow using generic `Mock` object for this case felt a bit weird. Seems like MyPy also forces on us a bit nicer approach for mocking.
##########
File path: tests/providers/microsoft/azure/hooks/test_azure_data_factory.py
##########
@@ -177,8 +177,9 @@ def test_create_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
implicit_factory=((MODEL,), (DEFAULT_RESOURCE_GROUP, DEFAULT_FACTORY, MODEL)),
)
def test_update_factory(hook: AzureDataFactoryHook, user_args, sdk_args):
- hook._factory_exists = Mock(return_value=True)
- hook.update_factory(*user_args)
+ with patch.object(hook, "_factory_exists") as mock_factory_exists:
Review comment:
@uranusjr - WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r759890949
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -144,13 +144,12 @@ def get_conn(self) -> DataFactoryManagementClient:
tenant = conn.extra_dejson.get('extra__azure_data_factory__tenantId')
subscription_id = conn.extra_dejson.get('extra__azure_data_factory__subscriptionId')
- credential = None
if conn.login is not None and conn.password is not None:
credential = ClientSecretCredential(
- client_id=conn.login, client_secret=conn.password, tenant_id=tenant
+ client_id=conn.login, client_secret=conn.password, tenant_id=tenant # type: ignore
)
else:
- credential = DefaultAzureCredential()
+ credential = DefaultAzureCredential() # type: ignore
Review comment:
Add `credential: Any` before the `if` instead. These credential classes are opaque and internal to Azure, and we are passing them directly back to Azure. This is better than `# type: ignore` because it is more obvious what kind of errors we are trying to ignore.
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -660,7 +659,7 @@ def wait_for_pipeline_run_status(
# Wait to check the status of the pipeline run based on the ``check_interval`` configured.
time.sleep(check_interval)
- pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
+ pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore
Review comment:
Same as above.
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -643,7 +642,7 @@ def wait_for_pipeline_run_status(
"factory_name": factory_name,
"resource_group_name": resource_group_name,
}
- pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
+ pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore
Review comment:
This feels unnecessary. What is the error we’re trying to work around here?
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -134,7 +134,7 @@ def get_conn(self) -> BlobServiceClient:
app_id = conn.login
app_secret = conn.password
tenant = extra.get('tenant_id') or extra.get('extra__wasb__tenant_id')
- token_credential = ClientSecretCredential(tenant, app_id, app_secret)
+ token_credential = ClientSecretCredential(tenant, app_id, app_secret) # type: ignore
Review comment:
Same
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760445384
##########
File path: airflow/providers/microsoft/azure/hooks/data_factory.py
##########
@@ -643,7 +642,7 @@ def wait_for_pipeline_run_status(
"factory_name": factory_name,
"resource_group_name": resource_group_name,
}
- pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
+ pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore
Review comment:
```
airflow/providers/microsoft/azure/hooks/data_factory.py:671: error: Argument 1 to "get_pipeline_run_status" of "AzureDataFactoryHook" has incompatible type "**Dict[str, Optional[str]]"; expected
"str"
pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info)
```
If I enforce keywords arguments on the `get_pipeline_run_status()` function, mypy doesn't complain. Although, we could always explicitly pass a dictionary rather than using the `**kwargs` syntax.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19923:
URL: https://github.com/apache/airflow/pull/19923#discussion_r760602271
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -22,7 +22,7 @@
from azure.common import AzureHttpError
try:
- from functools import cached_property
+ from functools import cached_property # type: ignore[attr-defined]
Review comment:
Oh nice. I have to do the same in one of my PRs :).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #19923: Fix mypy errors in Microsoft Azure provider
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19923:
URL: https://github.com/apache/airflow/pull/19923
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org