Do not delete previous annotation and support delete annotation via CLI (#4940)

* Do not delete previous annotation

Currently, if passing another annotations, original previous annotation
will be removed and the passed new annotations will be added.

It may give users some confused that why my previous annotation gone.
So it is better to not delete user's previous annotation when adding new
annotation, but at the same time, need to provide a feature that
support to delete annotation by user via ClI, e.g.
wsk action update hello --del-annotation key1 --del-annotation key2

CLI side needs to support as well

* Add test cases

* Fix some review comments

Co-authored-by: ning.yougang <[email protected]>

Author: ningyougang

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala

@@ -56,7 +56,8 @@ case class WhiskActionPut(exec: Option[Exec] = None,
                           limits: Option[ActionLimitsOption] = None,
                           version: Option[SemVer] = None,
                           publish: Option[Boolean] = None,
-                          annotations: Option[Parameters] = None) {
+                          annotations: Option[Parameters] = None,
+                          delAnnotations: Option[Array[String]] = None) {
 
   protected[core] def replace(exec: Exec) = {
     WhiskActionPut(Some(exec), parameters, limits, version, publish, annotations)
@@ -643,5 +644,5 @@ object ActionLimitsOption extends DefaultJsonProtocol {
 }
 
 object WhiskActionPut extends DefaultJsonProtocol {
-  implicit val serdes = jsonFormat6(WhiskActionPut.apply)
+  implicit val serdes = jsonFormat7(WhiskActionPut.apply)
 }

core/controller/src/main/resources/apiv1swagger.json

@@ -1910,6 +1910,13 @@
           },
           "description": "annotations on the item"
         },
+        "delAnnotations": {
+          "type": "array",
+          "items": {
+            "type": "string"
+          },
+          "description": "The list of annotations to be deleted from the item"
+        },
         "parameters": {
           "type": "array",
           "items": {

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

@@ -537,6 +537,13 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
 
     val exec = content.exec getOrElse action.exec
 
+    val newAnnotations = content.delAnnotations
+      .map { annotationArray =>
+        annotationArray.foldRight(action.annotations)((a: String, b: Parameters) => b - a)
+      }
+      .map(_ ++ content.annotations)
+      .getOrElse(action.annotations ++ content.annotations)
+
     WhiskAction(
       action.namespace,
       action.name,
@@ -545,7 +552,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
       limits,
       content.version getOrElse action.version.upPatch,
       content.publish getOrElse action.publish,
-      WhiskActionsApi.amendAnnotations(content.annotations getOrElse action.annotations, exec, create = false))
+      WhiskActionsApi.amendAnnotations(newAnnotations, exec, create = false))
       .revision[WhiskAction](action.docinfo.rev)
   }
 

tests/src/test/scala/common/WskCliOperations.scala

@@ -195,6 +195,7 @@ class CliActionOperations(override val wsk: RunCliCmd)
     docker: Option[String] = None,
     parameters: Map[String, JsValue] = Map.empty,
     annotations: Map[String, JsValue] = Map.empty,
+    delAnnotations: Array[String] = Array(),
     parameterFile: Option[String] = None,
     annotationFile: Option[String] = None,
     timeout: Option[Duration] = None,
@@ -229,6 +230,10 @@ class CliActionOperations(override val wsk: RunCliCmd)
       annotations flatMap { p =>
         Seq("-a", p._1, p._2.compactPrint)
       }
+    } ++ {
+      delAnnotations flatMap { p =>
+        Seq("--del-annotation", p)
+      }
     } ++ {
       parameterFile map { pf =>
         Seq("-P", pf)

tests/src/test/scala/common/WskOperations.scala

@@ -234,6 +234,7 @@ trait ActionOperations extends DeleteFromCollectionOperations with ListOrGetFrom
              docker: Option[String] = None,
              parameters: Map[String, JsValue] = Map.empty,
              annotations: Map[String, JsValue] = Map.empty,
+             delAnnotations: Array[String] = Array(),
              parameterFile: Option[String] = None,
              annotationFile: Option[String] = None,
              timeout: Option[Duration] = None,

tests/src/test/scala/common/rest/WskRestOperations.scala

@@ -264,6 +264,7 @@ class RestActionOperations(implicit val actorSystem: ActorSystem)
     docker: Option[String] = None,
     parameters: Map[String, JsValue] = Map.empty,
     annotations: Map[String, JsValue] = Map.empty,
+    delAnnotations: Array[String] = Array(),
     parameterFile: Option[String] = None,
     annotationFile: Option[String] = None,
     timeout: Option[Duration] = None,
@@ -364,6 +365,8 @@ class RestActionOperations(implicit val actorSystem: ActorSystem)
         content = content + ("annotations" -> annos.toJson)
       if (limits.nonEmpty)
         content = content + ("limits" -> limits.toJson)
+      if (delAnnotations.nonEmpty)
+        content = content + ("delAnnotations" -> delAnnotations.toJson)
       content
     }
 

tests/src/test/scala/org/apache/openwhisk/core/cli/test/WskRestBasicUsageTests.scala

@@ -131,6 +131,38 @@ class WskRestBasicUsageTests extends TestHelpers with WskTestHelpers with WskAct
       }
   }
 
+  it should "delete the given annotations using delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
+    val name = "hello"
+
+    assetHelper.withCleaner(wsk.action, name) { (action, _) =>
+      val annotations = Map("key1" -> "value1".toJson, "key2" -> "value2".toJson)
+      action.create(name, Some(TestUtils.getTestActionFilename("hello.js")), annotations = annotations)
+      val annotationString = wsk.parseJsonString(wsk.action.get(name).stdout).fields("annotations").toString
+
+      annotationString should include(""""key":"key1"""")
+      annotationString should include(""""value":"value1"""")
+      annotationString should include(""""key":"key2"""")
+      annotationString should include(""""value":"value2"""")
+
+      //Delete key1 only
+      val delAnnotations = Array("key1")
+
+      action.create(
+        name,
+        Some(TestUtils.getTestActionFilename("hello.js")),
+        delAnnotations = delAnnotations,
+        update = true)
+      val newAnnotationString = wsk.parseJsonString(wsk.action.get(name).stdout).fields("annotations").toString
+
+      newAnnotationString should not include (""""key":"key1"""")
+      newAnnotationString should not include (""""value":"value1"""")
+      newAnnotationString should include(""""key":"key2"""")
+      newAnnotationString should include(""""value":"value2"""")
+
+      action.create(name, Some(TestUtils.getTestActionFilename("hello.js")), update = true)
+    }
+  }
+
   it should "create, and get an action to verify file parameter and annotation parsing" in withAssetCleaner(wskprops) {
     (wp, assetHelper) =>
       val name = "actionAnnotAndParamParsing"

tests/src/test/scala/system/basic/WskActionTests.scala

@@ -357,4 +357,39 @@ class WskActionTests extends TestHelpers with WskTestHelpers with JsHelpers with
     }
   }
 
+  it should "not delete existing annotations when updating action with new annotation" in withAssetCleaner(wskprops) {
+    (wp, assetHelper) =>
+      val name = "hello"
+
+      assetHelper.withCleaner(wsk.action, name) { (action, _) =>
+        val annotations = Map("key1" -> "value1".toJson, "key2" -> "value2".toJson)
+        action.create(name, Some(TestUtils.getTestActionFilename("hello.js")), annotations = annotations)
+        val annotationString = wsk.parseJsonString(wsk.action.get(name).stdout).fields("annotations").toString
+
+        annotationString should include(""""key":"key1"""")
+        annotationString should include(""""value":"value1"""")
+        annotationString should include(""""key":"key2"""")
+        annotationString should include(""""value":"value2"""")
+
+        val newAnnotations = Map("key3" -> "value3".toJson, "key4" -> "value4".toJson)
+        action.create(
+          name,
+          Some(TestUtils.getTestActionFilename("hello.js")),
+          annotations = newAnnotations,
+          update = true)
+        val newAnnotationString = wsk.parseJsonString(wsk.action.get(name).stdout).fields("annotations").toString
+
+        newAnnotationString should include(""""key":"key1"""")
+        newAnnotationString should include(""""value":"value1"""")
+        newAnnotationString should include(""""key":"key2"""")
+        newAnnotationString should include(""""value":"value2"""")
+        newAnnotationString should include(""""key":"key3"""")
+        newAnnotationString should include(""""value":"value3"""")
+        newAnnotationString should include(""""key":"key4"""")
+        newAnnotationString should include(""""value":"value4"""")
+
+        action.create(name, Some(TestUtils.getTestActionFilename("hello.js")), update = true)
+      }
+  }
+
 }