[GitHub] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144573130
##
File path: tests/src/test/scala/system/packages/MessageHubFeedTests.scala
##
@@ -274,6 +274,67 @@ class MessageHubFeedTests
assert(matchingActivations.length == 0)
}
+ it should "return correct status and configuration" in
withAssetCleaner(wskprops) {
+val currentTime = s"${System.currentTimeMillis}"
+
+(wp, assetHelper) =>
+ val triggerName = s"/_/dummyMessageHubTrigger-$currentTime"
+ println(s"Creating trigger ${triggerName}")
+
+ val username = kafkaUtils.getAsJson("user")
+ val password = kafkaUtils.getAsJson("password")
+ val admin_url = kafkaUtils.getAsJson("kafka_admin_url")
+ val brokers = kafkaUtils.getAsJson("brokers")
+
+ createTrigger(assetHelper, triggerName, parameters = Map(
+"user" -> username,
+"password" -> password,
+"api_key" -> kafkaUtils.getAsJson("api_key"),
+"kafka_admin_url" -> admin_url,
+"kafka_brokers_sasl" -> brokers,
+"topic" -> topic.toJson,
+"isBinaryKey" -> false.toJson,
+"isBinaryValue" -> false.toJson
+ ))
+
+ val run = wsk.action.invoke(actionName, parameters = Map(
Review comment:
right. that is a required parameter for the action itself regardless of the
value of `lifecycleEvent`
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144572363
##
File path: tests/src/test/scala/system/packages/MessageHubFeedTests.scala
##
@@ -274,6 +274,67 @@ class MessageHubFeedTests
assert(matchingActivations.length == 0)
}
+ it should "return correct status and configuration" in
withAssetCleaner(wskprops) {
+val currentTime = s"${System.currentTimeMillis}"
+
+(wp, assetHelper) =>
+ val triggerName = s"/_/dummyMessageHubTrigger-$currentTime"
+ println(s"Creating trigger ${triggerName}")
+
+ val username = kafkaUtils.getAsJson("user")
+ val password = kafkaUtils.getAsJson("password")
+ val admin_url = kafkaUtils.getAsJson("kafka_admin_url")
+ val brokers = kafkaUtils.getAsJson("brokers")
+
+ createTrigger(assetHelper, triggerName, parameters = Map(
+"user" -> username,
+"password" -> password,
+"api_key" -> kafkaUtils.getAsJson("api_key"),
+"kafka_admin_url" -> admin_url,
+"kafka_brokers_sasl" -> brokers,
+"topic" -> topic.toJson,
+"isBinaryKey" -> false.toJson,
+"isBinaryValue" -> false.toJson
+ ))
+
+ val run = wsk.action.invoke(actionName, parameters = Map(
+"triggerName" -> triggerName.toJson,
+"lifecycleEvent" -> "READ".toJson,
+"authKey" -> wp.authKey.toJson
+ ))
+
+ withActivation(wsk.activation, run) {
+activation =>
+ activation.response.success shouldBe true
+
+ inside (activation.response.result) {
+case Some(result) =>
+ val config =
result.getFields("config").head.asInstanceOf[JsObject].fields
+ val status =
result.getFields("status").head.asInstanceOf[JsObject].fields
+
+ config should contain("brokers" -> brokers)
Review comment:
passed in as `/_/dummyTrigger-...` returned as `/guest/dummyTrigger-...`
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144406862
##
File path: action/kafkaFeedWeb.js
##
@@ -61,6 +61,35 @@ function main(params) {
resolve(common.webResponse(statusCode, body));
});
+} else if (params.__ow_method === "get") {
Review comment:
you'll notice the code between messageHubFeedWeb.js and kafkaFeedWeb.js is
entirely duplicated. i think it's definitely fair to get rid of the
duplication. but i don't think this is the the place to do it. it should be
done under a different issue/epic
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144410648
##
File path: action/lib/common.js
##
@@ -130,6 +130,30 @@ function deleteTrigger(endpoint, params, actionName) {
});
}
+function getTrigger(endpoint, params, actionName) {
+var options = {
+method: 'GET',
+url: getWebActionURL(endpoint, actionName),
+rejectUnauthorized: false,
Review comment:
not entirely sure though
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144410595
##
File path: action/lib/common.js
##
@@ -130,6 +130,30 @@ function deleteTrigger(endpoint, params, actionName) {
});
}
+function getTrigger(endpoint, params, actionName) {
+var options = {
+method: 'GET',
+url: getWebActionURL(endpoint, actionName),
+rejectUnauthorized: false,
Review comment:
not entirely sure. i think it might have something to do with accepting
self-signed certs https://github.com/request/request-promise/issues/225
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144409384
##
File path: tests/src/test/scala/system/packages/MessageHubFeedTests.scala
##
@@ -274,6 +274,67 @@ class MessageHubFeedTests
assert(matchingActivations.length == 0)
}
+ it should "return correct status and configuration" in
withAssetCleaner(wskprops) {
+val currentTime = s"${System.currentTimeMillis}"
+
+(wp, assetHelper) =>
+ val triggerName = s"/_/dummyMessageHubTrigger-$currentTime"
+ println(s"Creating trigger ${triggerName}")
+
+ val username = kafkaUtils.getAsJson("user")
+ val password = kafkaUtils.getAsJson("password")
+ val admin_url = kafkaUtils.getAsJson("kafka_admin_url")
+ val brokers = kafkaUtils.getAsJson("brokers")
+
+ createTrigger(assetHelper, triggerName, parameters = Map(
+"user" -> username,
+"password" -> password,
+"api_key" -> kafkaUtils.getAsJson("api_key"),
+"kafka_admin_url" -> admin_url,
+"kafka_brokers_sasl" -> brokers,
+"topic" -> topic.toJson,
+"isBinaryKey" -> false.toJson,
+"isBinaryValue" -> false.toJson
+ ))
+
+ val run = wsk.action.invoke(actionName, parameters = Map(
Review comment:
shouldn't that be a test for testing the action itself? i.e. failing with a
bad auth key is functionality that is inherent to invoking the action itself,
not invoking the action with the parameter `lifecycleEvent` set to `READ`. i'm
all for having this test if it doesn't exist for the provider already. but a
separate issue should be opened as it doesn't specifically relate to this
functionality. i.e. an action should never be able to be invoked without proper
authKey
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144408689
##
File path: tests/src/test/scala/system/packages/MessageHubFeedTests.scala
##
@@ -274,6 +274,67 @@ class MessageHubFeedTests
assert(matchingActivations.length == 0)
}
+ it should "return correct status and configuration" in
withAssetCleaner(wskprops) {
+val currentTime = s"${System.currentTimeMillis}"
+
+(wp, assetHelper) =>
+ val triggerName = s"/_/dummyMessageHubTrigger-$currentTime"
+ println(s"Creating trigger ${triggerName}")
+
+ val username = kafkaUtils.getAsJson("user")
+ val password = kafkaUtils.getAsJson("password")
+ val admin_url = kafkaUtils.getAsJson("kafka_admin_url")
+ val brokers = kafkaUtils.getAsJson("brokers")
+
+ createTrigger(assetHelper, triggerName, parameters = Map(
+"user" -> username,
+"password" -> password,
+"api_key" -> kafkaUtils.getAsJson("api_key"),
+"kafka_admin_url" -> admin_url,
+"kafka_brokers_sasl" -> brokers,
+"topic" -> topic.toJson,
+"isBinaryKey" -> false.toJson,
+"isBinaryValue" -> false.toJson
+ ))
+
+ val run = wsk.action.invoke(actionName, parameters = Map(
+"triggerName" -> triggerName.toJson,
+"lifecycleEvent" -> "READ".toJson,
+"authKey" -> wp.authKey.toJson
+ ))
+
+ withActivation(wsk.activation, run) {
+activation =>
+ activation.response.success shouldBe true
+
+ inside (activation.response.result) {
+case Some(result) =>
+ val config =
result.getFields("config").head.asInstanceOf[JsObject].fields
+ val status =
result.getFields("status").head.asInstanceOf[JsObject].fields
+
+ config should contain("brokers" -> brokers)
Review comment:
and i would still have to test fields individually for status as i cannot
know the value of the dateChanged field
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144408526
##
File path: tests/src/test/scala/system/packages/MessageHubFeedTests.scala
##
@@ -274,6 +274,67 @@ class MessageHubFeedTests
assert(matchingActivations.length == 0)
}
+ it should "return correct status and configuration" in
withAssetCleaner(wskprops) {
+val currentTime = s"${System.currentTimeMillis}"
+
+(wp, assetHelper) =>
+ val triggerName = s"/_/dummyMessageHubTrigger-$currentTime"
+ println(s"Creating trigger ${triggerName}")
+
+ val username = kafkaUtils.getAsJson("user")
+ val password = kafkaUtils.getAsJson("password")
+ val admin_url = kafkaUtils.getAsJson("kafka_admin_url")
+ val brokers = kafkaUtils.getAsJson("brokers")
+
+ createTrigger(assetHelper, triggerName, parameters = Map(
+"user" -> username,
+"password" -> password,
+"api_key" -> kafkaUtils.getAsJson("api_key"),
+"kafka_admin_url" -> admin_url,
+"kafka_brokers_sasl" -> brokers,
+"topic" -> topic.toJson,
+"isBinaryKey" -> false.toJson,
+"isBinaryValue" -> false.toJson
+ ))
+
+ val run = wsk.action.invoke(actionName, parameters = Map(
+"triggerName" -> triggerName.toJson,
+"lifecycleEvent" -> "READ".toJson,
+"authKey" -> wp.authKey.toJson
+ ))
+
+ withActivation(wsk.activation, run) {
+activation =>
+ activation.response.success shouldBe true
+
+ inside (activation.response.result) {
+case Some(result) =>
+ val config =
result.getFields("config").head.asInstanceOf[JsObject].fields
+ val status =
result.getFields("status").head.asInstanceOf[JsObject].fields
+
+ config should contain("brokers" -> brokers)
Review comment:
that would work except for the fact the trigger name as returned is not
equal to the trigger name passed in
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144407283
##
File path: action/kafkaFeedWeb.js
##
@@ -61,6 +61,35 @@ function main(params) {
resolve(common.webResponse(statusCode, body));
});
+} else if (params.__ow_method === "get") {
+const triggerURL = common.getTriggerURL(params.authKey,
params.endpoint, params.triggerName);
+
+return common.verifyTriggerAuth(triggerURL)
+.then(() => {
+db = new Database(params.DB_URL, params.DB_NAME);
+return db.getTrigger(params.triggerName);
+})
+.then((triggerDoc) => {
+var body = {
+config: {
+triggerName: triggerDoc.triggerName,
+topic: triggerDoc.topic,
+isJSONData: triggerDoc.isJSONData,
+isBinaryValue: triggerDoc.isBinaryValue,
+isBinaryKey: triggerDoc.isBinaryKey,
+isMessageHub: triggerDoc.isMessageHub,
+brokers: triggerDoc.brokers,
+kafka_admin_url: triggerDoc.kafka_admin_url,
+username: triggerDoc.username,
+password: triggerDoc.password
Review comment:
we will likely add flags in the future to adjust what is returned.
@csantanapr wants to get this initial work out before focusing on that as this
is highly desirable
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144407001
##
File path: action/kafkaFeedWeb.js
##
@@ -61,6 +61,35 @@ function main(params) {
resolve(common.webResponse(statusCode, body));
});
+} else if (params.__ow_method === "get") {
+const triggerURL = common.getTriggerURL(params.authKey,
params.endpoint, params.triggerName);
+
+return common.verifyTriggerAuth(triggerURL)
+.then(() => {
+db = new Database(params.DB_URL, params.DB_NAME);
+return db.getTrigger(params.triggerName);
+})
+.then((triggerDoc) => {
+var body = {
+config: {
+triggerName: triggerDoc.triggerName,
+topic: triggerDoc.topic,
+isJSONData: triggerDoc.isJSONData,
+isBinaryValue: triggerDoc.isBinaryValue,
+isBinaryKey: triggerDoc.isBinaryKey,
+isMessageHub: triggerDoc.isMessageHub,
+brokers: triggerDoc.brokers,
+kafka_admin_url: triggerDoc.kafka_admin_url,
+username: triggerDoc.username,
+password: triggerDoc.password
Review comment:
at this point in time yes. the user needs to be authenticated. this is no
different than fetching the service credentials for your instance of message
hub from bluemix
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] abaruni commented on a change in pull request #217: Add ability to get trigger configuration and status
abaruni commented on a change in pull request #217: Add ability to get trigger
configuration and status
URL:
https://github.com/apache/incubator-openwhisk-package-kafka/pull/217#discussion_r144406862
##
File path: action/kafkaFeedWeb.js
##
@@ -61,6 +61,35 @@ function main(params) {
resolve(common.webResponse(statusCode, body));
});
+} else if (params.__ow_method === "get") {
Review comment:
you'll notice the code between messageHubFeedWeb.js and kafkaFeedWeb.js is
entirely duplicated. i think it's definitely fair to get rid of the
duplication. but i don't this this is the the place to do it. it should be done
under a different issue/epic
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
