Skip to content

Support array result #516

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.

Sign up for GitHub

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

Closed
ningyougang wants to merge 3 commits into from
Closed

Support array result #516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
867039b
Support array result
ningyougang May 24, 2022
443499c
Fix review comment
ningyougang Aug 18, 2022
ae92c33
Fix does not return activationId
ningyougang Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions commands/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func invokeAction(
qualifiedName QualifiedName,
parameters interface{},
blocking bool,
result bool) (map[string]interface{}, error) {
result bool) (interface{}, error) {
// TODO remove all global modifiers
Client.Namespace = qualifiedName.GetNamespace()
res, _, err := Client.Actions.Invoke(
Expand All @@ -214,7 +214,7 @@ func printInvocationResponse(
qualifiedName QualifiedName,
blocking bool,
header bool,
result map[string]interface{},
result interface{},
err error) error {
if err == nil {
printInvocationMsg(qualifiedName, blocking, header, result, color.Output)
Expand All @@ -232,13 +232,13 @@ func printInvocationResponse(
func printFailedBlockingInvocationResponse(
qualifiedName QualifiedName,
header bool,
result map[string]interface{},
result interface{},
err error) error {
if isBlockingTimeout(err) {
printBlockingTimeoutMsg(
qualifiedName.GetNamespace(),
qualifiedName.GetEntityName(),
getValueFromJSONResponse(ACTIVATION_ID, result))
getValueFromResponse(ACTIVATION_ID, result))
return err
} else if isApplicationError(err) {
printInvocationMsg(
Expand Down Expand Up @@ -1169,7 +1169,7 @@ func printInvocationMsg(
qualifiedName QualifiedName,
blocking bool,
header bool,
response map[string]interface{},
response interface{},
outputStream io.Writer) {
if header {
fmt.Fprintf(
Expand All @@ -1180,7 +1180,7 @@ func printInvocationMsg(
"ok": color.GreenString("ok:"),
"namespace": boldString(qualifiedName.GetNamespace()),
"name": boldString(qualifiedName.GetEntityName()),
"id": boldString(getValueFromJSONResponse(ACTIVATION_ID, response)),
"id": boldString(getValueFromResponse(ACTIVATION_ID, response)),
}))
}

Expand Down
20 changes: 11 additions & 9 deletions commands/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,17 +604,19 @@ func getChildValueStrings(keyValueArr whisk.KeyValueArr, key string, childKey st
return res
}

func getValueFromJSONResponse(field string, response map[string]interface{}) interface{} {
var res interface{}

for key, value := range response {
if key == field {
res = value
break
func getValueFromResponse(field string, response interface{}) interface{} {
if result, ok := response.(map[string]interface{}); ok {
for key, value := range result {
if key == field {
return value
}
}
}

return res
if result, ok := response.([]interface{}); ok {
Copy link
Member

@dgrove-oss dgrove-oss Aug 10, 2022

Choose a reason for hiding this comment

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

Should we insist that field be "" if response is an array? I'm not sure exactly how this function is used, but it might be error prone to just silently return the array result when the caller asked for a specific field of what they must have been expecting to be a JSON object.

Copy link
Contributor Author

@ningyougang ningyougang Aug 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dgrove-oss yes, you right, need to return "" when it is array.

Copy link
Member

@style95 style95 Aug 22, 2022

Choose a reason for hiding this comment

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

@ningyougang
This method was used to print the invocation result.

openwhisk-cli/commands/action.go

Line 238 in 92c251f

printBlockingTimeoutMsg(

Do we need to print the array result itself for the array case?

Copy link
Contributor Author

@ningyougang ningyougang Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The result is nil due to it is blocking with timeout, will not print the array result

I tested in such case (btw, i stopped all invokers containers)
image

The result is nil
image

Finally is
image

If i added sleep 61s for that action and make all invokers up again, the result is
image

For normally hello-array action with this pr, the result is
image

Copy link
Contributor Author

@ningyougang ningyougang Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, i already cherry-pick this pr's commits to #517,
Because add-array-result pr: apache/openwhisk#5290 is merged before than this pr, so when build this pr and execute this pr's test cases, it would execute openwhisk core relative's add-array-result feature test cases. e.g. when invoke below test case
image
it invoke use CLIActivationsOperations to get result in this pr
image

When build openwhisk pr, it would use RestActivationsOperations to get the result

Copy link
Member

@style95 style95 Aug 22, 2022

Choose a reason for hiding this comment

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

ok, it seems now I get it.

Previously, it returns var res interface{} without any initialization, when it failed to find the field.
Is this an empty string in case it could not find anything?
Isn't it nil?

Copy link
Contributor Author

@ningyougang ningyougang Aug 22, 2022

Choose a reason for hiding this comment

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

Yes, it would return emtry string now if it failed to find the field

Copy link
Member

@style95 style95 Aug 23, 2022

Choose a reason for hiding this comment

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

Was it an empty string or nil?

Copy link
Contributor Author

@ningyougang ningyougang Aug 23, 2022

Choose a reason for hiding this comment

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

nil

return result
} else {
return ""
}
}

func logoText() string {
Expand Down