Sam delete bug fixes

samcli/commands/delete/delete_context.py

@@ -35,6 +35,7 @@
 
 
 class DeleteContext:
+    # TODO: Separate this context into 2 separate contexts guided and non-guided, just like deploy.
     def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str, no_prompts: bool):
         self.stack_name = stack_name
         self.region = region
@@ -57,9 +58,15 @@ def __enter__(self):
         self.parse_config_file()
         if not self.stack_name:
             LOG.debug("No stack-name input found")
-            self.stack_name = prompt(
-                click.style("\tEnter stack name you want to delete:", bold=True), type=click.STRING
-            )
+            if not self.no_prompts:
+                self.stack_name = prompt(
+                    click.style("\tEnter stack name you want to delete", bold=True), type=click.STRING
+                )
+            else:
+                raise click.BadOptionUsage(
+                    option_name="--stack-name",
+                    message="Missing option '--stack-name', provide a stack name that needs to be deleted.",
+                )
 
         self.init_clients()
         return self
@@ -94,9 +101,15 @@ def init_clients(self):
         Initialize all the clients being used by sam delete.
         """
         if not self.region:
-            session = boto3.Session()
-            region = session.region_name
-            self.region = region if region else "us-east-1"
+            if not self.no_prompts:
+                session = boto3.Session()
+                region = session.region_name
+                self.region = region if region else "us-east-1"
+            else:
+                raise click.BadOptionUsage(
+                    option_name="--region",
+                    message="Missing option '--region', region is required to run the non guided delete command.",
+                )
 
         if self.profile:
             Context.get_current_context().profile = self.profile
@@ -218,7 +231,6 @@ def delete_ecr_companion_stack(self):
             )
 
             retain_repos = self.ecr_repos_prompts(ecr_companion_stack_template)
-
             # Delete the repos created by ECR companion stack if not retained
             ecr_companion_stack_template.delete(retain_resources=retain_repos)
 
@@ -229,9 +241,11 @@ def delete_ecr_companion_stack(self):
                 self.cf_utils.delete_stack(stack_name=self.companion_stack_name)
                 self.cf_utils.wait_for_delete(stack_name=self.companion_stack_name)
                 LOG.debug("Deleted ECR Companion Stack: %s", self.companion_stack_name)
+
             except CfDeleteFailedStatusError:
                 LOG.debug("delete_stack resulted failed and so re-try with retain_resources")
                 self.cf_utils.delete_stack(stack_name=self.companion_stack_name, retain_resources=retain_repos)
+                self.cf_utils.wait_for_delete(stack_name=self.companion_stack_name)
 
     def delete(self):
         """
@@ -296,9 +310,11 @@ def delete(self):
             self.cf_utils.delete_stack(stack_name=self.stack_name)
             self.cf_utils.wait_for_delete(self.stack_name)
             LOG.debug("Deleted Cloudformation stack: %s", self.stack_name)
+
         except CfDeleteFailedStatusError:
             LOG.debug("delete_stack resulted failed and so re-try with retain_resources")
             self.cf_utils.delete_stack(stack_name=self.stack_name, retain_resources=retain_resources)
+            self.cf_utils.wait_for_delete(self.stack_name)
 
         # If s3_bucket information is not available, warn the user
         if not self.s3_bucket:

samcli/lib/delete/cf_utils.py

@@ -30,6 +30,10 @@ def has_stack(self, stack_name: str) -> bool:
                 return False
 
             stack = resp["Stacks"][0]
+            if stack["EnableTerminationProtection"]:
+                message = "Stack cannot be deleted while TerminationProtection is enabled."
+                raise DeleteFailedError(stack_name=stack_name, msg=message)
+
             # Note: Stacks with REVIEW_IN_PROGRESS can be deleted
             # using delete_stack but get_template does not return
             # the template_str for this stack restricting deletion of
@@ -53,11 +57,6 @@ def has_stack(self, stack_name: str) -> bool:
             LOG.error("Botocore Exception : %s", str(e))
             raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e
 
-        except Exception as e:
-            # We don't know anything about this exception. Don't handle
-            LOG.error("Unable to get stack details.", exc_info=e)
-            raise e
-
     def get_stack_template(self, stack_name: str, stage: str) -> Dict:
         """
         Return the Cloudformation template of the given stack_name

samcli/lib/package/ecr_uploader.py

@@ -133,8 +133,10 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str):
 
         except botocore.exceptions.ClientError as ex:
             # Handle Client errors such as RepositoryNotFoundException or InvalidParameterException
-            LOG.error("DeleteArtifactFailedError Exception : %s", str(ex))
-            raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex
+            if "RepositoryNotFoundException" not in str(ex):
+                LOG.debug("DeleteArtifactFailedError Exception : %s", str(ex))
+                raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex
+            LOG.debug("RepositoryNotFoundException : %s", str(ex))
 
     def delete_ecr_repository(self, physical_id: str):
         """

samcli/lib/package/packageable_resources.py

@@ -178,7 +178,11 @@ def get_property_value(self, resource_dict):
             return {"Bucket": None, "Key": None}
 
         resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict)
-        if resource_path:
+        # In the case where resource_path is pointing to an intrinsinc
+        # ref function, sam delete will delete the stack but skip the deletion of this
+        # artifact, as deletion of intrinsic ref function artifacts is not supported yet.
+        # TODO: Allow deletion of S3 artifacts with intrinsic ref functions.
+        if resource_path and isinstance(resource_path, str):
             return self.uploader.parse_s3_url(resource_path)
         return {"Bucket": None, "Key": None}
 
@@ -233,12 +237,14 @@ def delete(self, resource_id, resource_dict):
             return
 
         remote_path = resource_dict.get(self.PROPERTY_NAME, {}).get(self.EXPORT_PROPERTY_CODE_KEY)
-        if is_ecr_url(remote_path):
+        # In the case where remote_path is pointing to an intrinsinc
+        # ref function, sam delete will delete the stack but skip the deletion of this
+        # artifact, as deletion of intrinsic ref function artifacts is not supported yet.
+        # TODO: Allow deletion of ECR artifacts with intrinsic ref functions.
+        if isinstance(remote_path, str) and is_ecr_url(remote_path):
             self.uploader.delete_artifact(
                 image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME
             )
-        else:
-            raise ValueError("URL given to the parse method is not a valid ECR url {0}".format(remote_path))
 
 
 class ResourceImage(Resource):
@@ -288,13 +294,15 @@ def delete(self, resource_id, resource_dict):
         if resource_dict is None:
             return
 
-        remote_path = resource_dict[self.PROPERTY_NAME]
-        if is_ecr_url(remote_path):
+        remote_path = resource_dict.get(self.PROPERTY_NAME)
+        # In the case where remote_path is pointing to an intrinsinc
+        # ref function, sam delete will delete the stack but skip the deletion of this
+        # artifact, as deletion of intrinsic ref function artifacts is not supported yet.
+        # TODO: Allow deletion of ECR artifacts with intrinsic ref functions.
+        if isinstance(remote_path, str) and is_ecr_url(remote_path):
             self.uploader.delete_artifact(
                 image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME
             )
-        else:
-            raise ValueError("URL given to the parse method is not a valid ECR url {0}".format(remote_path))
 
 
 class ResourceWithS3UrlDict(ResourceZip):
@@ -350,7 +358,13 @@ def get_property_value(self, resource_dict):
         s3_bucket = resource_path.get(self.BUCKET_NAME_PROPERTY, None)
 
         key = resource_path.get(self.OBJECT_KEY_PROPERTY, None)
-        return {"Bucket": s3_bucket, "Key": key}
+        # In the case where resource_path is pointing to an intrinsinc
+        # ref function, sam delete will delete the stack but skip the deletion of this
+        # artifact, as deletion of intrinsic ref function artifacts is not supported yet.
+        # TODO: Allow deletion of S3 artifacts with intrinsic ref functions.
+        if isinstance(s3_bucket, str) and isinstance(key, str):
+            return {"Bucket": s3_bucket, "Key": key}
+        return {"Bucket": None, "Key": None}
 
 
 class ServerlessFunctionResource(ResourceZip):
@@ -535,7 +549,8 @@ def delete(self, resource_id, resource_dict):
             return
 
         repository_name = self.get_property_value(resource_dict)
-        if repository_name:
+        # TODO: Allow deletion of ECR Repositories with intrinsic ref functions.
+        if repository_name and isinstance(repository_name, str):
             self.uploader.delete_ecr_repository(physical_id=repository_name)
 
     def get_property_value(self, resource_dict):

samcli/lib/package/s3_uploader.py

@@ -189,7 +189,9 @@ def delete_prefix_artifacts(self):
             LOG.error("Bucket not specified")
             raise BucketNotSpecifiedError()
         if self.prefix:
-            response = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix)
+            # Note: list_objects_v2 api uses prefix to fetch the keys that begin with the prefix
+            # To restrict fetching files with exact prefix self.prefix, "/" is used below.
+            response = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix + "/")
             prefix_files = response.get("Contents", [])
             for obj in prefix_files:
                 self.delete_artifact(obj["Key"], True)

tests/integration/delete/delete_integ_base.py

@@ -1,11 +1,12 @@
 import os
+from pathlib import Path
 from unittest import TestCase
 
 
 class DeleteIntegBase(TestCase):
     @classmethod
     def setUpClass(cls):
-        pass
+        cls.delete_test_data_path = Path(__file__).resolve().parents[1].joinpath("testdata", "delete")
 
     def setUp(self):
         super().setUp()