[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r136648021
##
File path: tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala
##
@@ -120,7 +120,7 @@ class WskBasicUsageTests
}
it should "include CLI user agent headers with outbound requests" in {
-val stdout = wsk.cli(Seq("list"), verbose = true).stdout
+val stdout = wsk.cli(Seq("list", "--auth", wskprops.authKey) ++
wskprops.overrides, verbose = true).stdout
stdout should include regex ("""\bUser-Agent\b":
\[\s+"CloudFunctions\-CLI/1.\d+.*""")
Review comment:
Testing something out, will change back shortly.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r136115291
##
File path: tools/cli/go-whisk/whisk/client.go
##
@@ -126,6 +127,10 @@ func NewClient(httpClient *http.Client, config *Config)
(*Client, error) {
config.Version = "v1"
}
+if len(config.UserAgent) == 0 || config.UserAgent == "/not set" {
Review comment:
the plugin doesn't use the same gradle build I believe (or maybe I didn't
build it correctly), so yes, it is not being set. I was thinking I can check
for this prior to appending the cli version in commands.go if we wanna move it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version header URL: https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r136079018 ## File path: tools/cli/go-whisk-cli/commands/commands.go ## @@ -30,6 +30,7 @@ import ( var client *whisk.Client const DefaultOpenWhiskApiPath string = "/api" +var UserAgent string = "OpenWhisk-CLI" Review comment: not a const so that it can be overwritten by the plugin etc This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r136078640
##
File path: tools/cli/go-whisk/whisk/client.go
##
@@ -126,6 +127,10 @@ func NewClient(httpClient *http.Client, config *Config)
(*Client, error) {
config.Version = "v1"
}
+if len(config.UserAgent) == 0 || config.UserAgent == "/not set" {
Review comment:
the CLI version, In my testing I saw the plugin will show `OW-Plugin/not
set`, so I added it here to cover the missing UserAgent case, aka "/not set"
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r133768141
##
File path: docs/rest_api.md
##
@@ -80,11 +80,14 @@ wsk namespace list -v
```
```
REQUEST:
-[GET] https://openwhisk.ng.bluemix.net/api/v1/namespaces
+[GET] https://openwhisk.ng.bluemix.net/api/v1/namespaces
Req Headers
{
"Authorization": [
"Basic XXX"
+ ],
+ "User-Agent": [
Review comment:
I backed out the changes referencing the rest api requiring User-Agent, but
this is an appropriate doc update for the CLI changes in my mind.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r133314355
##
File path: tests/src/test/scala/whisk/core/cli/test/WskWebActionsTests.scala
##
@@ -250,7 +172,7 @@ trait WskWebActionsTests
response.statusCode shouldBe 200
response.header("Access-Control-Allow-Origin") shouldBe "*"
response.header("Access-Control-Allow-Methods") shouldBe
"OPTIONS, GET, DELETE, POST, PUT, HEAD, PATCH"
-response.header("Access-Control-Allow-Headers") shouldBe
"Authorization, Content-Type"
+response.header("Access-Control-Allow-Headers") shouldBe
"Authorization, Content-Type, User-Agent"
Review comment:
Maybe I am confused, but I thought this user-agent header was for our own
tracking data, not any thing that is exposed to the user for customization.
Would enforcing this for web actions potentially hinder users from implementing
their own user-agent header? and/or also force them to add a user-agent to all
their web action calls?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r133007795
##
File path: tests/src/test/scala/whisk/core/cli/test/WskWebActionsTests.scala
##
@@ -47,94 +47,16 @@ import whisk.core.entity.Subject
* Tests web actions.
*/
@RunWith(classOf[JUnitRunner])
-class WskWebActionsTestsV1 extends WskWebActionsTests {
-override val testRoutePath = "/api/v1/experimental/web"
-}
-
-@RunWith(classOf[JUnitRunner])
-class WskWebActionsTestsV2 extends WskWebActionsTests with BeforeAndAfterAll {
-override val testRoutePath = "/api/v1/web"
-
-private val subdomainRegex =
Seq.fill(WhiskProperties.getPartsInVanitySubdomain)("[a-zA-Z0-9]+").mkString("-")
-
-private val (vanitySubdomain, vanityNamespace, makeTestSubject) = {
-if (namespace.matches(subdomainRegex)) {
-(namespace, namespace, false)
-} else {
-val s = Subject().asString.toLowerCase // this will generate two
confirming parts
-(s, s.replace("-", "_"), true)
-}
-}
-
-private val wskPropsForSubdomainTest = if (makeTestSubject) {
-getAdditionalTestSubject(vanityNamespace) // create new subject for
the test
-} else {
-WskProps()
-}
-
-override def afterAll() = {
-if (makeTestSubject) {
-disposeAdditionalTestSubject(vanityNamespace)
-}
-}
-
-"test subdomain" should "have conforming parts" in {
-vanitySubdomain should fullyMatch regex subdomainRegex.r
-vanitySubdomain.length should be <= 63
-}
-
-"vanity subdomain" should "access a web action via namespace subdomain" in
withAssetCleaner(wskPropsForSubdomainTest) {
-(wp, assetHelper) =>
-val actionName = "webaction"
-
-val file = Some(TestUtils.getTestActionFilename("echo.js"))
-assetHelper.withCleaner(wsk.action, actionName) {
-(action, _) => action.create(actionName, file, web =
Some(true.toString))(wp)
-}
-
-val url = getServiceApiHost(vanitySubdomain, true) +
s"/default/$actionName.text/a?a=A"
-println(s"url: $url")
-
-// try the rest assured path first, failing that, try curl with
explicit resolve
-Try {
-val response = RestAssured.given().config(sslconfig).get(url)
-val responseCode = response.statusCode
-responseCode shouldBe 200
-response.body.asString shouldBe "A"
-} match {
-case Failure(t) =>
-println(s"RestAssured path failed, trying curl: $t")
-implicit val tid = TransactionId.testing
-implicit val logger = new PrintStreamLogging(Console.out)
-val host = getServiceApiHost(vanitySubdomain, false)
-// if the edge host is a name, try to resolve it,
otherwise, it should be an ip address already
-val edgehost = WhiskProperties.getEdgeHost
-val ip =
Try(java.net.InetAddress.getByName(edgehost).getHostAddress) getOrElse "???"
-println(s"edge: $edgehost, ip: $ip")
-val cmd = Seq("curl", "-k", url, "--resolve", s"$host:$ip")
-val (stdout, stderr, exitCode) = SimpleExec.syncRunCmd(cmd)
-withClue(s"\n$stderr\n") {
-stdout shouldBe "A"
-exitCode shouldBe 0
-}
-
-case _ =>
-}
-}
-}
-
-trait WskWebActionsTests
-extends TestHelpers
-with WskTestHelpers
-with RestUtil {
+class WskWebActionsTests extends TestHelpers
+with WskTestHelpers with RestUtil with BeforeAndAfterAll {
val MAX_URL_LENGTH = 8192 // 8K matching nginx default
val wsk = new Wsk
private implicit val wskprops = WskProps()
val namespace = wsk.namespace.whois()
-protected val testRoutePath: String
+protected val testRoutePath: String = "/api/v1/web"
Review comment:
Is this appropriate here? or will testRoutePath be populated at this point?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r133056257
##
File path: tools/cli/go-whisk/whisk/client.go
##
@@ -126,6 +127,10 @@ func NewClient(httpClient *http.Client, config *Config)
(*Client, error) {
config.Version = "v1"
}
+if len(config.CLIVersion) == 0 {
+config.CLIVersion = "not_set"
+}
+
Review comment:
didn't check, just followed suit with the others, will test it out
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r133007795
##
File path: tests/src/test/scala/whisk/core/cli/test/WskWebActionsTests.scala
##
@@ -47,94 +47,16 @@ import whisk.core.entity.Subject
* Tests web actions.
*/
@RunWith(classOf[JUnitRunner])
-class WskWebActionsTestsV1 extends WskWebActionsTests {
-override val testRoutePath = "/api/v1/experimental/web"
-}
-
-@RunWith(classOf[JUnitRunner])
-class WskWebActionsTestsV2 extends WskWebActionsTests with BeforeAndAfterAll {
-override val testRoutePath = "/api/v1/web"
-
-private val subdomainRegex =
Seq.fill(WhiskProperties.getPartsInVanitySubdomain)("[a-zA-Z0-9]+").mkString("-")
-
-private val (vanitySubdomain, vanityNamespace, makeTestSubject) = {
-if (namespace.matches(subdomainRegex)) {
-(namespace, namespace, false)
-} else {
-val s = Subject().asString.toLowerCase // this will generate two
confirming parts
-(s, s.replace("-", "_"), true)
-}
-}
-
-private val wskPropsForSubdomainTest = if (makeTestSubject) {
-getAdditionalTestSubject(vanityNamespace) // create new subject for
the test
-} else {
-WskProps()
-}
-
-override def afterAll() = {
-if (makeTestSubject) {
-disposeAdditionalTestSubject(vanityNamespace)
-}
-}
-
-"test subdomain" should "have conforming parts" in {
-vanitySubdomain should fullyMatch regex subdomainRegex.r
-vanitySubdomain.length should be <= 63
-}
-
-"vanity subdomain" should "access a web action via namespace subdomain" in
withAssetCleaner(wskPropsForSubdomainTest) {
-(wp, assetHelper) =>
-val actionName = "webaction"
-
-val file = Some(TestUtils.getTestActionFilename("echo.js"))
-assetHelper.withCleaner(wsk.action, actionName) {
-(action, _) => action.create(actionName, file, web =
Some(true.toString))(wp)
-}
-
-val url = getServiceApiHost(vanitySubdomain, true) +
s"/default/$actionName.text/a?a=A"
-println(s"url: $url")
-
-// try the rest assured path first, failing that, try curl with
explicit resolve
-Try {
-val response = RestAssured.given().config(sslconfig).get(url)
-val responseCode = response.statusCode
-responseCode shouldBe 200
-response.body.asString shouldBe "A"
-} match {
-case Failure(t) =>
-println(s"RestAssured path failed, trying curl: $t")
-implicit val tid = TransactionId.testing
-implicit val logger = new PrintStreamLogging(Console.out)
-val host = getServiceApiHost(vanitySubdomain, false)
-// if the edge host is a name, try to resolve it,
otherwise, it should be an ip address already
-val edgehost = WhiskProperties.getEdgeHost
-val ip =
Try(java.net.InetAddress.getByName(edgehost).getHostAddress) getOrElse "???"
-println(s"edge: $edgehost, ip: $ip")
-val cmd = Seq("curl", "-k", url, "--resolve", s"$host:$ip")
-val (stdout, stderr, exitCode) = SimpleExec.syncRunCmd(cmd)
-withClue(s"\n$stderr\n") {
-stdout shouldBe "A"
-exitCode shouldBe 0
-}
-
-case _ =>
-}
-}
-}
-
-trait WskWebActionsTests
-extends TestHelpers
-with WskTestHelpers
-with RestUtil {
+class WskWebActionsTests extends TestHelpers
+with WskTestHelpers with RestUtil with BeforeAndAfterAll {
val MAX_URL_LENGTH = 8192 // 8K matching nginx default
val wsk = new Wsk
private implicit val wskprops = WskProps()
val namespace = wsk.namespace.whois()
-protected val testRoutePath: String
+protected val testRoutePath: String = "/api/v1/web"
Review comment:
Is this appropriate here? or will testRoutePath be populated at this point?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version header URL: https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132733987 ## File path: core/controller/src/main/scala/whisk/core/controller/WebActions.scala ## @@ -326,7 +326,7 @@ trait WhiskWebActionsApi private val defaultCorsResponse = List( `Access-Control-Allow-Origin`(AllOrigins), `Access-Control-Allow-Methods`(OPTIONS, GET, DELETE, POST, PUT, HEAD, PATCH), -`Access-Control-Allow-Headers`(`Authorization`.name, `Content-Type`.name)) +`Access-Control-Allow-Headers`(`Authorization`.name, `Content-Type`.name, `User-Agent`.name)) Review comment: rest_api.md has been updated This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132558633
##
File path: tools/cli/go-whisk/whisk/api.go
##
@@ -43,7 +43,6 @@ type ApiListRequestOptions struct {
Skipint `url:"skip"`
Docsbool `url:"docs,omitempty"`
}
-//type ApiListResponse RetApiArray
type ApiListResponse RetApiArray
Review comment:
Some lingering cleanup from the experimental-api removal PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132290510
##
File path: tools/cli/go-whisk-cli/main.go
##
@@ -48,6 +48,7 @@ func init() {
// Rest of CLI uses the Properties struct, so set the build time there
commands.Properties.CLIVersion = CLI_BUILD_TIME
+os.Setenv("CLI_Version", CLI_BUILD_TIME)
Review comment:
I would either need to inject the CLIVersion properly into
```go-whisk-cli/commands/property.go``` then import this into
```go-whisk/whisk/client.go```, or just import ```cli/go-whisk-cli/main.go```
into ```/cli/go-whisk/whisk/client.go``` and access the ```CLI_BUILD_TIME```
var from there...?
Not the greatest options imo
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132290510
##
File path: tools/cli/go-whisk-cli/main.go
##
@@ -48,6 +48,7 @@ func init() {
// Rest of CLI uses the Properties struct, so set the build time there
commands.Properties.CLIVersion = CLI_BUILD_TIME
+os.Setenv("CLI_Version", CLI_BUILD_TIME)
Review comment:
I would either need to inject the CLIVersion properly into
```go-whisk-cli/commands/property.go``` then import this into
```go-whisk/whisk/client.go```, or just import ```cli/go-whisk-cli/main.go```
into ```/cli/go-whisk/whisk/client.go```...?
Not the greatest options imo
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132290510
##
File path: tools/cli/go-whisk-cli/main.go
##
@@ -48,6 +48,7 @@ func init() {
// Rest of CLI uses the Properties struct, so set the build time there
commands.Properties.CLIVersion = CLI_BUILD_TIME
+os.Setenv("CLI_Version", CLI_BUILD_TIME)
Review comment:
I would either need to inject the CLIVersion properly into
```cli/go-whisk-cli/commands/property.go``` then import this into
```/cli/go-whisk/whisk/client.go```, or just import
```cli/go-whisk-cli/main.go``` into ```/cli/go-whisk/whisk/client.go```.
Not the greatest options imo
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] drcariel commented on a change in pull request #2591: User-Agent CLI/version header
drcariel commented on a change in pull request #2591: User-Agent CLI/version
header
URL:
https://github.com/apache/incubator-openwhisk/pull/2591#discussion_r132289217
##
File path: tools/cli/go-whisk-cli/main.go
##
@@ -48,6 +48,7 @@ func init() {
// Rest of CLI uses the Properties struct, so set the build time there
commands.Properties.CLIVersion = CLI_BUILD_TIME
+os.Setenv("CLI_Version", CLI_BUILD_TIME)
Review comment:
I was attempting this at first, but isn't as simple. The CLIVersion is
intentionally omitted from that config. I assume for good reason?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
