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

ningyougang · 2020-08-12T03:39:46Z

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

CLI side needs to support as well in this pr:

After all above 3 prs merged, supported

when update action with new annotation, the previous old annotation will not be deleted
if want to delete some annotations, user can execute below command via CLI
```
wsk action update $action -del-annotation key1  -del-annotation key2
```
support pass multiple keys in one line.

Description

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

ningyougang · 2020-08-12T08:34:42Z

Please ignore blow content due to already fixed

There has one test case run failed: Wsk actions should update an action via passing delAnnotations,

I tried to find the root reason but failed, below is my steps of find the reason

for delete annotation, in my local,
I executed below command using my debug enviroment

after click idea's Run, we can see it passed delAnnotation via body like below

And finally, it is successful.

But for my test case in this pr: Wsk actions should update an action via passing delAnnotations
I tried to print this line's body: https://github.com/apache/openwhisk/blob/master/tests/src/test/scala/common/rest/WskRestOperations.scala#L372
the body content i copied from my console, like below

It seems the param delAnnotations passed correctly.
But finally, the test case run failed

two key error log is:

TestFailedException: HTTP request or response did not match the Swagger spec.
Object instance has properties which are not allowed by the schema: ["delAnnotations"]: []

Someone konws the reason?
Already solved by added below content to apiv1swagger.json

style95 · 2020-08-18T07:54:34Z

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

@@ -1910,6 +1910,13 @@
          },
          "description": "annotations on the item"
        },
+        "delAnnotations": {


I wish we can have a better name for this.
But for now, I have no good idea.

hm... regarding better name, i have no idea for this as well.

style95 · 2020-08-18T07:57:13Z

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

+          "items": {
+            "type": "string"
+          },
+          "description": "del annotations on the item"


Suggested change

"description": "del annotations on the item"

"description": "The list of annotations to be deleted from the item"

Updated accordingly

style95 · 2020-08-18T08:09:46Z

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 previous annotation when update action with new annotation" in withAssetCleaner(wskprops) {


Suggested change

it should "not delete previous annotation when update action with new annotation" in withAssetCleaner(wskprops) {

it should "not delete existing annotations when updating action with new annotation" in withAssetCleaner(wskprops) {

Updated accordingly

style95 · 2020-08-18T08:12:01Z

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 "update an action via passing delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>


Suggested change

it should "update an action via passing delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

it should "delete the given annotations using delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

Updated accordingly

style95 · 2020-08-18T08:43:16Z

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

@@ -537,6 +537,14 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with

    val exec = content.exec getOrElse action.exec

+    var newAnnotations = action.annotations


I think we would not want to use the var variable.

Since the main target here is to remove items from action.annotations which are in the content.delAnnotations, I think we can do something similar to this:

Suggested change

var newAnnotations = action.annotations

val newAnnotations = content.delAnnotations.map { annotationArray =>

annotationArray.foldRight(action.annotations)((a: String, b: Parameters) => b - a)

}.map( _ ++ content.annotations).getOrElse(action.annotations ++ content.annotations)

Updated accordingly

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

style95

LGTM

rabbah · 2021-01-31T18:44:55Z

@ningyougang @style95 i'm curious why this was implemented for annotations only and not parameters (delete individual parameters). Should we extend the functionality to include parameters?

style95 · 2021-02-01T00:33:37Z

@rabbah That sounds good to me.

ningyougang · 2021-02-01T01:01:43Z

@rabbah ,oh, i forgot to support parameters, anyway, i will create a new pr to support it.

rabbah · 2021-02-01T01:54:46Z

That's great! Thank you.

ningyougang mentioned this pull request Aug 12, 2020

Support passing del annotation apache/openwhisk-cli#488

Merged

ningyougang force-pushed the not-delete-previous-annotation branch from 941ca44 to 37f979b Compare August 12, 2020 03:43

ningyougang mentioned this pull request Aug 12, 2020

Add DelAnnotations field to support del annotation apache/openwhisk-client-go#137

Merged

ningyougang force-pushed the not-delete-previous-annotation branch from 60334a2 to 24ee986 Compare August 12, 2020 08:21

ningyougang force-pushed the not-delete-previous-annotation branch from 24ee986 to b8e2bce Compare August 12, 2020 08:51

ningyougang closed this Aug 13, 2020

ningyougang reopened this Aug 13, 2020

ningyougang closed this Aug 13, 2020

ningyougang reopened this Aug 13, 2020

style95 reviewed Aug 18, 2020

View reviewed changes

ningyougang closed this Aug 19, 2020

ningyougang reopened this Aug 19, 2020

ningyougang closed this Aug 19, 2020

ningyougang reopened this Aug 19, 2020

ningyougang added 3 commits August 19, 2020 14:27

Add test cases

b14628c

Fix some review comments

3557896

ningyougang force-pushed the not-delete-previous-annotation branch from 5a5f527 to 3557896 Compare August 19, 2020 06:28

ningyougang closed this Aug 19, 2020

ningyougang reopened this Aug 19, 2020

style95 approved these changes Aug 21, 2020

View reviewed changes

style95 merged commit 64ccb22 into apache:master Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ningyougang commented Aug 12, 2020 •

edited

Loading

ningyougang commented Aug 12, 2020 •

edited

Loading

style95 Aug 18, 2020

ningyougang Aug 18, 2020

style95 Aug 18, 2020

ningyougang Aug 18, 2020

style95 Aug 18, 2020

ningyougang Aug 18, 2020

style95 Aug 18, 2020

ningyougang Aug 18, 2020

style95 Aug 18, 2020 •

edited

Loading

ningyougang Aug 18, 2020

style95 left a comment

rabbah commented Jan 31, 2021

style95 commented Feb 1, 2021

ningyougang commented Feb 1, 2021

rabbah commented Feb 1, 2021

	"description": "del annotations on the item"
	"description": "The list of annotations to be deleted from the item"

	it should "not delete previous annotation when update action with new annotation" in withAssetCleaner(wskprops) {
	it should "not delete existing annotations when updating action with new annotation" in withAssetCleaner(wskprops) {

	it should "update an action via passing delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
	it should "delete the given annotations using delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

		@@ -537,6 +537,14 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with

		val exec = content.exec getOrElse action.exec

		var newAnnotations = action.annotations

-    var newAnnotations = action.annotations
+    val newAnnotations = content.delAnnotations.map { annotationArray =>
+      annotationArray.foldRight(action.annotations)((a: String, b: Parameters) => b - a)
+    }.map( _ ++ content.annotations).getOrElse(action.annotations ++ content.annotations)

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

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

Conversation

ningyougang commented Aug 12, 2020 • edited Loading

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

ningyougang commented Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

style95 Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

style95 left a comment

Choose a reason for hiding this comment

rabbah commented Jan 31, 2021

style95 commented Feb 1, 2021

ningyougang commented Feb 1, 2021

rabbah commented Feb 1, 2021

ningyougang commented Aug 12, 2020 •

edited

Loading

ningyougang commented Aug 12, 2020 •

edited

Loading

style95 Aug 18, 2020 •

edited

Loading