Skip to content

Display appropriate message for trigger fire when 204 is returned #311

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Display appropriate message for trigger fire when 204 is returned #311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca51a84
Display appropriate message when an activation ID is not returned fro…
dubee Jun 4, 2018
3031300
Review refactor
dubee Jun 4, 2018
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
15 changes: 14 additions & 1 deletion commands/trigger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package commands
import (
"errors"
"fmt"
"net/http"

"github.com/apache/incubator-openwhisk-cli/wski18n"
"github.com/apache/incubator-openwhisk-client-go/whisk"
Expand Down Expand Up @@ -117,7 +118,8 @@ var triggerFireCmd = &cobra.Command{

// TODO get rid of these global modifiers
Client.Namespace = qualifiedName.GetNamespace()
trigResp, _, err := Client.Triggers.Fire(qualifiedName.GetEntityName(), parameters)

trigResp, resp, err := Client.Triggers.Fire(qualifiedName.GetEntityName(), parameters)
if err != nil {
whisk.Debug(whisk.DbgError, "Client.Triggers.Fire(%s, %#v) failed: %s\n", qualifiedName.GetEntityName(), parameters, err)
errStr := wski18n.T("Unable to fire trigger '{{.name}}': {{.err}}",
Expand All @@ -127,13 +129,24 @@ var triggerFireCmd = &cobra.Command{
return werr
}

if resp.StatusCode == http.StatusNoContent {
Copy link
Member

@rabbah rabbah Jun 11, 2018

Choose a reason for hiding this comment

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

I suggest starting to add go unit tests as well for such changes as it will vector toward better factoring of the code. For example a unit test that mocks an HTTP response and confirms returning the expected message and CLI code.

fmt.Fprintf(color.Output,
wski18n.T("trigger /{{.namespace}}/{{.name}} did not fire as it is not associated with an active rule\n",
Copy link
Member

@rabbah rabbah Jun 11, 2018

Choose a reason for hiding this comment

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

I suggest for all new message and to start transitioning these message ids to constants - see for example baa07f0.

map[string]interface{}{
"namespace": boldString(qualifiedName.GetNamespace()),
"name": boldString(qualifiedName.GetEntityName())}))

return nil
Copy link
Contributor

@mdeuser mdeuser Jun 4, 2018

Choose a reason for hiding this comment

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

up for discussion... is this situation considered an error (i.e. non-zero exit code, stderr)?

Copy link
Member Author

@dubee dubee Jun 11, 2018

Choose a reason for hiding this comment

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

@mdeuser, I believe we agreed to return a non-zero exit code here.

Copy link
Contributor

@mdeuser mdeuser Jun 12, 2018

Choose a reason for hiding this comment

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

@dubee yeah.. that's what you and i agreed upon. was looking if anyone had an opposite argument :-)

Copy link
Member

@rabbah rabbah Jun 12, 2018

Choose a reason for hiding this comment

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

which exit code would you return?

}

fmt.Fprintf(color.Output,
wski18n.T("{{.ok}} triggered /{{.namespace}}/{{.name}} with id {{.id}}\n",
map[string]interface{}{
"ok": color.GreenString("ok:"),
"namespace": boldString(qualifiedName.GetNamespace()),
"name": boldString(qualifiedName.GetEntityName()),
"id": boldString(trigResp.ActivationId)}))

return nil
},
}
Expand Down
24 changes: 18 additions & 6 deletions tests/src/test/scala/system/basic/WskCliBasicTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,12 @@ class WskCliBasicTests extends TestHelpers with WskTestHelpers {
}

val stdout = wsk.action.get(name).stdout
stdout should not include regex(""""key": "a"""")
stdout should not include regex(""""value": "A"""")
stdout should include regex (""""key": "b""")
stdout should include regex (""""value": "B"""")
stdout should include regex (""""publish": false""")
stdout should include regex (""""version": "0.0.2"""")
stdout should not include (""""key": "a"""")
stdout should not include (""""value": "A"""")
stdout should include (""""key": "b""")
stdout should include (""""value": "B"""")
stdout should include (""""publish": false""")
stdout should include (""""version": "0.0.2"""")
wsk.action.list().stdout should include(name)
}

Expand Down Expand Up @@ -749,6 +749,18 @@ class WskCliBasicTests extends TestHelpers with WskTestHelpers {
}
}

it should "display proper error when trigger is not associated with active rule" in withAssetCleaner(wskprops) {
val triggerName = withTimestamp("noRuleTrigger")

(wp, assetHelper) =>
assetHelper.withCleaner(wsk.trigger, triggerName) { (trigger, _) =>
trigger.create(triggerName)
}

wsk.trigger.fire(triggerName)
.stdout should include regex(s"trigger /.*/$triggerName did not fire as it is not associated with an active rule")
}

behavior of "Wsk Rule CLI"

it should "create rule, get rule, update rule and list rule" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Expand Down
4 changes: 4 additions & 0 deletions wski18n/resources/en_US.all.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,10 @@
"id": "{{.ok}} triggered /{{.namespace}}/{{.name}} with id {{.id}}\n",
"translation": "{{.ok}} triggered /{{.namespace}}/{{.name}} with id {{.id}}\n"
},
{
"id": "trigger /{{.namespace}}/{{.name}} did not fire as it is not associated with an active rule\n",
"translation": "trigger /{{.namespace}}/{{.name}} did not fire as it is not associated with an active rule\n"
},
{
"id": "create new trigger",
"translation": "create new trigger"
Expand Down