Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run mypy in azure-keyvault-secrets CI #20507

Merged
merged 16 commits into from
Sep 8, 2021
1 change: 1 addition & 0 deletions eng/tox/mypy_hard_failure_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"azure-eventhub",
"azure-identity",
"azure-keyvault-administration",
"azure-keyvault-secrets",
"azure-servicebus",
"azure-ai-textanalytics",
"azure-ai-formrecognizer",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ class KeyVaultOperationPoller(LROPoller):
# pylint: disable=arguments-differ
def __init__(self, polling_method):
# type: (PollingMethod) -> None
super(KeyVaultOperationPoller, self).__init__(None, None, None, NoPolling())
super(KeyVaultOperationPoller, self).__init__(None, None, lambda *_: None, NoPolling())
self._polling_method = polling_method

# pylint: disable=arguments-differ
def result(self):
def result(self): # type: ignore
# type: () -> Any
"""Returns a representation of the final resource without waiting for the operation to complete.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ class KeyVaultOperationPoller(LROPoller):
# pylint: disable=arguments-differ
def __init__(self, polling_method):
# type: (PollingMethod) -> None
super(KeyVaultOperationPoller, self).__init__(None, None, None, NoPolling())
super(KeyVaultOperationPoller, self).__init__(None, None, lambda *_: None, NoPolling())
self._polling_method = polling_method

# pylint: disable=arguments-differ
def result(self):
def result(self): # type: ignore
# type: () -> Any
"""Returns a representation of the final resource without waiting for the operation to complete.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# ------------------------------------
from functools import partial
from azure.core.tracing.decorator import distributed_trace
from azure.core.polling import LROPoller

from ._models import KeyVaultSecret, DeletedSecret, SecretProperties
from ._shared import KeyVaultClientBase
Expand Down Expand Up @@ -294,7 +295,7 @@ def restore_secret_backup(self, backup, **kwargs):

@distributed_trace
def begin_delete_secret(self, name, **kwargs):
# type: (str, **Any) -> DeletedSecret
# type: (str, **Any) -> LROPoller
"""Delete all versions of a secret. Requires secrets/delete permission.

When this method returns Key Vault has begun deleting the secret. Deletion may take several seconds in a vault
Expand Down Expand Up @@ -416,7 +417,7 @@ def purge_deleted_secret(self, name, **kwargs):

@distributed_trace
def begin_recover_deleted_secret(self, name, **kwargs):
# type: (str, **Any) -> SecretProperties
# type: (str, **Any) -> LROPoller
"""Recover a deleted secret to its latest version. Possible only in a vault with soft-delete enabled.

If the vault does not have soft-delete enabled, :func:`begin_delete_secret` is permanent, and this method will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def __init__(self, attributes, vault_id, **kwargs):
# type: (_models.SecretAttributes, str, **Any) -> None
self._attributes = attributes
self._id = vault_id
self._vault_id = KeyVaultSecretIdentifier(vault_id)
self._vault_id = KeyVaultSecretIdentifier(vault_id) if vault_id else None
YalinLi0312 marked this conversation as resolved.
Show resolved Hide resolved
self._content_type = kwargs.get("content_type", None)
self._key_id = kwargs.get("key_id", None)
self._managed = kwargs.get("managed", None)
Expand All @@ -38,8 +38,8 @@ def _from_secret_bundle(cls, secret_bundle):
# type: (_models.SecretBundle) -> SecretProperties
"""Construct a SecretProperties from an autorest-generated SecretBundle"""
return cls(
secret_bundle.attributes,
secret_bundle.id,
secret_bundle.attributes, # type: ignore
secret_bundle.id, # type: ignore
content_type=secret_bundle.content_type,
key_id=secret_bundle.kid,
managed=secret_bundle.managed,
Expand All @@ -51,8 +51,8 @@ def _from_secret_item(cls, secret_item):
# type: (_models.SecretItem) -> SecretProperties
"""Construct a SecretProperties from an autorest-generated SecretItem"""
return cls(
secret_item.attributes,
secret_item.id,
secret_item.attributes, # type: ignore
secret_item.id, # type: ignore
content_type=secret_item.content_type,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like content_type, tags, and possibly other properties should technically be optional as well

Copy link
Member Author

@YalinLi0312 YalinLi0312 Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Here is ignoring the errors that expect attributes and id to be optional while we don't want to set them with None. Same in _from_secret_bundle().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True -- I just mean that I'm surprised mypy doesn't complain despite the content_type property on line 62, for example, not being listed as possibly None

managed=secret_item.managed,
tags=secret_item.tags,
Expand All @@ -69,10 +69,10 @@ def content_type(self):

@property
def id(self):
# type: () -> str
# type: () -> Optional[str]
YalinLi0312 marked this conversation as resolved.
Show resolved Hide resolved
"""The secret's id

:rtype: str
:rtype: optional[str]
YalinLi0312 marked this conversation as resolved.
Show resolved Hide resolved
"""
return self._id

Expand All @@ -87,55 +87,55 @@ def key_id(self):

@property
def enabled(self):
# type: () -> bool
# type: () -> Optional[bool]
"""Whether the secret is enabled for use

:rtype: bool
:rtype: optional[bool]
"""
return self._attributes.enabled
return self._attributes.enabled if self._attributes else None

@property
def not_before(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""The time before which the secret can not be used, in UTC

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._attributes.not_before
return self._attributes.not_before if self._attributes else None

@property
def expires_on(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""When the secret expires, in UTC

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._attributes.expires
return self._attributes.expires if self._attributes else None

@property
def created_on(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""When the secret was created, in UTC

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._attributes.created
return self._attributes.created if self._attributes else None

@property
def updated_on(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""When the secret was last updated, in UTC

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._attributes.updated
return self._attributes.updated if self._attributes else None

@property
def recoverable_days(self):
# type: () -> Optional[int]
"""The number of days the key is retained before being deleted from a soft-delete enabled Key Vault.

:rtype: int
:rtype: optional[int]
"""
# recoverable_days was added in 7.1-preview
if self._attributes and hasattr(self._attributes, "recoverable_days"):
Expand All @@ -144,39 +144,39 @@ def recoverable_days(self):

@property
def recovery_level(self):
# type: () -> str
# type: () -> Optional[str]
"""The vault's deletion recovery level for secrets

:rtype: str
:rtype: optional[str]
"""
return self._attributes.recovery_level
return self._attributes.recovery_level if self._attributes else None

@property
def vault_url(self):
# type: () -> str
# type: () -> Optional[str]
"""URL of the vault containing the secret

:rtype: str
:rtype: optional[str]
"""
return self._vault_id.vault_url
return self._vault_id.vault_url if self._vault_id else None

@property
def name(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's name

:rtype: str
:rtype: optional[str]
"""
return self._vault_id.name
return self._vault_id.name if self._vault_id else None

@property
def version(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's version

:rtype: str
:rtype: Optional[str]
"""
return self._vault_id.version
return self._vault_id.version if self._vault_id else None

@property
def tags(self):
Expand All @@ -191,7 +191,7 @@ class KeyVaultSecret(object):
"""All of a secret's properties, and its value."""

def __init__(self, properties, value):
# type: (SecretProperties, str) -> None
# type: (SecretProperties, Optional[str]) -> None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case where the type checking gets a little confusing. A SecretBundle can have a None value, which is why this Optional[str] type makes sense -- but also, SecretSetParameters requires a non-None value. We have to send over SecretSetParameters to create a secret in the first place, meaning that -- in theory -- any secret we get back and turn into a KeyVaultSecret should have a value for value. I don't know what to do about this yet, but I'm thinking aloud a bit about why this is awkward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does SecretBundle have a None value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't in any real scenario I've seen (and I doubt it ever would), but its type signature leaves the possibility open.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this introduces one question that happened several times in mypy ci:
when swagger defines a property to be optional while we think there's no chance to be None, should we ignore mypy error in this case?

self._properties = properties
self._value = value

Expand All @@ -210,19 +210,19 @@ def _from_secret_bundle(cls, secret_bundle):

@property
def name(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's name

:rtype: str
:rtype: optional[str]
"""
return self._properties.name

@property
def id(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's id

:rtype: str
:rtype: optional[str]
"""
return self._properties.id

Expand All @@ -237,10 +237,10 @@ def properties(self):

@property
def value(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's value

:rtype: str
:rtype: optional[str]
"""
return self._value

Expand Down Expand Up @@ -329,19 +329,19 @@ def _from_deleted_secret_item(cls, deleted_secret_item):

@property
def name(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's name

:rtype: str
:rtype: optional[str]
"""
return self._properties.name

@property
def id(self):
# type: () -> str
# type: () -> Optional[str]
"""The secret's id

:rtype: str
:rtype: optional[str]
"""
return self._properties.id

Expand All @@ -356,27 +356,27 @@ def properties(self):

@property
def deleted_date(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""When the secret was deleted, in UTC

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._deleted_date

@property
def recovery_id(self):
# type: () -> str
# type: () -> Optional[str]
"""An identifier used to recover the deleted secret. Returns ``None`` if soft-delete is disabled.

:rtype: str
:rtype: optional[str]
"""
return self._recovery_id

@property
def scheduled_purge_date(self):
# type: () -> datetime
# type: () -> Optional[datetime]
"""When the secret is scheduled to be purged, in UTC. Returns ``None`` if soft-delete is disabled.

:rtype: ~datetime.datetime
:rtype: optional[~datetime.datetime]
"""
return self._scheduled_purge_date
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ class KeyVaultOperationPoller(LROPoller):
# pylint: disable=arguments-differ
def __init__(self, polling_method):
# type: (PollingMethod) -> None
super(KeyVaultOperationPoller, self).__init__(None, None, None, NoPolling())
super(KeyVaultOperationPoller, self).__init__(None, None, lambda *_: None, NoPolling())
self._polling_method = polling_method

# pylint: disable=arguments-differ
def result(self):
def result(self): # type: ignore
# type: () -> Any
"""Returns a representation of the final resource without waiting for the operation to complete.

Expand Down
7 changes: 7 additions & 0 deletions sdk/keyvault/azure-keyvault-secrets/mypy.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[mypy]
python_version = 3.6
warn_unused_configs = True
ignore_missing_imports = True

[mypy-azure.keyvault.*._generated.*]
ignore_errors = True