You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/08/12 03:39:56 UTC
[GitHub] [openwhisk] ningyougang opened a new pull request #4940: Do not delete previous annotation and support delete annotation via CLI
ningyougang opened a new pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
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 in this pr: https://github.com/apache/openwhisk-cli/pull/488
<!--- Provide a concise summary of your changes in the Title -->
## Description
<!--- Provide a detailed description of your changes. -->
<!--- Include details of what problem you are solving and how your changes are tested. -->
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
<!--- Select below all system components are affected by your change. -->
<!--- Enter an `x` in all applicable boxes. -->
- [ ] API
- [ ] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [x] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- [x] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [x] I reviewed the [style guides](https://github.com/apache/openwhisk/wiki/Contributing:-Git-guidelines#code-readiness) 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.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang closed pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang closed pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#discussion_r471996907
##########
File path: 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) =>
Review comment:
```suggestion
it should "delete the given annotations using delAnnotations" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
```
##########
File path: core/controller/src/main/resources/apiv1swagger.json
##########
@@ -1910,6 +1910,13 @@
},
"description": "annotations on the item"
},
+ "delAnnotations": {
Review comment:
I wish we can have a better name for this.
But for now, I have no good idea.
##########
File path: 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
Review comment:
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:
```suggestion
val newAnnotations = content.delAnnotations.map { annotationArray =>
annotationArray.foldRight(action.annotations)((a: String, b: Parameters) => b - a)
}.map( _ ++ content.annotations).getOrElse(content.annotations ++ action.annotations)
```
##########
File path: 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) {
Review comment:
```suggestion
it should "not delete existing annotations when updating action with new annotation" in withAssetCleaner(wskprops) {
```
##########
File path: core/controller/src/main/resources/apiv1swagger.json
##########
@@ -1910,6 +1910,13 @@
},
"description": "annotations on the item"
},
+ "delAnnotations": {
+ "type": "array",
+ "items": {
+ "type": "string"
+ },
+ "description": "del annotations on the item"
Review comment:
```suggestion
"description": "The list of annotations to be deleted from the item"
```
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 merged pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
style95 merged pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
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
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
But for my test case in this pr: `Wsk actions should update an action via passing delAnnotations`
I print the body 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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
There has one test cases failed: `Wsk actions should update an action via passing delAnnotations`,
I tried to find the root reason but failed, below is my steps
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
But for my test case in this pr: `Wsk actions should update an action via passing delAnnotations`
I print the body 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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang closed pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
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 print the body 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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
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
----------------------------------------------------------------
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
[GitHub] [openwhisk] rabbah commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
rabbah commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-770512056
That's great! Thank you.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang removed a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang removed a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-673210352
when do system Tests, report below error (other passed prs have no below error)
hm..
It seems have no relation with this pr.
Strange, doesn't know the reason then.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
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
But for my test case in this pr: `Wsk actions should update an action via passing delAnnotations`
I print the body 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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-672734096
Please ignore blow content(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
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-673210352
when do system Tests, report below error (other passed prs have no below error)
hm..
It seems have no relation with this pr.
Strange, doesn't know the reason then.
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
style95 commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-770483957
@rabbah That sounds good to me.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#discussion_r472056331
##########
File path: core/controller/src/main/resources/apiv1swagger.json
##########
@@ -1910,6 +1910,13 @@
},
"description": "annotations on the item"
},
+ "delAnnotations": {
+ "type": "array",
+ "items": {
+ "type": "string"
+ },
+ "description": "del annotations on the item"
Review comment:
Updated accordingly
##########
File path: 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) {
Review comment:
Updated accordingly
##########
File path: 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) =>
Review comment:
Updated accordingly
##########
File path: 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
Review comment:
Updated accordingly
----------------------------------------------------------------
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
[GitHub] [openwhisk] rabbah commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
rabbah commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-770429457
@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?
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang closed pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#discussion_r472016173
##########
File path: 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
Review comment:
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:
```suggestion
val newAnnotations = content.delAnnotations.map { annotationArray =>
annotationArray.foldRight(action.annotations)((a: String, b: Parameters) => b - a)
}.map( _ ++ content.annotations).getOrElse(action.annotations ++ content.annotations)
```
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang commented on pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#issuecomment-770494297
@rabbah ,oh, i forgot to support `parameters`, anyway, i will create a new pr to support it.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940#discussion_r472039595
##########
File path: core/controller/src/main/resources/apiv1swagger.json
##########
@@ -1910,6 +1910,13 @@
},
"description": "annotations on the item"
},
+ "delAnnotations": {
Review comment:
hm... regarding `better name`, i have no idea for this as well.
----------------------------------------------------------------
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
[GitHub] [openwhisk] ningyougang closed pull request #4940: Do not delete previous annotation and support delete annotation via CLI
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4940:
URL: https://github.com/apache/openwhisk/pull/4940
----------------------------------------------------------------
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