Handle namespace API listing on the client. #191

rabbah · 2018-01-09T22:11:58Z

Replaces call to GET /namespace/_ with calls to GET /namespace/_/[actions, triggers, rules, packages]

dubee · 2018-01-10T03:38:54Z

commands/namespace.go

-            whisk.Debug(whisk.DbgError, "Client.Namespaces.Get(%s) error: %s\n", getClientNamespace(), err)
-            errStr := wski18n.T("Unable to obtain the list of entities for namespace '{{.namespace}}': {{.err}}",
-                    map[string]interface{}{"namespace": getClientNamespace(), "err": err})
+        errHandler := func(err error, kind string) error {


Good idea! Maybe it's a good idea to put it in shared.go so other methods can utilize it. Plus it will serve as a template for debug/error handling to reduce code duplication in similar circumstances.

dubee · 2018-01-10T03:42:11Z

commands/namespace.go

-            errStr := wski18n.T("Unable to obtain the list of entities for namespace '{{.namespace}}': {{.err}}",
-                    map[string]interface{}{"namespace": getClientNamespace(), "err": err})
+        errHandler := func(err error, kind string) error {
+            whisk.Debug(whisk.DbgError, "Client.%s.Get(%s) error: %s\n", getClientNamespace(), kind, err)


Should be Client.%s.List(%s) instead of Client.%s.Get(%s).

dubee

Few test failures:

whisk.core.cli.test.WskConfigTests > Wsk CLI config should reject authenticated command when no auth key is given �[31mFAILED�[39m
    org.scalatest.exceptions.TestFailedException: "error: Unable to obtain the list of %s for namespace '{{.namespace}}': {{.err}}
    Run 'wsk --help' for usage.
    " did not include substring "--auth is required"
        at org.scalatest.MatchersHelper$.indicateFailure(MatchersHelper.scala:340)
        at org.scalatest.Matchers$ShouldMethodHelper$.shouldMatcher(Matchers.scala:6668)
        at org.scalatest.Matchers$AnyShouldWrapper.should(Matchers.scala:6704)
        at whisk.core.cli.test.WskConfigTests$$anonfun$4.apply(WskConfigTests.scala:103)
        at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)
        at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
        at org.scalatest.Transformer.apply(Transformer.scala:22)
        at org.scalatest.Transformer.apply(Transformer.scala:20)
        at org.scalatest.FlatSpecLike$$anon$1.apply(FlatSpecLike.scala:1682)
        at org.scalatest.TestSuite$class.withFixture(TestSuite.scala:196)
        at org.scalatest.FlatSpec.withFixture(FlatSpec.scala:1685)
        at org.scalatest.FlatSpecLike$class.invokeWithFixture$1(FlatSpecLike.scala:1679)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.SuperEngine.runTestImpl(Engine.scala:289)
        at org.scalatest.FlatSpecLike$class.runTest(FlatSpecLike.scala:1692)
        at whisk.core.cli.test.WskConfigTests.org$scalatest$BeforeAndAfterEachTestData$$super$runTest(WskConfigTests.scala:36)
        at 

org.scalatest.BeforeAndAfterEachTestData$class.runTest(BeforeAndAfterEachTestData.scala:194)
        at whisk.core.cli.test.WskConfigTests.runTest(WskConfigTests.scala:36)
 system.basic.WskBasicTests > Wsk CLI should reject unauthenticated access �[31mFAILED�[39m�[0K
    org.scalatest.exceptions.TestFailedException: "error: Unable to obtain the list of %s for namespace '{{.namespace}}': {{.err}}
    " did not include substring "The supplied authentication is invalid"
        at org.scalatest.MatchersHelper$.indicateFailure(MatchersHelper.scala:340)
        at org.scalatest.Matchers$ShouldMethodHelper$.shouldMatcher(Matchers.scala:6668)
        at org.scalatest.Matchers$AnyShouldWrapper.should(Matchers.scala:6704)
        at system.basic.WskBasicTests$$anonfun$3.apply(WskBasicTests.scala:72)
        at system.basic.WskBasicTests$$anonfun$3.apply(WskBasicTests.scala:68)
        at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)
        at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
        at org.scalatest.Transformer.apply(Transformer.scala:22)
        at org.scalatest.Transformer.apply(Transformer.scala:20)
        at org.scalatest.FlatSpecLike$$anon$1.apply(FlatSpecLike.scala:1682)
        at org.scalatest.TestSuite$class.withFixture(TestSuite.scala:196)
        at org.scalatest.FlatSpec.withFixture(FlatSpec.scala:1685)
        at org.scalatest.FlatSpecLike$class.invokeWithFixture$1(FlatSpecLike.scala:1679)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.SuperEngine.runTestImpl(Engine.scala:289)
        at org.scalatest.FlatSpecLike$class.runTest(FlatSpecLike.scala:1692)
        at system.basic.WskBasicTests.org$scalatest$BeforeAndAfterEachTestData$$super$runTest(WskBasicTests.scala:42)
        at org.scalatest.BeforeAndAfterEachTestData$class.runTest(BeforeAndAfterEachTestData.scala:194)
        at system.basic.WskBasicTests.runTest(WskBasicTests.scala:42)


 system.basic.WskBasicTests > Wsk Namespace CLI should not list entities with an invalid namespace �[0K�[31mFAILED�[39m�[0K
    org.scalatest.exceptions.TestFailedException: /home/travis/gopath/src/github.com/apache/incubator-openwhisk/bin/wsk -i --apihost 172.17.0.1 --apiversion v1 namespace get /fakeNamespace --auth 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP
    Entities in namespace: default
    packages
    actions
    triggers
    rules

    exit code: 0 was not equal to 147
        at org.scalatest.MatchersHelper$.indicateFailure(MatchersHelper.scala:340)
        at org.scalatest.Matchers$AnyShouldWrapper.shouldBe(Matchers.scala:6864)
        at common.RunWskCmd$$anonfun$cli$1.apply(Wsk.scala:1046)
        at org.scalatest.Assertions$class.withClue(Assertions.scala:1221)
        at common.WskNamespace.withClue(Wsk.scala:714)
        at common.RunWskCmd$class.cli(Wsk.scala:1042)
        at common.WskNamespace.cli(Wsk.scala:714)
        at common.WskNamespace.get(Wsk.scala:761)
        at system.basic.WskBasicTests$$anonfun$56.apply(WskBasicTests.scala:786)
        at system.basic.WskBasicTests$$anonfun$56.apply(WskBasicTests.scala:784)
        at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)
        at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
        at org.scalatest.Transformer.apply(Transformer.scala:22)
        at org.scalatest.Transformer.apply(Transformer.scala:20)
        at org.scalatest.FlatSpecLike$$anon$1.apply(FlatSpecLike.scala:1682)
        at org.scalatest.TestSuite$class.withFixture(TestSuite.scala:196)
        at org.scalatest.FlatSpec.withFixture(FlatSpec.scala:1685)
        at org.scalatest.FlatSpecLike$class.invokeWithFixture$1(FlatSpecLike.scala:1679)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.FlatSpecLike$$anonfun$runTest$1.apply(FlatSpecLike.scala:1692)
        at org.scalatest.SuperEngine.runTestImpl(Engine.scala:289)
        at org.scalatest.FlatSpecLike$class.runTest(FlatSpecLike.scala:1692)
        at system.basic.WskBasicTests.org$scalatest$BeforeAndAfterEachTestData$$super$runTest(WskBasicTests.scala:42)
        at org.scalatest.BeforeAndAfterEachTestData$class.runTest(BeforeAndAfterEachTestData.scala:194)
        at system.basic.WskBasicTests.runTest(WskBasicTests.scala:42)

rabbah · 2018-01-10T04:11:28Z

Thanks - two of these were easy to fix, I reverted the error message.

For this test

it should "not list entities with an invalid namespace" in {
    val namespace = "fakeNamespace"
    val stderr = wsk.namespace.get(Some(s"/${namespace}"), expectedExitCode = FORBIDDEN).stderr

    stderr should include(s"Unable to obtain the list of entities for namespace '${namespace}'")
  }

Why does the CLI even accept a namespace positional argument here? This is independent of the PR FWIW. Should this behavior be disallowed?

The action/trigger/rule/package list helpers don't accept a namespace. I'm not sure how to preserve this behavior which seems broken to me already.

rabbah · 2018-01-10T04:12:28Z

@dubeejw what change are you requesting? Moving the errHandler function? I can move it to shared.go but it seems useless to do that without then also refactoring all the error handlers to use it. Maybe I misunderstood.

dubee · 2018-01-10T04:19:19Z

@rabbah, Get to List is the only thing that really needed to be changed.

dubee · 2018-01-10T04:20:21Z

Moving the error handler would be nice. Refactoring the functions to use it can be done later.

dubee · 2018-01-10T04:26:28Z

Seems like we can remove the namespace get test, and update the command usage and parameter expectations (https://github.com/dubeejw/incubator-openwhisk-cli/blob/master/commands/namespace.go#L64).

@mdeuser, any input on the above?

mdeuser · 2018-01-10T13:26:58Z

the original python based cli implementation took a positional argument, so that syntax was carried forward into the go implementation.

C:\>wsk namespace get -h
usage: wsk-script.py namespace get [-h] [-u AUTH] [name]

positional arguments:
  name                  the namespace to list

mdeuser · 2018-01-10T13:52:18Z

commands/namespace.go

-        if err != nil {
-            whisk.Debug(whisk.DbgError, "Client.Namespaces.Get(%s) error: %s\n", getClientNamespace(), err)
+        errHandler := func(err error, kind string) error {
+            whisk.Debug(whisk.DbgError, "Client.%s.List(%s) error: %s\n", getClientNamespace(), kind, err)


the first %s in the Client.%s.List(%s) should be the entity name (i.e. Actions, Packages, etc) that matches the actual Client function name - not the namespace argument.

Thanks - order fixed.

mdeuser · 2018-01-10T13:56:30Z

commands/namespace.go

-
-        if err != nil {
-            whisk.Debug(whisk.DbgError, "Client.Namespaces.Get(%s) error: %s\n", getClientNamespace(), err)
+        errHandler := func(err error, kind string) error {


for consistency with other existing error handler defintions, i suggest moving this error handler outside of the RunE function definition... in this same module though.

mdeuser · 2018-01-10T14:03:49Z

if a user has permissions to access another namespace then wsk namespace get NAMESPACE should work with the go implementation.

>wsk namespace get /whisk.system
error: Unable to obtain the list of entities for namespace 'whisk.system': The supplied authentication is not authorized to access 'whisk.system'. (code 22018494)

rabbah · 2018-01-10T14:07:57Z

The behavior dates back to when a single key could access multiple spaces. This is no longer allowed so supporting this (wsk list x) doesn’t make sense anymore in the client.

mdeuser · 2018-01-10T14:44:03Z

@rabbah - as an aside, what are the current cross-namespace shareable entity types these days?

rabbah · 2018-01-10T14:46:46Z

public packages and nothing else.

Replaces call to GET /namespace/_ with calls to GET /namespace/_/[actions, triggers, rules, packages] Also updates wsk list/wsk namespace get to reject positional argument.

Remove redundant test. Fix go integration test.

dubee · 2018-01-11T00:59:31Z

commands/namespace.go

-
-            if len(qualifiedName.GetEntityName()) > 0 {
-                return entityNameError(qualifiedName.GetEntityName())
+        if (!(len(args) == 1 && args[0] == "/_")) {


This seems to clash a little with the usage info that states the namespace parameter is optional.

Also, there isn't a test for wsk namespace get /_.

there is, the CLI tests already do this: wsk namespace get /_ (that's how I found it).
I have to add support for /_ because there's a circular dependence between this repo and the main repo and can't break it otherwise.

dubee · 2018-01-11T01:00:18Z

commands/shared.go

@@ -33,3 +33,12 @@ func entityNameError(entityName string) (error) {

    return whisk.MakeWskError(errors.New(errMsg), whisk.EXIT_CODE_ERR_GENERAL, whisk.DISPLAY_MSG, whisk.DISPLAY_USAGE)
 }
+
+func entityListError(err error, namespace string, kind string) error {


dubee

Can you update the Godeps to pick up your commit from client-go? Changes here: https://github.com/apache/incubator-openwhisk-cli/blob/master/Godeps/Godeps.json#L68.

dubee

I think you will need to update this test in incubator-openwhisk too: https://github.com/apache/incubator-openwhisk/blob/4e5a8117f6c19776f3bcafcfe04e55f49ca843e8/tests/src/test/scala/system/basic/WskBasicTests.scala.

rabbah · 2018-01-11T01:11:59Z

go deps updated.

As for updating wsk basic tests in the main repo, see apache/openwhisk#3167. It can't pass until this PR is merged (unless I disable tests).

The tests shouldn't be running in the main repo and will be removed by apache/openwhisk#3103 and apache/openwhisk#2996.

If you have an better idea to break the circular dependence I'm all ears.

rabbah · 2018-01-11T01:17:12Z

To clarify, the tests in this repo have a dependence on the Wsk CLI client in the main repo: https://github.com/apache/incubator-openwhisk/blob/master/tests/src/test/scala/common/Wsk.scala#L761

That Wsk CLI binding generates the /_.
So there's no way to make the tests here pass by removing the position argument unless I remove the tests or doing more funky things.

rabbah requested a review from dubee January 9, 2018 22:11

dubee requested changes Jan 10, 2018

View reviewed changes

rabbah force-pushed the wsklist branch from 7028a85 to 3c8baa5 Compare January 10, 2018 04:06

mdeuser reviewed Jan 10, 2018

View reviewed changes

rabbah force-pushed the wsklist branch from 699a5f6 to 0a257cd Compare January 10, 2018 15:24

Handle namespace API listing on the client.

7f33e76

Replaces call to GET /namespace/_ with calls to GET /namespace/_/[actions, triggers, rules, packages] Also updates wsk list/wsk namespace get to reject positional argument.

rabbah force-pushed the wsklist branch 5 times, most recently from 326caf8 to a491ee2 Compare January 10, 2018 20:14

rabbah added 2 commits January 10, 2018 15:14

Update .gitignore.

2275197

Preserve wsk list /_ compatibility to break circular dependence.

a491ee2

Remove redundant test. Fix go integration test.

rabbah assigned dubee Jan 10, 2018

rabbah added the review label Jan 10, 2018

dubee reviewed Jan 11, 2018

View reviewed changes

dubee requested changes Jan 11, 2018

View reviewed changes

dubee reviewed Jan 11, 2018

View reviewed changes

Pick up changes in client lib.

c96139e

dubee approved these changes Jan 11, 2018

View reviewed changes

dubee merged commit 7b78091 into apache:master Jan 11, 2018

mrutkows mentioned this pull request Feb 20, 2018

Remove API for listing all entities in namespace apache/openwhisk#3167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle namespace API listing on the client. #191

Handle namespace API listing on the client. #191

rabbah commented Jan 9, 2018

dubee Jan 10, 2018

dubee Jan 10, 2018

dubee left a comment •

edited

Loading

rabbah commented Jan 10, 2018 •

edited

Loading

rabbah commented Jan 10, 2018

dubee commented Jan 10, 2018 •

edited

Loading

dubee commented Jan 10, 2018 •

edited

Loading

dubee commented Jan 10, 2018 •

edited

Loading

mdeuser commented Jan 10, 2018

mdeuser Jan 10, 2018

rabbah Jan 10, 2018

mdeuser Jan 10, 2018

mdeuser commented Jan 10, 2018

rabbah commented Jan 10, 2018

mdeuser commented Jan 10, 2018

rabbah commented Jan 10, 2018

dubee Jan 11, 2018

dubee Jan 11, 2018

rabbah Jan 11, 2018

dubee Jan 11, 2018

dubee left a comment

dubee left a comment •

edited

Loading

rabbah commented Jan 11, 2018 •

edited

Loading

rabbah commented Jan 11, 2018 •

edited

Loading

Handle namespace API listing on the client. #191

Handle namespace API listing on the client. #191

Conversation

rabbah commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubee left a comment • edited Loading

Choose a reason for hiding this comment

rabbah commented Jan 10, 2018 • edited Loading

rabbah commented Jan 10, 2018

dubee commented Jan 10, 2018 • edited Loading

dubee commented Jan 10, 2018 • edited Loading

dubee commented Jan 10, 2018 • edited Loading

mdeuser commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdeuser commented Jan 10, 2018

rabbah commented Jan 10, 2018

mdeuser commented Jan 10, 2018

rabbah commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubee left a comment

Choose a reason for hiding this comment

dubee left a comment • edited Loading

Choose a reason for hiding this comment

rabbah commented Jan 11, 2018 • edited Loading

rabbah commented Jan 11, 2018 • edited Loading

dubee left a comment •

edited

Loading

rabbah commented Jan 10, 2018 •

edited

Loading

dubee commented Jan 10, 2018 •

edited

Loading

dubee commented Jan 10, 2018 •

edited

Loading

dubee commented Jan 10, 2018 •

edited

Loading

dubee left a comment •

edited

Loading

rabbah commented Jan 11, 2018 •

edited

Loading

rabbah commented Jan 11, 2018 •

edited

Loading