Correct unable to set apihost issue. #2463

lzbj · 2017-07-07T12:46:34Z

line 864 url.Parse caused failure.
Fixes #2411.

dubee · 2017-07-10T14:53:27Z

tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala

@@ -151,7 +151,7 @@ class WskBasicUsageTests
        val tmpwskprops = File.createTempFile("wskprops", ".tmp")
        try {
            val env = Map("WSK_CONFIG_FILE" -> tmpwskprops.getAbsolutePath())
-            val apihost = s"http://${WhiskProperties.getBaseControllerAddress()}"
+            val apihost = s"${WhiskProperties.getBaseControllerAddress()}"


@lzbj, need a test for setting the API host with and without a protocol.

@lzbj, also looks like the test failed because the server is expecting an http request instead of an https request.

I believe the original error comes from the url.Parse(urlBase) function.

Yes, correct, I'll update according to your comments. But When I change the "https" to "http", I got another deployment error, we should add "http" if the host not specified the protocol, right? @dubeejw

dubee · 2017-07-10T14:57:06Z

tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala

@@ -151,7 +151,7 @@ class WskBasicUsageTests
        val tmpwskprops = File.createTempFile("wskprops", ".tmp")
        try {
            val env = Map("WSK_CONFIG_FILE" -> tmpwskprops.getAbsolutePath())
-            val apihost = s"http://${WhiskProperties.getBaseControllerAddress()}"
+            val apihost = s"${WhiskProperties.getBaseControllerAddress()}"


@lzbj, also looks like the test failed because the server is expecting an http request instead of an https request.

dubee · 2017-07-10T14:58:22Z

tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala

@@ -151,7 +151,7 @@ class WskBasicUsageTests
        val tmpwskprops = File.createTempFile("wskprops", ".tmp")
        try {
            val env = Map("WSK_CONFIG_FILE" -> tmpwskprops.getAbsolutePath())
-            val apihost = s"http://${WhiskProperties.getBaseControllerAddress()}"
+            val apihost = s"${WhiskProperties.getBaseControllerAddress()}"


I believe the original error comes from the url.Parse(urlBase) function.

dubee · 2017-07-12T05:53:10Z

tools/cli/go-whisk-cli/commands/util.go

@@ -856,6 +856,10 @@ func getURLBase(host string, path string) (*url.URL, error)  {
        return nil, whiskErr
    }

+    if !strings.HasPrefix(host, "http") {
+        host = "http://" + host


What if the OpenWhisk deployment only supports https?

I believe the error is thrown blow on line 864 url.Parse.

correct, updated the code. If we can't avoid deployment using https, we can just add one test. do you have any suggestion we can add more tests?

rabbah · 2017-07-13T03:48:32Z

The cli must support both - there's a test for http as well as https which is the nginx path.

lzbj · 2017-07-14T03:53:13Z

Ok, thank you. Then now the test in WskBasicUsageTests is sufficient for the change, right?

rabbah

LGTM

mdeuser · 2017-07-19T17:05:16Z

tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala

@@ -148,7 +148,7 @@ class WskBasicUsageTests
        }
    }

-    it should "show api build using http apihost" in {
+    it should "show api build using http apihost with protocol specified" in {


@lzbj - is another successful test needed that provides the api host without the protocol being provided?

we can just add one http test, because the server expects http protocol, if we add another test without providing 'http', the test will fail. Do you have better idea?

this is the area of code i was considering

+ if !strings.HasPrefix(host, "http") { + host = "https://" + host + }

so i was thinking an apihost setting with and without a http prefix should work. will a protocol-less apihost fail?

yes, this line "I believe the error is thrown blow on line 864 url.Parse." will fail as @dubeejw comments.

dubee · 2017-08-16T18:12:54Z

tools/cli/go-whisk-cli/commands/util.go

@@ -841,6 +841,10 @@ func getURLBase(host string, path string) (*url.URL, error)  {
        return nil, whiskErr
    }

+    if !strings.HasPrefix(host, "http") {
+        host = "https://" + host


Need a test for this.

rabbah · 2017-09-08T03:30:54Z

@mdeuser @dubeejw @lzbj i pushed a commit which adds tests.

lzbj · 2017-09-08T05:05:35Z

@rabbah , thanks.

dubee · 2017-09-14T03:02:18Z

tests/src/test/scala/whisk/core/cli/test/WskConfigTests.scala

+          val apihost = s"$prefix$host"
+          withClue(apihost) {
+            val rr = wsk.cli(Seq("property", "set", "--apihost", apihost), env = env)
+            rr.stdout should not include regex("Application exited unexpectedly")


Errors are printed to stderr, so shouldn't need this check.

dubee

LGTM

rabbah · 2017-09-14T03:25:06Z

pg1/2055 🔵

lzbj mentioned this pull request Jul 7, 2017

Unable to set the api host #2411

Closed

lzbj force-pushed the unable_set_apihost branch 2 times, most recently from db77c02 to 36ae998 Compare July 10, 2017 08:36

dubee requested changes Jul 10, 2017

View reviewed changes

lzbj force-pushed the unable_set_apihost branch from 36ae998 to aa74457 Compare July 11, 2017 02:15

dubee requested changes Jul 12, 2017

View reviewed changes

lzbj force-pushed the unable_set_apihost branch from aa74457 to 8d9cbd0 Compare July 13, 2017 02:44

rabbah approved these changes Jul 19, 2017

View reviewed changes

rabbah added the cli label Jul 19, 2017

mdeuser reviewed Jul 19, 2017

View reviewed changes

dubee requested changes Aug 16, 2017

View reviewed changes

rabbah force-pushed the unable_set_apihost branch 2 times, most recently from c248c41 to 0b3cdeb Compare September 8, 2017 03:30

rabbah assigned csantanapr Sep 8, 2017

rabbah added the bug label Sep 8, 2017

dubee requested changes Sep 14, 2017

View reviewed changes

dubee reviewed Sep 14, 2017

View reviewed changes

Correct unable to set apihost issue.

4d92435

rabbah force-pushed the unable_set_apihost branch 2 times, most recently from 6943140 to dfe5cf8 Compare September 14, 2017 03:20

Test change for api host setting which previously caused CLI to crash.

dfe5cf8

dubee approved these changes Sep 14, 2017

View reviewed changes

dubee mentioned this pull request Sep 14, 2017

cli is oblivous to protocol set for apihost #2720

Closed

csantanapr merged commit 5856485 into apache:master Sep 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct unable to set apihost issue. #2463

Correct unable to set apihost issue. #2463

lzbj commented Jul 7, 2017 •

edited by rabbah

dubee Jul 10, 2017

dubee Jul 10, 2017

dubee Jul 10, 2017

lzbj Jul 11, 2017 •

edited

dubee Jul 10, 2017

dubee Jul 10, 2017

dubee Jul 12, 2017

dubee Jul 12, 2017

lzbj Jul 13, 2017

rabbah commented Jul 13, 2017

lzbj commented Jul 14, 2017

rabbah left a comment

mdeuser Jul 19, 2017

lzbj Jul 20, 2017

mdeuser Jul 20, 2017

lzbj Jul 21, 2017

dubee Aug 16, 2017

rabbah commented Sep 8, 2017

lzbj commented Sep 8, 2017

dubee Sep 14, 2017

dubee left a comment •

edited

rabbah commented Sep 14, 2017 •

edited

Correct unable to set apihost issue. #2463

Correct unable to set apihost issue. #2463

Conversation

lzbj commented Jul 7, 2017 • edited by rabbah

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzbj Jul 11, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah commented Jul 13, 2017

lzbj commented Jul 14, 2017

rabbah left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah commented Sep 8, 2017

lzbj commented Sep 8, 2017

Choose a reason for hiding this comment

dubee left a comment • edited

Choose a reason for hiding this comment

rabbah commented Sep 14, 2017 • edited

lzbj commented Jul 7, 2017 •

edited by rabbah

lzbj Jul 11, 2017 •

edited

dubee left a comment •

edited

rabbah commented Sep 14, 2017 •

edited