[Storage] Add missing include-leased option for delete_snapshots= (#34042)

Author: vincenttran-msft

sdk/storage/azure-storage-file-share/CHANGELOG.md

@@ -14,6 +14,8 @@ pointing to the root of the file share would raise an `InvalidResourceName` on a
 Python 3.12.
 - Fixed an issue where authentication errors could raise `AttributeError` instead of `ClientAuthenticationError` when
 using async OAuth credentials.
+- Fixed an issue where parameter `delete_snapshots` to `delete_share` API did not support all possible enums. This change
+makes `delete_snapshots` now accept string literals 'include' and 'include-leased'.
 - Fixed an issue where specifying datetime objects with less than 7 digits of precision as input could incorrectly raise
 `InvalidHeaderValue` due to improper precision parsing.
 

sdk/storage/azure-storage-file-share/assets.json

@@ -2,5 +2,5 @@
   "AssetsRepo": "Azure/azure-sdk-assets",
   "AssetsRepoPrefixPath": "python",
   "TagPrefix": "python/storage/azure-storage-file-share",
-  "Tag": "python/storage/azure-storage-file-share_48979bc3d9"
+  "Tag": "python/storage/azure-storage-file-share_4de10ed98c"
 }

sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_client.py

@@ -5,9 +5,8 @@
 # --------------------------------------------------------------------------
 # pylint: disable=too-many-lines
 
-import sys
 from typing import (
-    Optional, Union, Dict, Any, Iterable, TYPE_CHECKING
+    Optional, Union, Dict, Any, Iterable, Literal, TYPE_CHECKING
 )
 from urllib.parse import urlparse, quote, unquote
 
@@ -34,10 +33,6 @@
 from ._lease import ShareLeaseClient
 from ._models import ShareProtocols
 
-if sys.version_info >= (3, 8):
-    from typing import Literal  # pylint: disable=no-name-in-module, ungrouped-imports
-else:
-    from typing_extensions import Literal  # pylint: disable=ungrouped-imports
 
 if TYPE_CHECKING:
     from azure.core.credentials import AzureNamedKeyCredential, AzureSasCredential, TokenCredential
@@ -454,15 +449,16 @@ def create_snapshot( # type: ignore
 
     @distributed_trace
     def delete_share(
-            self, delete_snapshots=False, # type: Optional[bool]
-            **kwargs
-        ):
-        # type: (...) -> None
+        self, delete_snapshots: Optional[Union[bool, Literal['include', 'include-leased']]] = False,
+        **kwargs: Any
+    ) -> None:
         """Marks the specified share for deletion. The share is
         later deleted during garbage collection.
 
-        :param bool delete_snapshots:
+        :param delete_snapshots:
             Indicates if snapshots are to be deleted.
+        :type delete_snapshots:
+            Optional[Union[bool, Literal['include', 'include-leased']]]
         :keyword lease:
             Required if the share has an active lease. Value can be a ShareLeaseClient object
             or the lease ID as a string.
@@ -489,8 +485,13 @@ def delete_share(
         access_conditions = get_access_conditions(kwargs.pop('lease', None))
         timeout = kwargs.pop('timeout', None)
         delete_include = None
-        if delete_snapshots:
-            delete_include = DeleteSnapshotsOptionType.include
+        if isinstance(delete_snapshots, bool) and delete_snapshots:
+            delete_include = DeleteSnapshotsOptionType.INCLUDE
+        else:
+            if delete_snapshots == 'include':
+                delete_include = DeleteSnapshotsOptionType.INCLUDE
+            elif delete_snapshots == 'include-leased':
+                delete_include = DeleteSnapshotsOptionType.INCLUDE_LEASED
         try:
             self._client.share.delete(
                 timeout=timeout,

sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_share_client_async.py

@@ -7,7 +7,7 @@
 import warnings
 import sys
 from typing import ( # pylint: disable=unused-import
-    Optional, Union, Dict, Any, Iterable, TYPE_CHECKING
+    Optional, Union, Dict, Any, Iterable, Literal, TYPE_CHECKING
 )
 
 from azure.core.exceptions import HttpResponseError
@@ -33,11 +33,6 @@
 from ..aio._lease_async import ShareLeaseClient
 from .._models import ShareProtocols
 
-if sys.version_info >= (3, 8):
-    from typing import Literal  # pylint: disable=no-name-in-module, ungrouped-imports
-else:
-    from typing_extensions import Literal  # pylint: disable=ungrouped-imports
-
 if TYPE_CHECKING:
     from azure.core.credentials import AzureNamedKeyCredential, AzureSasCredential
     from azure.core.credentials_async import AsyncTokenCredential
@@ -320,15 +315,16 @@ async def create_snapshot( # type: ignore
 
     @distributed_trace_async
     async def delete_share(
-            self, delete_snapshots=False, # type: Optional[bool]
-            **kwargs
-        ):
-        # type: (...) -> None
+        self, delete_snapshots: Optional[Union[bool, Literal['include', 'include-leased']]] = False,
+        **kwargs: Any
+        ) -> None:
         """Marks the specified share for deletion. The share is
         later deleted during garbage collection.
 
-        :param bool delete_snapshots:
+        :param delete_snapshots:
             Indicates if snapshots are to be deleted.
+        :type delete_snapshots:
+            Optional[Union[bool, Literal['include', 'include-leased']]]
         :keyword int timeout:
             Sets the server-side timeout for the operation in seconds. For more details see
             https://learn.microsoft.com/rest/api/storageservices/setting-timeouts-for-file-service-operations.
@@ -354,8 +350,13 @@ async def delete_share(
         access_conditions = get_access_conditions(kwargs.pop('lease', None))
         timeout = kwargs.pop('timeout', None)
         delete_include = None
-        if delete_snapshots:
-            delete_include = DeleteSnapshotsOptionType.include
+        if isinstance(delete_snapshots, bool) and delete_snapshots:
+            delete_include = DeleteSnapshotsOptionType.INCLUDE
+        else:
+            if delete_snapshots == 'include':
+                delete_include = DeleteSnapshotsOptionType.INCLUDE
+            elif delete_snapshots == 'include-leased':
+                delete_include = DeleteSnapshotsOptionType.INCLUDE_LEASED
         try:
             await self._client.share.delete(
                 timeout=timeout,

sdk/storage/azure-storage-file-share/tests/test_share.py

@@ -172,25 +172,6 @@ def test_create_snapshot_with_metadata(self, **kwargs):
         assert snapshot_props.metadata == metadata2
         self._delete_shares(share.share_name)
 
-    @FileSharePreparer()
-    @recorded_by_proxy
-    def test_delete_share_with_snapshots(self, **kwargs):
-        storage_account_name = kwargs.pop("storage_account_name")
-        storage_account_key = kwargs.pop("storage_account_key")
-
-        self._setup(storage_account_name, storage_account_key)
-        share = self._get_share_reference()
-        share.create_share()
-        snapshot = share.create_snapshot()
-
-        # Act
-        with pytest.raises(HttpResponseError):
-            share.delete_share()
-
-        deleted = share.delete_share(delete_snapshots=True)
-        assert deleted is None
-        self._delete_shares()
-
     @pytest.mark.playback_test_only
     @FileSharePreparer()
     @recorded_by_proxy
@@ -834,6 +815,37 @@ def test_list_shares_with_snapshot(self, **kwargs):
         share.delete_share(delete_snapshots=True)
         self._delete_shares()
 
+    @FileSharePreparer()
+    @recorded_by_proxy
+    def test_delete_snapshots_options(self, **kwargs):
+        storage_account_name = kwargs.pop("storage_account_name")
+        storage_account_key = kwargs.pop("storage_account_key")
+
+        self._setup(storage_account_name, storage_account_key)
+        share = self._create_share('prefix')
+        share.create_snapshot()
+        share.create_snapshot()
+
+        # Act / Assert
+
+        # Test backwards compatibility (False)
+        with pytest.raises(ResourceExistsError):
+            share.delete_share(delete_snapshots=False)
+
+        # Test backwards compatibility (True)
+        share.delete_share(delete_snapshots=True)
+
+        # Test "include"
+        share = self._create_share('prefix2')
+        share.create_snapshot()
+        share.delete_share(delete_snapshots='include')
+
+        # Test "include-leased"
+        share = self._create_share('prefix3')
+        lease = share.acquire_lease(lease_id='00000000-1111-2222-3333-444444444444')
+        share.create_snapshot()
+        share.delete_share(delete_snapshots='include-leased', lease='00000000-1111-2222-3333-444444444444')
+
     @FileSharePreparer()
     @recorded_by_proxy
     def test_list_shares_with_prefix(self, **kwargs):

sdk/storage/azure-storage-file-share/tests/test_share_async.py

@@ -158,26 +158,6 @@ async def test_create_snapshot_with_metadata(self, **kwargs):
         assert snapshot_props.metadata == metadata2
         await self._delete_shares(share.share_name)
 
-
-    @FileSharePreparer()
-    @recorded_by_proxy_async
-    async def test_delete_share_with_snapshots(self, **kwargs):
-        storage_account_name = kwargs.pop("storage_account_name")
-        storage_account_key = kwargs.pop("storage_account_key")
-
-        self._setup(storage_account_name, storage_account_key)
-        share = self._get_share_reference()
-        await share.create_share()
-        snapshot = await share.create_snapshot()
-
-        # Act
-        with pytest.raises(HttpResponseError):
-            await share.delete_share()
-
-        deleted = await share.delete_share(delete_snapshots=True)
-        assert deleted is None
-        await self._delete_shares(share.share_name)
-
     @pytest.mark.playback_test_only
     @FileSharePreparer()
     @recorded_by_proxy_async
@@ -837,6 +817,37 @@ async def test_list_shares_with_snapshot(self, **kwargs):
         self.assertNamedItemInContainer(all_shares, snapshot2['snapshot'])
         await self._delete_shares(share.share_name)
 
+    @FileSharePreparer()
+    @recorded_by_proxy_async
+    async def test_delete_snapshots_options(self, **kwargs):
+        storage_account_name = kwargs.pop("storage_account_name")
+        storage_account_key = kwargs.pop("storage_account_key")
+
+        self._setup(storage_account_name, storage_account_key)
+        share = await self._create_share('prefix')
+        await share.create_snapshot()
+        await share.create_snapshot()
+
+        # Act / Assert
+
+        # Test backwards compatibility (False)
+        with pytest.raises(ResourceExistsError):
+            await share.delete_share(delete_snapshots=False)
+
+        # Test backwards compatibility (True)
+        await share.delete_share(delete_snapshots=True)
+
+        # Test "include"
+        share = await self._create_share('prefix2')
+        await share.create_snapshot()
+        await share.delete_share(delete_snapshots='include')
+
+        # Test "include-leased"
+        share = await self._create_share('prefix3')
+        lease = await share.acquire_lease(lease_id='00000000-1111-2222-3333-444444444444')
+        await share.create_snapshot()
+        await share.delete_share(delete_snapshots='include-leased', lease='00000000-1111-2222-3333-444444444444')
+
     @FileSharePreparer()
     @recorded_by_proxy_async
     async def test_list_shares_with_prefix(self, **kwargs):