Fix mypy errors in Microsoft Azure provider #19923
Conversation
boring-cyborg
bot
added
area:logging
area:providers
area:secrets
provider:microsoft-azure
|
CC @potiuk |
| 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 |
There was a problem hiding this 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.
| pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) | ||
| pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore |
There was a problem hiding this comment.
This feels unnecessary. What is the error we’re trying to work around here?
There was a problem hiding this 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.
There was a problem hiding this comment.
Oh I guess I could use a TypedDict here
There was a problem hiding this comment.
pipeline_run_info: Dict[str, Any] = resolves the check as well but it's not entirely a correct typing.
| pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) | ||
| pipeline_run_status = self.get_pipeline_run_status(**pipeline_run_info) # type: ignore |
| token_credential = ClientSecretCredential(tenant, app_id, app_secret) | ||
| token_credential = ClientSecretCredential(tenant, app_id, app_secret) # type: ignore |
There was a problem hiding this 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.
| @@ -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 | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
A patch.object(...) context manager does the trick. WDYT? Mypy still complained adding a spec unfortunately.
| @@ -22,7 +22,7 @@ | |||
| from azure.common import AzureHttpError | |||
|
|
|||
| try: | |||
| from functools import cached_property | |||
| from functools import cached_property # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
Oh nice. I have to do the same in one of my PRs :).
|
|
||
| credential = None | ||
| try: |
There was a problem hiding this 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.
| @@ -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: | |||
There was a problem hiding this 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.
The change apache#19923 introduced (accidentally) a required validation for an azure extra, which caused an error in the connection UI when the connection could not be added because the field was missing. This failed the test_crate_connection fail. This change removes back the validation.
The change #19923 introduced (accidentally) a required validation for an azure extra, which caused an error in the connection UI when the connection could not be added because the field was missing. This failed the test_crate_connection fail. This change removes back the validation.
Part of #19891
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.