Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] POEM: provide array result for all runtime images #5244

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

ningyougang
Copy link
Contributor

@ningyougang ningyougang commented May 24, 2022

Description

This is the POEM documentation for new feature providing support array result for all runtime images
I refer to this guide document for POEM: https://github.com/apache/openwhisk/blob/master/proposals/README.md

Related issue and scope

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

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Scheduler
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

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.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (c507069) to head (d4899a6).
Report is 110 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5244      +/-   ##
==========================================
- Coverage   79.97%   75.27%   -4.70%     
==========================================
  Files         238      238              
  Lines       14199    14199              
  Branches      577      577              
==========================================
- Hits        11355    10689     -666     
- Misses       2844     3510     +666     

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

@ningyougang
Copy link
Contributor Author

Hi,guys.
Regarding prvoide array result, for common action or sequence action, i have implemented in my local, e.g. for this example: https://github.com/apache/openwhisk/blob/master/docs/actions.md#creating-action-sequences

I updated the split.js and sort.js like below and invoked successully using my local openwhisk codes

function main(msg) {
   var separator = msg.separator || /\r?\n/;
   var payload = msg.payload.toString();
   var lines = payload.split(separator);
   return lines;
}

And

function main(msg) {
   var lines = msg || [];
   console.log('sort before: ' + lines);
   lines.sort();
   return lines;
}

-->
# Title

Currently, openwhisk supports return json object only, e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Currently, openwhisk supports return json object only, e.g.
Currently, OpenWhisk supports returning a JSON object only, e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

"greeting": "Hello stranger!"
}
```
It is necessary to support return array for common action, e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It is necessary to support return array for common action, e.g.
It is necessary to support returning an array too as an array is also a proper JSON object, e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

"b"
]
```
For sequence action, need to support as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For sequence action, need to support as well.
The sequence action should be considered as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly


# Summary and Motivation

This POEM proposes a new feature that allows user to write their own action which supports array result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This POEM proposes a new feature that allows user to write their own action which supports array result.
This POEM proposes a new feature that allows user to write their own action which supports an array result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

# Summary and Motivation

This POEM proposes a new feature that allows user to write their own action which supports array result.
So the result will support object and array both in future.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
So the result will support object and array both in future.
So actions would be able to return a JSON object or an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

Make controller and invoker support array result both.

## Runtime repos
All runtime images should support array result. e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
All runtime images should support array result. e.g.
All runtime images should support an array result. e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly


# Proposed changes
## Openwhisk main repo
Make controller and invoker support array result both.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Make controller and invoker support array result both.
Make controller and invoker support both a JSON object and an array result.

* ballerina

## Openwhisk-cli repo
* When use wsk to execute action, need to support parse array result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* When use wsk to execute action, need to support parse array result.
* The `wsk` CLI needs to support parsing an array result when executing actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly


## Openwhisk-cli repo
* When use wsk to execute action, need to support parse array result.
* When use wsk to get activation, need to support parse array result as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* When use wsk to get activation, need to support parse array result as well.
* The `wsk` CLI needs to support parsing an array result when getting activations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

@ningyougang ningyougang reopened this Jul 27, 2022
@ningyougang
Copy link
Contributor Author

Have any more comment?

@ningyougang ningyougang merged commit 3014b87 into apache:master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants