Added confirmation prompt for sky storage delete, and --yes flag to s…

…kip it (#2726)

* for issue 2708

* for issue 2708

* fixed tests

* formatting

* format

* format

* Update sky/cli.py

Co-authored-by: Doyoung Kim <[email protected]>

* cli changes

* format

* final touches to delete

* final changes

* additional changes made for more cases

* Update tests/test_smoke.py

Co-authored-by: Doyoung Kim <[email protected]>

* final changes

* formatting

* format

* Update sky/cli.py

Co-authored-by: Doyoung Kim <[email protected]>

* refactored

---------

Co-authored-by: Andrew Kim <[email protected]>
Co-authored-by: Doyoung Kim <[email protected]>
Co-authored-by: Andrew Kim <[email protected]>
Co-authored-by: Andrew Kim <[email protected]>

sky/cli.py

@@ -136,9 +136,6 @@ def _get_glob_storages(storages: List[str]) -> List[str]:
         glob_storage = global_user_state.get_glob_storage_name(storage_object)
         if len(glob_storage) == 0:
             click.echo(f'Storage {storage_object} not found.')
-        else:
-            plural = 's' if len(glob_storage) > 1 else ''
-            click.echo(f'Deleting {len(glob_storage)} storage object{plural}.')
         glob_storages.extend(glob_storage)
     return list(set(glob_storages))
 
@@ -3457,8 +3454,14 @@ def storage_ls(all: bool):
               is_flag=True,
               required=False,
               help='Delete all storage objects.')
+@click.option('--yes',
+              '-y',
+              default=False,
+              is_flag=True,
+              required=False,
+              help='Skip confirmation prompt.')
 @usage_lib.entrypoint
-def storage_delete(names: List[str], all: bool):  # pylint: disable=redefined-builtin
+def storage_delete(names: List[str], all: bool, yes: bool):  # pylint: disable=redefined-builtin
     """Delete storage objects.
 
     Examples:
@@ -3477,11 +3480,23 @@ def storage_delete(names: List[str], all: bool):  # pylint: disable=redefined-bu
     if sum([len(names) > 0, all]) != 1:
         raise click.UsageError('Either --all or a name must be specified.')
     if all:
-        click.echo('Deleting all storage objects.')
         storages = sky.storage_ls()
+        if not storages:
+            click.echo('No storage(s) to delete.')
+            return
         names = [s['name'] for s in storages]
     else:
         names = _get_glob_storages(names)
+    if names:
+        if not yes:
+            storage_names = ', '.join(names)
+            storage_str = 'storages' if len(names) > 1 else 'storage'
+            click.confirm(
+                f'Deleting {len(names)} {storage_str}: '
+                f'{storage_names}. Proceed?',
+                default=True,
+                abort=True,
+                show_default=True)
 
     subprocess_utils.run_in_parallel(sky.storage_delete, names)
 

tests/test_smoke.py

@@ -3062,7 +3062,7 @@ def test_new_bucket_creation_and_deletion(self, tmp_local_storage_obj,
 
         # Run sky storage delete to delete the storage object
         subprocess.check_output(
-            ['sky', 'storage', 'delete', tmp_local_storage_obj.name])
+            ['sky', 'storage', 'delete', tmp_local_storage_obj.name, '--yes'])
 
         # Run sky storage ls to check if storage object is deleted
         out = subprocess.check_output(['sky', 'storage', 'ls'])
@@ -3094,7 +3094,7 @@ def test_multiple_buckets_creation_and_deletion(
         assert all([item in out for item in storage_obj_name])
 
         # Run sky storage delete all to delete all storage objects
-        delete_cmd = ['sky', 'storage', 'delete']
+        delete_cmd = ['sky', 'storage', 'delete', '--yes']
         delete_cmd += storage_obj_name
         subprocess.check_output(delete_cmd)
 
@@ -3129,7 +3129,7 @@ def test_bucket_external_deletion(self, tmp_scratch_storage_obj,
 
         # Run sky storage delete to delete the storage object
         out = subprocess.check_output(
-            ['sky', 'storage', 'delete', tmp_scratch_storage_obj.name])
+            ['sky', 'storage', 'delete', tmp_scratch_storage_obj.name, '--yes'])
         # Make sure bucket was not created during deletion (see issue #1322)
         assert 'created' not in out.decode('utf-8').lower()
 
@@ -3147,8 +3147,9 @@ def test_bucket_bulk_deletion(self, store_type, tmp_bulk_del_storage_obj):
         # files and folders to a new bucket, then delete bucket.
         tmp_bulk_del_storage_obj.add_store(store_type)
 
-        subprocess.check_output(
-            ['sky', 'storage', 'delete', tmp_bulk_del_storage_obj.name])
+        subprocess.check_output([
+            'sky', 'storage', 'delete', tmp_bulk_del_storage_obj.name, '--yes'
+        ])
 
         output = subprocess.check_output(['sky', 'storage', 'ls'])
         assert tmp_bulk_del_storage_obj.name not in output.decode('utf-8')