Do not delete previous annotation and support delete annotation via CLI by ningyougang · Pull Request #4940 · apache/openwhisk

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

          },
          "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

+          "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

    }
  }

+  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

      }
  }

+  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


    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.

codecov-commenter · 2026-04-04T01:58:00Z

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.02%. Comparing base (138ac18) to head (3557896).
⚠️ Report is 311 commits behind head on master.

Files with missing lines	Patch %	Lines
...org/apache/openwhisk/core/controller/Actions.scala	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
- Coverage   83.89%   77.02%   -6.87%     
==========================================
  Files         202      202              
  Lines        9653     9656       +3     
  Branches      404      414      +10     
==========================================
- Hits         8098     7438     -660     
- Misses       1555     2218     +663

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

	"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) =>


		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)

Conversation

ningyougang commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

ningyougang commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

style95 Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

rabbah commented Jan 31, 2021

Uh oh!

style95 commented Feb 1, 2021

Uh oh!

ningyougang commented Feb 1, 2021

Uh oh!

rabbah commented Feb 1, 2021

Uh oh!

codecov-commenter commented Apr 4, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ningyougang commented Aug 12, 2020 •

edited

Loading

ningyougang commented Aug 12, 2020 •

edited

Loading

style95 Aug 18, 2020 •

edited

Loading