[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #4941: Force Delete Non-Empty Packages

[GitBox <gi...@apache.org>]

bdoyle0182 commented on a change in pull request #4941:
URL: https://github.com/apache/openwhisk/pull/4941#discussion_r471655208



##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -121,29 +122,42 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
    * - 500 Internal Server Error
    */
   override def remove(user: Identity, entityName: FullyQualifiedEntityName)(implicit transid: TransactionId) = {
-    deleteEntity(
-      WhiskPackage,
-      entityStore,
-      entityName.toDocId,
-      (wp: WhiskPackage) => {
-        wp.binding map {
-          // this is a binding, it is safe to remove
-          _ =>
-            Future.successful({})
-        } getOrElse {
-          // may only delete a package if all its ingredients are deleted already
-          WhiskAction
-            .listCollectionInNamespace(entityStore, wp.namespace.addPath(wp.name), skip = 0, limit = 0) flatMap {
-            case Left(list) if (list.size != 0) =>
-              Future failed {
-                RejectRequest(
-                  Conflict,
-                  s"Package not empty (contains ${list.size} ${if (list.size == 1) "entity" else "entities"})")
-              }
-            case _ => Future.successful({})
+    parameter('force ? false) { force =>
+      deleteEntity(
+        WhiskPackage,
+        entityStore,
+        entityName.toDocId,
+        (wp: WhiskPackage) => {
+          wp.binding map {
+            // this is a binding, it is safe to remove
+            _ =>
+              Future.successful({})
+          } getOrElse {
+            // may only delete a package if all its ingredients are deleted already or force flag is set
+            WhiskAction
+              .listCollectionInNamespace(entityStore, wp.namespace.addPath(wp.name), includeDocs = true, skip = 0, limit = 0) flatMap {
+              case Right(list) if list.nonEmpty =>
+                if (force) {
+                  Future sequence {
+                    list.map(action => {
+                      WhiskAction.get(entityStore, wp.fullyQualifiedName(false).add(action.fullyQualifiedName(false).name).toDocId) flatMap { actionWithRevision =>

Review comment:
       Yea good point. From what I can tell with other entity deletions, they all go through the `deleteEntity` function which will get the document first to get the revision so I think failure to get is the same as any other deletion.
   
   For failure to delete, some of the list of actions can fail to delete and the future sequence will fail if a single action fails to delete. And since the actions are deleted as a part of the confirmation phase of the package delete, the package won't delete unless they all succeed. I just need to go back and make sure when an action fails to delete it propagates the correct api failure or I recover here to transform it to the correct api exception. Should be clear to the client that if they get a failure for one of the actions failing to delete, they need to redo the request.
   
   For concurrent modifications I wasn't really thinking about. What's the consideration I should make there with this?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org